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

Allow activities to pass file transfers without shell notification #621

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samdroid-apps
Copy link
Contributor

File transfer channels are a powerful solution for transfering data
1-to-1 over telepathy. However, currently the shell will try to
claim all file transfer channels and show a notification to the
user.

This commit makes the shell ignore the channel if it has the
content type of 'x-sugar/from-activity'. It will instead pass it
to the telepathy client specified in the file name. The file name
was used to house the client name because there does not appear
to be a dedicated place for this information in telepathy.
Activities can still use the description to store their metadata.

Note: This patch makes a lot more sense with the new collab wrapper patch. There is lots of lines changed in the collab wrapper to use file transfers. I need to review those myself to before I can share that. I will update this pull request when that happens.

File transfer channels are a powerful solution for transfering data
1-to-1 over telepathy.  However, currently the shell will try to
claim all file transfer channels and show a notification to the
user.

This commit makes the shell ignore the channel if it has the
content type of 'x-sugar/from-activity'.  It will instead pass it
to the telepathy client specified in the file name.  The file name
was used to house the client name because there does not appear
to be a dedicated place for this information in telepathy.
Activities can still use the description to store their metadata.
@samdroid-apps samdroid-apps added this to the 0.108 Features milestone Dec 5, 2015
@samdroid-apps
Copy link
Contributor Author

Could somebody please test this? It would be great if this is merged in 108, as this is useful for better usage of the collab wrapper.

You can easily test with physics activity from git [1]. To test, run physics ans share it with a buddy. If the buddy does have the patch applied:

  1. The activity should still sync the state normally on joining
  2. There should be no file transfer notification when joining

[1] https://github.com/walterbender/physics

@quozl
Copy link
Contributor

quozl commented Jan 6, 2016

Unfortunately I can't test physics at the moment due to 4926.

@tchx84
Copy link
Member

tchx84 commented Jan 6, 2016

I will give it a try.

@tchx84
Copy link
Member

tchx84 commented Jan 8, 2016

Hey @samdroid-apps, can you clarify something?

I tested collaboration with physics (master [1]) using two laptops (fedora 23 x86_64 without broot), with and without this patch, and there was no difference at all:

