Port to TelepathyGLib

[Aniket21mathur]

@quozl @chimosky, please review.
Fixes #1

[quozl]

Tested as at e130234 and toolkit as at sugarlabs/sugar-toolkit-gtk3@01cfd5f on Ubuntu 19.04 KVM virtual machines.

• logs contain a warning, please add gi.require_version('TelepathyGLib', '0.12') before import

Function was correct. Well done.

Reviewed.

• please rename the two new classes to prefix with underscore, as the caller of the module (the activity) is not expected to use them.

[chimosky]

Tested e130234 and toolkit as at sugarlabs/sugar-toolkit-gtk3@01cfd5f on Sugar live build virtual machines.

• log of owner shows get_data after quitting activity.

It'll be great to change e130234 to use imports once sugarlabs/sugar-toolkit-gtk3#412 is merged, but it won't be necessary if you follow James' review.

[Aniket21mathur]

@quozl @chimosky thanks for testing, made the changes @quozl suggested.

• log of owner shows get_data after quitting activity.

@chimosky sorry I am not able to understand, can you please share the full traceback.

It'll be great to change e130234 to use imports once sugarlabs/sugar-toolkit-gtk3#412 is merged, but it won't be necessary if you follow James' review.

Yes, I did had that in my mind, but I also agree with what James said in sugarlabs/sugar-toolkit-gtk3#412

In the back of my mind also is how we can avoid the Channel and InterfaceFactory classes, as they exist now because we used the previous API features. Having looked at the content of the classes, and seeing how little they contain, I'm wondering if it would make sense to remove them and cascade the change to what calls them. Once we have sugar ported, I hope to have a closer look.

[quozl]

Reviewed 75c7f78 changes. Noted the new import of dbus for PROPERTIES_IFACE, but there remains a reference that is prefixed with dbus., and one without.

The get_data in log is from test/activity.py and is unrelated to the port.

[Aniket21mathur]

Thanks, fixed it.

[Aniket21mathur]

@quozl I merged the two classes, removing all the unused methods please have a look. Thanks!

[quozl]

Yes, I did too, see 58a4ba7 ... but without a test harness, it can't be tested, so I'm working on that now.

[Aniket21mathur]

Thanks, reviewed. I did notice some methods which are (most probably) not used in the collabwrapper code as far as I studied collabwrapper.py. I removed them in my commit. :-)

[quozl]

Yes, but I wanted to prove they weren't used. ;-) Adding a send button to test activity.

[Aniket21mathur]

Ohh! Thanks. :-)

[quozl]

So far, I've discovered that;

• __contains__ is not used,
• get_channel_type_reply_cb and get_interfaces_reply_cb are not used because ready_handler is None.

It isn't clear to me why __getitem__ looks up D-Bus interface names; i.e. why are there two sets of keys?

I'm still working on testing.

The only activity that uses this feature is ImageViewer.

https://github.com/quozl/collabwrapper/commits/trim contains my work in progress for changes to the test activity.

The Gio.MemoryOutputStream instance misbehaves, or the code does something wrong;

Traceback (most recent call last):
  File "/home/guest/Activities/CollabWrapperTest.activity/activity.py", line 209, in on_notify_state_cb
    stream.close(None)
gi.repository.GLib.Error: g-io-error-quark: Stream has outstanding operation (20)

Seems unrelated, but I'd rather not merge something that can't be proven to work.

[Aniket21mathur]

Thanks a lot for testing.

So far, I've discovered that;

• __contains__ is not used,
• get_channel_type_reply_cb and get_interfaces_reply_cb are not used because ready_handler is None.

Agreed.

It isn't clear to me why __getitem__ looks up D-Bus interface names; i.e. why are there two sets of keys?

I'm still working on testing.

I am also not sure about that, since it is something taken from the telepathy sources, I just verified whether the method is called or not. The only difference we created was to embed the classes from the sources into our code. So I expect the methods to work as they are working before, provided with correct arguments (which we are ensuring).

The Gio.MemoryOutputStream instance misbehaves, or the code does something wrong;

Traceback (most recent call last):
  File "/home/guest/Activities/CollabWrapperTest.activity/activity.py", line 209, in on_notify_state_cb
    stream.close(None)
gi.repository.GLib.Error: g-io-error-quark: Stream has outstanding operation (20)

I reviewed the changes in your branch. This Traceback also seems unrelated to me.

[quozl]

I am also not sure about that, since it is something taken from the telepathy sources

Now that InterfaceFactory is removed, the next step is to remove the Channel class, so we'll be using the TelepathyGLib API directly.

I reviewed the changes in your branch.

Thanks.

This Traceback also seems unrelated to me.

