Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements Send File for both IM and CHAT conversations. #85

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jaroslawp
Copy link
Collaborator

Since we can send images , lets extend it and allow sending any files to chat/im conversations: unfortunately somehow adding ui menu to chat 'Conversation' menu does not work (lines ~ 379) , therefore in chats files can be sent using '/sendfile' command while in im convs -> using Conversation -> Send File menu ...

@EionRobb
Copy link
Owner

What about implementing it via the file transfer api? Then you'd be able to drag-drop files to send on the conversation window

@jaroslawp
Copy link
Collaborator Author

I think that in order to implement drag&drop plugin has to implement PIDGIN_PLUGIN_TYPE ? (not sure .. but for example purple_blist_get/set_ui_ops() does not work at present either ? ).

This PR uses transfer api (well, just a part of it, purple 'thinks' that the transfer finished OK just after we read the file and are about to start sending it to MM). I will have a look at how to implement proper update_progress and destroy callbacks,

What I wanted to have is 'Send File' option in conversation window: that works for buddies, but does not for chats: see around line 370 of libmattermost.c -> I get there a chat blist node .. but it points 'nowhere' - does not have any chat components set .. etc. as a result we were coming to mm_send_file_menu_cb .. not knowing from which chat we are called ..not sure how to solve that. .. - that's why the /sendfile command for chats ..

(iterating over all conversations and checking purple_conversation_has_focus() ? ..)

@EionRobb
Copy link
Owner

The commented out code there looks good. In what case does the bnode struct in mm_send_file_menu_cb() not have the chat components?

@EionRobb
Copy link
Owner

Looking at other code that uses the blistnode functions for chats, it can be possible for the id to not be set as a component if its not saved to the buddy list. Other code falls-back to using purple_chat_get_name()/purple_chat_get_name_only() - can we do the same here?

@EionRobb
Copy link
Owner

Using purple_conversation_has_focus() isn't very portable and will cause issues with some UI's. Why can't you look up the chat by its name?

@jaroslawp
Copy link
Collaborator Author

'''purple_chat_get_name'()'' returns chat alias - which could have been changed by user.. so not good here. However I think I found a workaround searching for focused conversation window and getting chat id from there - please check if that works for libpurple 3 ..

In what case does the bnode struct in mm_send_file_menu_cb()
not have the chat components?
When called via selecting from Conversation > More > Send File

@EionRobb
Copy link
Owner

Ah yeah, I forgot that purple_chat_get_name has a slightly different behaviour between purple2 and purple3 :)

I've just tried Conversation->More->Leave Chat on another prpl, and it has all the chat components. How did you join the chat? Was it opened from the buddy list or was it opened from the Tools->Room Listi menu?

@jaroslawp
Copy link
Collaborator Author

Opened from buddy list. (BTW: checked purple_chat_get_name(PURPLE_CHAT(bnode))

  • that returns NULL too ..)

@jaroslawp
Copy link
Collaborator Author

.. just discovered that it is possible to initiate transfer with undefined who in purple_xfer_new ..

@jaroslawp
Copy link
Collaborator Author

Using purple_conversation_has_focus() isn't very portable and will cause issues with some UI's.

Indeed, not portable: but if UI does not implement it the 'Send File' action will not work, in that case maybe just presenting a message that it is not implemented and enabling /sendfile would be a workaround until better implementation can be found ?

@jaroslawp
Copy link
Collaborator Author

OK, current version of patch enables /sendfile command for both chat and im convs and adds menu item Send File for both too: if given UI does not implement purple_conversation_has_focus and error message is presented asking user to use /sendfile in chat channel instead of menu selection.

@thorgrin
Copy link
Collaborator

thorgrin commented May 29, 2019

I've tried the code and when I send a file to a peer, it is corrupted. The size is OK, but checksum does not match.

Edit: I've located the problem and created a pull request for this branch.

When receiving file, I get [error: public links disabled on server, cannot get file: Test.txt]. Does that mean that the server can be reconfigured so that this will work?

@thorgrin
Copy link
Collaborator

thorgrin commented Jun 3, 2019

Is there anything preventing this PR from merging? It works for me, but I cannot test every platform.

@EionRobb
Copy link
Owner

EionRobb commented Jun 3, 2019

The main issues I have with the PR is the inconsistent UI with the file transfer API. Also, re-reviewing the code, it looks like it tries to load the entire file into memory, rather than sending it in chunks. The latest change in 2a56dcd looks like it only looks at the local_filename which there may not necessarily be, depending on the UI. The UI is allowed to pass through an fd or a local_filename (or both).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants