Port to TelepathyGLib

[Aniket21mathur]

Fixes https://github.com/sugarlabs/sugar/issues/836 This is an in progress pr.

[quozl]

Thanks. Reviewed 5834a7e.

Please also check flake8 for your changes. master has 82 flake8 warnings, and 5834a7e has 140. On the other hand, please do not fix flake8 warnings except where you have made changes.

A flake8 discovery of importance;

./src/jarabe/model/neighborhood.py:22:7: E999 SyntaxError: invalid syntax

I see again how the change to GObject introspection binding persuades us not to use names in global scope as we did before. Do you think we might instead do something like this;

from gi.repository import TelepathyGLib
CLIENT = TelepathyGLib.IFACE_CLIENT
CHANNEL_DISPATCHER = TelepathyGLib.IFACE_CHANNEL_DISPATCHER
...

Benefit would be reduced churn elsewhere in each file, making git bisect and git blame easier.

Noted in progress. Next time create the pull request as draft?

[Aniket21mathur]

./src/jarabe/model/neighborhood.py:22:7: E999 SyntaxError: invalid syntax

Thanks, same typo as @chimosky pointed out. I will fix it.

I see again how the change to GObject introspection binding persuades us not to use names in global scope as we did before. Do you think we might instead do something like this;

from gi.repository import TelepathyGLib
CLIENT = TelepathyGLib.IFACE_CLIENT
CHANNEL_DISPATCHER = TelepathyGLib.IFACE_CHANNEL_DISPATCHER
...

Yes, I agree with you, this would also reduce the work of replacing each telepathy constant appearance in the code.

Noted in progress. Next time create the pull request as draft?

Thanks will take care of that.

Also wanted to know what procedure you follow to test changes in sugar. Thanks

[Aniket21mathur]

Changes remaining to be done ~

• Remove dependency from Channel class.
• Remove dependency from Connection class.

@quozl @chimosky please review upto this and suggest a method to test.

Thanks!

[quozl]

Thanks. Won't test until all Telepathy static binding imports are removed. Let us know if there is anything blocking that.

[Aniket21mathur]

I observed that there are two files having Channel imports, filetransfer.py and neighborhood.py. Similarly there are four files having Connection imports buddy.py , filetransfer.py, neighbourhood.py and connection_watcher.py.

In sugar there is also use of some methods of Connection which TelepthyGLib.Connection do not provides, so we have to use the Connection class from the telepathy sources, this class also inherits the InterfaceFactory class. But since there 2 files for both Channel and Connection, and 2 more for Connection, I don't think it would be good to embed classes in each file, an alternative is to import.

@quozl @chimosky please guide how to proceed.

Thanks!

[quozl]

Change the use of the methods.

Looking at the sources of the static binding API;

• interfacefactory.py is only 85 lines, implements a dict. There's very little else this class does. As far as I can see, the keys of the dict (used by calling modules) are always constants, so the class has almost no justification to exist. Plain variables could be used instead.

• channel.py is only 59 lines, does something minimal and has two asynchronous callbacks; to get the channel type and interfaces.

• conn.py is only 113 lines, and also contains very little in the class Connection.

I'm not surprised if they have been removed, as they do very little, and steal somewhat some control from the programmer because of the asynchronous actions.

Here's what I suggest;

List the methods of Connection; by interactive inspection, and by documentation. Call this list one.

List the methods of TelepathyGLib.Connection; by interactive inspection, and by documentation. Call this list two.

List the methods of Connection that are used by the files buddy.py, filetransfer.py, neighbourhood.py and connection_watcher.py. Call this list three.

Correlate list one with list three. These are the methods known to be used.

Intersect list one with list two. These are the methods common to both APIs.

List those methods that are in the first list, but not the second list. These are the methods for which a replacement is yet unknown. Call this list four.

Discover what those methods in list four are for; i.e. what is their semantic. Discover in each source file how and why those methods are used. Perhaps they are used only because it was the documented way to implement.

Using list two, discover what similar methods are in TelepathyGLib.Connection. By interactive inspection, by documentation, and by source code analysis of both APIs.

Change the use of the methods in the files buddy.py, filetransfer.py, neighbourhood.py and connection_watcher.py.

Repeat the above for Channel.

Once you've solved it, make the same changes to sugar-toolkit-gtk3 and collabwrapper modules.