True, but critical to fix, otherwise we can't test what you've done. If it weren't for ImageViewer using this code, I'd remove it instead.

[Aniket21mathur]

True, but critical to fix, otherwise we can't test what you've done. If it weren't for ImageViewer using this code, I'd remove it instead.

I looked into the sources and found out this

if (stream->priv->pending)
    {
      g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_PENDING,
                           /* Translators: This is an error you get if there is
                            * already an operation running against this stream when
                            * you try to start one */
                           _("Stream has outstanding operation"));
      return FALSE;
    }

  stream->priv->pending = TRUE;
  return TRUE;
}

I think there is something wrong in the way the code is written. Don't you think we should put this line stream.close(None) in an else against the if ?.

                if stream.has_pending():
                    logging.error('stream has pending actions')
                stream.close(None)

Please also have a look at the first answer here and also please give it a read.

I also noticed that the share to my neighbourhood button for ImageViewer is also disabled.
Thanks!

[Aniket21mathur]

Now that InterfaceFactory is removed, the next step is to remove the Channel class, so we'll be using the TelepathyGLib API directly.

By your statement I understand embedding the variables in the code, so that there is no need of calling the classes. Please see my comment in sugar pull request.
Thanks!

[quozl]

Cause of the "Stream has outstanding operation" is the close on the Gio.OutputStream while the asynchronous splice is still underway. In turn the close is coded to occur in response to the Telepathy FileTransfer state change notification for FT_STATE_COMPLETED. The port to TelepathyGLib has changed the timing.

Putting it another way, the sequence is;

• the FileTransfer is begun,
• the FT_STATE_OPEN notification occurs,
• the UNIX socket is created, the output stream created, and the asynchronous splice begun,
• the end of file is written to the UNIX socket,
• the FT_STATE_COMPLETED notification occurs,
• the attempt to close the output stream fails due to pending actions (the splice),
• the end of file is read from the UNIX socket by the splice,
• the splice completes,
• the output stream is closed (due to Gio.OutputStreamSpliceFlags.CLOSE_TARGET),
• the data is ready.

Perhaps instead of relying on the FileTransfer state change, a __splice_done_cb should be added to the splice_async call. I'm testing that.

[quozl]

quozl@eb69ed7 is the fix to "Stream has outstanding operation".

[Aniket21mathur]

Thanks, reviewed. Should I cherry pick your commit into my branch?

[quozl]

You are always welcome to do that, but there are other commits in the branch to consider as well. The new file "Send" test passes with all commits in the branch. I'm next looking into how to get rid of _Channel and use TelepathyGLib directly.

[Aniket21mathur]

You are always welcome to do that, but there are other commits in the branch to consider as well. The new file "Send" test passes with all commits in the branch

Thanks!

I'm next looking into how to get rid of _Channel_ and use TelepathyGLib directly.

Yes, I am working on that too. :-)

[quozl]

It seems _Channel implements using dbus. Sequence is;

• dbus.Bus().get_object,
• dbus.Interface is used via __getitem__ to make a GetChannelType RPC,
• dbus.Interface is used via __getitem__ to make a GetInterfaces RPC,

Then it acts as a dictionary linking interface names to _dbus.Interface_a, except for the special case of the default interface.

It seems unnecessarily complicated. We can't keep using it, as we are bypassing TelepathyGLib by using D-Bus directly, so any future changes to TelepathyGLib may conflict. We should be able to create a TelepathyGLib.Channel just like Chat activity does.

The tests and example code in the Telepathy sources may be of use.

I'm stopping now. I'll look at it briefly tomorrow morning my time, then next chance will be on Monday afternoon. Have fun! 😁

[Aniket21mathur]

Thanks. So we have to remove the use of bus.get_object(service_name, object_path) and
dbus.Interface(self._dbus_object, name) ?

[quozl]

Thanks. So we have to remove the use of bus.get_object(service_name, object_path) and
dbus.Interface(self._dbus_object, name) ?

That's an odd way to put it.

• this code is your copy and paste from the Telepathy Python static binding,
• the TelepathyGLib introspection binding may ultimately use the same methods.

The goal was to port from the static binding import telepathy to the introspection binding from gi.repository import TelepathyGLib. Copying and pasting from the Telepathy Python static binding doesn't succeed in porting away from it; instead it is acquired without import.

[Aniket21mathur]

Thanks @quozl, got your point.

The way, I am proceeding does not include use of TelepathyGLib.Channel, instead I am using dbus.Interface. So wanted to ask whether this approach is right or not?

Please guide.

I tried with TelepathyGLib.Channel but not able to figure out how to use it.

[quozl]