With or without the patch the result is the same:

  1. fails to sync the initial state, showing the following error during:

    Traceback (most recent call last):
    File "/usr/lib64/python2.7/site-packages/dbus/connection.py", line 230, in maybe_handle_message
      self._handler(*args, **kwargs)
    File "/home/tch/Devel/sugar-build/activities/physics/collabwrapper.py", line 776, in _received_cb
      self._activity_cb(buddy, msg)
    File "/home/tch/Devel/sugar-build/activities/physics/collabwrapper.py", line 278, in __received_cb
      data = self.activity.get_data()
    File "/usr/lib64/python2.7/site-packages/gi/overrides/GObject.py", line 550, in _unsupported_data_method
      raise RuntimeError('Data access methods are unsupported. '
    RuntimeError: Data access methods are unsupported. Use normal Python attributes instead
    

    This happens with and without the patch being applied on both clients. Note that it does sync for newly created objects.

  2. No file transfer notification can be seen, with and without the patch.

Ideas?

[1] https://github.com/walterbender/physics

@samdroid-apps
Copy link
Contributor Author

My apologies, I did not notice that physics does not have get_data or set_data (initial state sync). This does not appear to be a regression [1].

I have double checked that Bibliography activity is a suitable testing activity, as it uses the initial state sync and sends the file transfer. Please find bibliography version 4 the git repository [2]. It will hopefully soon be available on the ASLO (although even v3 is still in the review queue) [3].

Sorry for the bad test case.

[1] sugarlabs/physics@e80f755
[2] https://github.com/SAMdroid-apps/bibliography-activity
[3] http://activities.sugarlabs.org/en-US/sugar/addon/4761/

@tchx84
Copy link
Member

tchx84 commented Jan 11, 2016

hey @samdroid-apps,

I tested with [1] and now I see differences, but there still one thing off:

  • without the patch: the other peer syncs fine, but there is a Sugar file transfer notification (in the frame). This was expected.
  • with the patch: the other peer syncs fine and there is no Sugar file transfer notification, BUT there is a regular save-to-disk dialog. This was not expected.

Are you not seeing this? I suspect there could logic hole.

[1] https://github.com/SAMdroid-apps/bibliography-activity

@samdroid-apps
Copy link
Contributor Author

Hi @tchx84, thanks for testing.

Can you share the logs (for shell and bibliography)? Logs with the log level set to debug would be awsome!

I tested on fc24 over salut (not gabble, see samdroid-apps/collabwrapper#2) and observed the intended behaviour using the following procedure:

  1. Start new bibliography activity on Alice's computer
  2. Alice to add a bibliography entry
  3. Alice invites Bob to the activity
  4. Bob joins the activity
  5. Bob waits for the Alice's bibliography activity to become visible
  6. Steps 1 to 5 are repeated but with Alice and Bob's roles reversed

I did not observe any file transfer notifications or file transfer popups (empathy).

Was your procedure significantly different from the one above?

@tchx84
Copy link
Member

tchx84 commented Jan 13, 2016

The only difference, in my test case, is that Alice don't invite Bob. Bob sees the shared activity in the neighborhood and joins.

Everything else is identical, except for the result.

Ill try to make some time to repeat these tests before Friday, but please try it with fc23 if you have it. We are just days away from the next release.

@samdroid-apps
Copy link
Contributor Author

Ok, so I attempted to test this with a GNOME Boxes fedora 23 to my fedora 24 computer over salut. It couldn't connect at all.

I'll investigate an alternate way of testing, as I assume that the boxes network setup may have been causing issues.

@tchx84
Copy link
Member

tchx84 commented Jan 21, 2016

@samdroid-apps I use VirtualBox, setting network device to "bridged". Very useful for this kind of tests.

@samdroid-apps
Copy link
Contributor Author

Heh, virtualbox kernel module doesn't build on rawhide. I'm trapped in the instability!

I tried installing on an XO, but how would you guess that mixing and matching fedora ans sugar versions would not end well.

I'll try and find another way, but please send through the logs if you can @tchx84!

@quozl
Copy link
Contributor

quozl commented Jan 21, 2016

@samdroid-apps wrote:

I tried installing on an XO, but how would you guess that mixing and matching
fedora ans sugar versions would not end well.

Why not? This mixing works fine for me. Try harder?

@tchx84
Copy link
Member

tchx84 commented Jan 23, 2016

@samdroid-apps you mean the virtual box additions? I never needed them for testing this.

Anyway, lets keep focusing on this fix, so we land it before the final release.

@davelab6
Copy link
Contributor

davelab6 commented Apr 4, 2016

Did this land in the final release?

@quozl
Copy link
Contributor

quozl commented Apr 4, 2016

@davelab6, the simplest method to check is to read the first patch band, locate the file, and examine the text to see if the patch was applied. For _new_channels_cb functiion in filetransfer.py, as of now, the patch hasn't been applied.

Also, for users who may merge pull requests, GitHub shows a "This branch has no conflicts with the base branch. Merging can be performed automatically." If the patch or a derivative was merged in another pull request, GitHub would likely show a conflict and we would ask for a rebase.

@davelab6
Copy link
Contributor

davelab6 commented Apr 5, 2016

@quozl thank you for explaining :D

@samdroid-apps
Copy link
Contributor Author

samdroid-apps commented Apr 30, 2016

I should test this. I setup an old machine with fc23 soas just to do this, but then I forgot completely. I'll try again tommorow.

@i5o
Copy link
Contributor

i5o commented Nov 15, 2016

This was supposed to be in 0.108 D:

Can you test @samdroid-apps ?

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.

5 participants