(Remember, you're on track; this is exactly what was in the first step of the Port to Python 3 project idea. Don't worry about it taking longer than you planned. That's your inexperience and unfamiliarity, and we all have that at some stage.)

[Aniket21mathur]

Thanks, got your point. I agree with you. I had a look at the files, and I noticed that they are always 2 to 3 instances of Channel class or Connection class in a file using the same methods. I don't think it would be a good idea to write same code again and again for each instance. What if we merge the Channel or Connection class with the Interfacefactory class, removing and modifying code as per our use, into a single class. I think It will make the code cleaner and easy to review and debug? I have made the changes accordingly in https://github.com/sugarlabs/collabwrapper/pull/14/commits/1ec617ab53dc017a1ccfbfe4da554c949ea7c42d Please suggest.

[Aniket21mathur]

Tested sugar head 5027d3008a872a0a0f143333f9d46da1a901657d with sugar-toolkit-gtk3 head https://github.com/sugarlabs/sugar-toolkit-gtk3/commit/b3eb9869abf6a43ea96738892db5b422e1ba8c37 with Chat activity using two instances of Oracle virtual machines on bridge network running Ubuntu 18.04. The behaviour of neighbourhood view was fine and collaboration worked. I am still able to reproduce #840 . Thanks!

[Aniket21mathur]

I have pushed eba6dbeb5b3399a49c926a2470fbb260d29f3c72. I made a test with toolkit at https://github.com/sugarlabs/sugar-toolkit-gtk3/commit/64a229689905685822bee6e1514ce39a7fafa1e2 and with Chat https://github.com/Aniket21mathur/chat/commit/89c94a3587a443b868d9f09d48bae122a4e032fe, using virtual machines, though I am not able to see the X0 icon of the other instance in the neighbourhood view, I am still working on it. :-)

Still I request a review, might help in figuring out what is wrong. Thanks!

[Aniket21mathur]

One thing that I noticed is that the changes are not able to handle asynchronous calls. For eg. .. in buddy.py

def __name_owner_changed_cb(self, name, old, new):
        if name.startswith(CONNECTION + '.') and not old and new:
            path = '/' + name.replace('.', '/')
            obj = dbus.Bus().get_object(name, path)
            dbus.Interface(obj, CONNECTION).GetInterfaces(
                    reply_handler=self._get_interfaces_reply_cb,
                    error_handler=self.__error_handler_cb)
            self.__connection_ready_cb(obj)

This function is called and before getting response from .GetInterfaces self.__connection_ready_cb(obj) is called, which results in undesired behaviour.

[quozl]

Not surprising, as you have changed when the connection ready callback is made. It was made when the connection was ready. Now you have made it occur instantly.

I'm looking at https://github.com/sugarlabs/sugar/pull/837/files in the patch band from lines 113 onwards.

[Aniket21mathur]

Tested https://github.com/sugarlabs/sugar/pull/837/commits/7a7c533cd780ee77349a5c3e791b5ac5cf9bc2da with https://github.com/sugarlabs/sugar-toolkit-gtk3/commit/64a229689905685822bee6e1514ce39a7fafa1e2 and https://github.com/Aniket21mathur/chat/commit/89c94a3587a443b868d9f09d48bae122a4e032fe.

Used two oracle virtual machines on a bridged network running Ubuntu 18.04. Everything looks good, neither I am able to reproduce https://github.com/sugarlabs/sugar/issues/840 . :smile:

Thanks!

[quozl]

Thanks, that's especially good that #840 does not happen, though it is unfortunate that we don't know why.

I've looked again at the aggregate change, and got a little lost. I'm not sure if you intend every little change, as we have adjusted our goals in the 19 days the pull request has been open.

Could you please do a self-review of https://github.com/sugarlabs/sugar/pull/837/files on a change by change basis and see if there is anything you can do that reduces the amount of change?

Also, please do a test of journal object file transfer from one Sugar user to another?

[quozl]

Tested with CollabWrapperTest.

Issue #840 does occur for me.

Several interesting errors in shell.log of Sugar.

http://dev.laptop.org/~quozl/z/1hfeN1.txt is inviter user. http://dev.laptop.org/~quozl/z/1hfeMl.txt is invitee user.

[Aniket21mathur]

Could you please do a self-review of https://github.com/sugarlabs/sugar/pull/837/files on a change by change basis and see if there is anything you can do that reduces the amount of change?

Yes, I will review it again, and will remove extra changes.

Also, please do a test of journal object file transfer from one Sugar user to another?

Never used that feature before, but I will try testing that.

Tested with CollabWrapperTest.

Thanks for testing.

Issue #840 does occur for me.

Thanks, yes indeed it is occurring for me as well, might be possible I didn't reproduced it correctly at that time.

Several interesting errors in shell.log of Sugar.

I will have a look into them :-)