Why don't we create a new TelepathyGLib.Connection instance for use in TelepathyGLib.Channel. TelepathyGLib.Connection.new requires three arguments TelepathyGLib.DBusDaemon, bus_name and object_path. All three are available to us.

Yes. And this change is to be in sugar-toolkit-gtk3. It probably can't be done in CollabWrapper alone, otherwise the activity would have two connections; one created by the toolkit, one created by the activity. They would not share relevant state or configuration.

My guess is that this would take many seven-hour days of hard work and testing, but it is necessary to complete in order to port to Python 3 without continuing to use the Python 2 Telepathy API.

Going that way, we first have to do something like "Remove dependency from DBus" ?

If by that you mean remove all use of import dbus, yes. But you would also have to audit all function calls to isolate any that use a D-Bus object returned by the Telepathy APIs. When you look at the toolkit code in detail, you'll see a lot of things that may need to change.

[Aniket21mathur]

Thanks. I did had a look into the toolkit source code, realising that a major code change will be involved.
Can you please explain how do we proceed next ?

[quozl]

If by that you mean remove all use of import dbus, yes.

Correction; all use of import dbus that bypasses the Telepathy API. Using D-Bus for other functionality is necessary and fine.

[quozl]

Thanks. I did had a look into the toolkit source code, realising that a major code change will be involved.
Can you please explain how do we proceed next ?

If you like, but I'm surprised you need to ask. 😁

• list each use of D-Bus, and classify as either independent of Telepathy API (e.g. using sugar-datastore), necessary use of Telepathy API (e.g. preparing a properties), or bypasses Telepathy API,
• replace each use that bypasses Telepathy API with a call to the Telepathy API.

You'll need fairly complete knowledge of every Telepathy feature that Sugar uses.

Cause of this mess is that Sugar was written a long time ago when the Telepathy API was inadequate or required use of D-Bus, and since then the Telepathy API has changed without Sugar changing enough. Until your work on this, I was unaware the problem was so large.

[quozl]

I had said above;

I don't think using D-Bus is right approach.
...
I think what is needed is;

• rework sugar-toolkit-gtk3 to use TelepathyGLib only, without using D-Bus, i.e. not dig underneath the API,

Can I have review of this by @pro-panda and @chimosky please? Arguments for leaving the use of D-Bus in place are;

• in sugar-toolkit back in 2010, the chosen design was to use D-Bus via dbus-python,
• it is working now, why break it,
• Telepathy provides a documented D-Bus API as well as telepathy-python and TelepathyGLib APIs,
• we'll never get away from D-Bus, because even TelepathyGLib has to integrate with it,
• we might get the TelepathyGLib.Connection instance some other way,

[Aniket21mathur]

Wanted to add some inputs of mine 😅

• we might get the TelepathyGLib.Connection instance some other way,

Yes that might be possible in this way:-

1. Make a TelepathyGLib.Connection object specifically to use for TelepathyGLib.Channel. We have all the required parameters available.
2. Make TelepathyGLib.Channel object. My research on DBus and TelepathyGLib in the last couple of days left me with some TelepathyGLib methods that we can use as a replacement.

And Yeah, one more point.

1. If we want to continue using D-Bus, TelepathyGLib.Channel is not required nor did Channel class of static bindings.
2. What Channel does is that it returns a dbus object with the a TelepathyGLib constant. Instead of calling dbus through Channel we can directly call it, like done in connectionmanager.py file of sugar3.

Ultimately we will have a source code using TelepathyGLib API's , having Dbus to read the Telepathy Connection.

Waiting for @chimosky and @pro-panda to review.

[quozl]

Thanks. I've been reading TelepathyGLib and dbus-python docs too. Also, just found the PyGObject D-Bus API, it is hiding in Gio.DBus*, and the APIs named DBus and DBusGLib are very bare. What a mess. 😁

[Aniket21mathur]

• we'll never get away from D-Bus, because even TelepathyGLib has to integrate with it,

Yes. Agreed. Ultimately Telepathy is a D-Bus API. TelepathyGLib is a Telepathy specific API that was designed to make Telepathy use easier. 😄 .

[quozl]

See section 20.4 of the overview of Telepathy’s architecture, by Danielle Madeley.

In her view,

• TelepathyGLib is a high-level binding which includes elements of a low-level binding. I'd say these are things like constant strings.
• telepathy-python is a low-level binding.

So I think our problem is we have a code base which is mostly using the D-Bus API, but some parts have used the low-level binding instead of D-Bus. If those parts are few and far between, we might instead convert them to using D-Bus API.

[Aniket21mathur]

