Port to TelepathyGLib

[Aniket21mathur]

Fixes #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 sugarlabs/collabwrapper@1ec617a
Please suggest.

[Aniket21mathur]

Tested sugar head 5027d30 with sugar-toolkit-gtk3 head sugarlabs/sugar-toolkit-gtk3@b3eb986 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 eba6dbe. I made a test with toolkit at sugarlabs/sugar-toolkit-gtk3@64a2296 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 7a7c533 with
sugarlabs/sugar-toolkit-gtk3@64a2296 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 #840 . 😄

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 934c72a with toolkit at sugarlabs/sugar-toolkit-gtk3@a7245f9 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 30c11eb. 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?

[quozl]

Reviewed as-at 30c11eb.

[quozl]

Function name does not use our style; it is not a callback, so the name should not end in _cb.

[Aniket21mathur]

Thanks for noticing.

[quozl]

This is a synchronous call to D-Bus; can it be made asynchronous?

This method of asking D-Bus to list names, only to locate names that represent connections, causes traffic on D-Bus. Could we instead keep a list of connections and iterate through that?

[Aniket21mathur]

Thanks. Made a list of connections, to iterate through.

[quozl]

No, that's not what I meant. I'll try again.

In master the ConnectionWatcher calls get_connections, a method of the Connection class. In client/conn.py of telepathy-python sources, this is revealed to be a synchronous call to ListNames. Each detected name is then used to create a new Connection.

Synchronous calls are to be avoided because they can force interprocess context switches, clearing processor caches, and delay response by the application that makes the call. So I'm asking if we can avoid synchronous calls by either avoiding the ListNames call altogether (by caching our own list of connections), or if we are not in command of the list of connections (e.g. because they can be created by other processes), at least make the call to ListNames asynchronous, which would require changes to respond to the completion event.

However, if you can say "ListNames completes without requiring action by other processes", I'd say leave it synchronous.

Is there an equivalent feature in TelepathyGLib that we can look at to understand, though not to use?

[Aniket21mathur]

or if we are not in command of the list of connections

Yes we are not in command of that I have checked that with developing list of services with and without sugar. Also the same service have unique id's for different users.

[dbus.String(u'org.freedesktop.Telepathy.Connection.salut.local_xmpp.e26599a6') 

The 8 digit hash at the last differ for different users.

[Aniket21mathur]

No, that's not what I meant. I'll try again.

Thanks. I think I understood your point this time. What we want is to make the synchronous ListNames call an asynchronous call. I have pushed 0b9bcef in favor of that, please review.

[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.