[Aniket21mathur]

I am not able to test file transfer, between the users, didn't get how to do that. I followed this tutorial. Need help.

[Aniket21mathur]

Tested https://github.com/sugarlabs/sugar/pull/837/commits/934c72a781adc25a4c15dbd4db398454c0d7dde3 with toolkit at https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/412/commits/a7245f9d9ab723a32b8bdd008509361c4d8124b6 and Chat master.

I am not able to test file transfer, between the users, didn't get how to do that. I followed this tutorial. Need help

I am still not able to test file transfer.

Thanks!

[quozl]

Here's how to test;

1. ensure buddy icons in neighbourhood view, make friend, and create a journal object, such as a Write document,

2. on the sending system, in the Journal, right-click and "Send to" the buddy representing the receiving system,

3. on the receiving system, a notification will appear in top left corner, press F6 for Frame, right-click on the download icon, and click accept; the journal object will be copied,

4. on the sending system, press F6 for Frame, right-click on the upload icon, and click dismiss,

5. on the receiving system, press F6 for Frame, right-click on the upload icon, and click dismiss,

6. validate the content of the journal object by opening it.

Look at shell.log for any interesting errors or warnings.

https://help.sugarlabs.org/en/journal.html#sending-journal-entries-via-a-network is a bit brief, it could do with more detail.

[Aniket21mathur]

Thanks. Tested Collaboration as well as File transfer with https://github.com/sugarlabs/sugar/pull/837/commits/30c11eb80daaf6d97a44e294652c76e8d7725ae4. Looks fine.

[Aniket21mathur]

Look at shell.log for any interesting errors or warnings

Surprising, but I don't get any related to collaboration. Way I followed~

• First test collaboration and file transfer using Sugar v 0.112.
• Test collaboration and file transfer with head of toolkit and sugar telepathy port pull request.

Compare the logs.

Updates

I am getting some occassional errors:

Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/jarabe/model/filetransfer.py", line 344, in file_transfer_available
    CONNECTION_INTERFACE_REQUESTS)
  File "/usr/lib/python2.7/dist-packages/dbus/proxies.py", line 145, in __call__
    **keywords)
  File "/usr/lib/python2.7/dist-packages/dbus/connection.py", line 651, in call_blocking
    message, timeout)
DBusException: org.freedesktop.DBus.Error.ServiceUnknown: The name :1.17 was not provided by any .service files
1561778074.643586 ERROR root: org.freedesktop.DBus.Error.ServiceUnknown: The name :1.17 was not provided by any .service files
Traceback (most recent call last):

and

1561779534.626020 ERROR root: __handle_with_reply_cb DBusException(dbus.String(u'Method "HandleWith" with signature "s" on interface "org.freedesktop.Telepathy.ChannelDispatchOperation" doesn\'t exist\n'),)

[Aniket21mathur]

I am able to reproduce both of the errors with the static bindings. Please see #843 and #842. Thanks!

[quozl]

Good, thanks. They shouldn't be kept. They hinder testing. When will you fix them?

[Aniket21mathur]

Good, thanks. They shouldn't be kept. They hinder testing. When will you fix them?

843 is a result of #840. I tried to fix #840 some days back, was not able to figure out the problem. I will look into it again.

I will work on #842 soon.

Thanks!

[quozl]

Tested 0b9bcef.

• private invitation to Chat, passed,
• file transfer, failed, no friend in send-to menu,

Logs contained;

1562629120.239359 ERROR root: org.freedesktop.DBus.Error.ServiceUnknown: The name :1.18 was not provided by any .service files
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/jarabe/model/filetransfer.py", line 344, in file_transfer_available
    CONNECTION_INTERFACE_REQUESTS)
  File "/usr/lib/python2.7/dist-packages/dbus/proxies.py", line 145, in __call__
    **keywords)
  File "/usr/lib/python2.7/dist-packages/dbus/connection.py", line 651, in call_blocking
    message, timeout)
DBusException: org.freedesktop.DBus.Error.ServiceUnknown: The name :1.18 was not provided by any .service files

This is adjacent to one of your changes, so any ideas?

[Aniket21mathur]

Thanks for testing. Please see #843 , the treceback you reported is reproducible with the static bindings and is caused by #840 as we discussed before :-)

[quozl]

Thanks. Reproduced; #840 happened in the previous test step; when Chat was stopped.