So I think our problem is we have a code base which is mostly using the D-Bus API, but some parts have used the low-level binding instead of D-Bus. If those parts are few and far between, we might instead convert them to using D-Bus API.

Yes. Agreed. Those includes parts having call to Channel and Connection classes. We can replace them with D-bus directly. I think we were indirectly doing that by embedding Channel and Connection classes in our code, as they themselves use D-Bus API?

What about constants ? Can we continue using them through TelepathyGLib?

[quozl]

Yes, I'm happy with using constants from TelepathyGLib. Acquiring them can be simpler, and more like how we acquired from telepathy-python;

from gi.repository.TelepathyGLib import IFACE_CHANNEL, \
  IFACE_CHANNEL_INTERFACE_GROUP, \
  IFACE_CHANNEL_TYPE_TEXT, \
  IFACE_CHANNEL_TYPE_FILE_TRANSFER \
...

or

from gi.repository.TelepathyGLib import \
    IFACE_CONNECTION_INTERFACE_ALIASING as CONN_INTERFACE_ALIASING, \
...

[Aniket21mathur]

Thanks. I like the second way. It wil prevent us from doing the hardwork of replacing constants everywhere in the file, and will also make it easier to review. 😅 .

[rhl-bthr]

Thanks, I've gone through this discussion. No comments

[quozl]

Thanks. Briefly reviewed ee5f646. You still have a use of TelepathyGLib functions and classes; the call to TelepathyGLib.Connection.new, can you please replace this with calls through the Telepathy D-Bus API? I'll be back on Monday.

[Aniket21mathur]

can you please replace this with calls through the Telepathy D-Bus API?

Done.
Thanks for reviewing!

[quozl]

On review, 74072c8 didn't seem to implement any calls through D-Bus. On test, CollabWrapper instances did not share state, and logs contained ``` Traceback (most recent call last): File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 234, in __joined_cb self._setup_text_channel() File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 248, in _setup_text_channel self.shared_activity.obj, AttributeError: 'Activity' object has no attribute 'obj' ``` Test configuration; * sugarlabs/sugar-toolkit-gtk3@1233eec * sugarlabs/sugar@7a7c533 * 74072c8

[Aniket21mathur]

Thanks, Activity did have an 'obj' attribute please see line https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/412/files#diff-03de8f54944bb6037c70d8ec36f91c15R266.

On review, 74072c8 didn't seem to implement any calls through D-Bus.

Thanks reviewed it again, I didn't find any calls through telepathy classes, replaced them by dbus, did I miss anything?

[quozl]

Thanks. Cause was /usr/local and /usr both containing different copies of Toolkit. In trying to remedy that, the colour and grade prompts appear, because of "Error parsing public key", in turn because your Toolkit branch is based on the master branch about a year ago. Testing again, the invitee instance of CollabWrapper gets this in logs Traceback (most recent call last): File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 237, in __joined_cb self.post({'action': ACTION_INIT_REQUEST}) File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 376, in post self._text_channel.post(msg) File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 738, in post self._send(json.dumps(msg)) File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 744, in _send if self._text_chan is not None: AttributeError: '_TextChannelWrapper' object has no attribute '_text_chan' and ping does not work. Traceback (most recent call last): File "/home/guest/Activities/CollabWrapperTest.activity/activity.py", line 112, in _test_clicked_cb self._collab.post({'action': 'echo-request', 'text': now}) File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 376, in post self._text_channel.post(msg) File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 738, in post self._send(json.dumps(msg)) File "/home/guest/Activities/CollabWrapperTest.activity/collabwrapper.py", line 744, in _send if self._text_chan is not None: AttributeError: '_TextChannelWrapper' object has no attribute '_text_chan' Looking at your change to TelepathyGLib.Connection.new, you have replaced it with a get_object call. 74072c8 I've not yet tested this part of the code; it will need the changes to the CollabWrapperTest activity from my trim branch. So I guess you haven't tested it either?

[quozl]

Please cherry-pick 8a13f02 ("Add send test"). I've tested that it applies cleanly on top of 74072c8.

[quozl]

Tested c0f568f with 464538f928644a9eed8e115f57b685839a5f0450 and; * ping did work, * send did work.

[Aniket21mathur]

in turn because your Toolkit branch is based on the master branch about a year ago.

Yes, as I continued my work from sugarlabs/sugar-toolkit-gtk3#401.

Thanks fixed the traceback, did the changes in my local while testing, but forgot to push. 😅

've not yet tested this part of the code; it will need the changes to the CollabWrapperTest activity from my trim branch. So I guess you

No I have not tested that part.

[Aniket21mathur]

@quozl your changes in quozl@0da858c to master are sufficient. So should I close this pr?

[quozl]

Replaced by #15.