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

Port to Python3, Gtk3 and GLib #5

Closed
wants to merge 4 commits into from

Conversation

Saumya-Mishra9129
Copy link
Member

I have done with porting . Solved the issue #3 . Kindly review

This was referenced Mar 23, 2020
@@ -1,8 +1,8 @@
[Activity]
name = Frotz
activity_version = 3
bundle_id = vu.lux.olpc.Frotz
exec = sugar-activity frotz.FrotzActivity
bundle_id = org.sugarlabs.Frotz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason why you changed the bundle id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was giving bundle_id not found error when I tested with python2. But after making changes the error has gone. The activity was not starting before with python2. You can try this one on python2 by changing. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasons not to change the bundle_id include;

  • when an upgrade of an activity occurs on a system, Journal objects from the old version can continue to be used on the new version; otherwise those Journal objects cannot be opened,
  • when an activity is shared between a system using the old bundle_id and another system using the new bundle_id, the sharing will work; otherwise the shared instance will not be displayed in the Neighbourhood View,
  • when an upgrade happens while Sugar is running, Sugar will represent only one activity by this name on the menu; otherwise Sugar may represent both the old and new activities on the menu at once if there is a copy of the activity in ~/Activities as well as /usr/share/sugar/activities,
  • locating source code repositories is easy because we can search for the name; otherwise it becomes harder.

I don't think a bundle_id not found error is a correct reason to change the bundle_id. Next time that happens, try to diagnose why it happened rather than jumping to conclusions about the cause. 😁

@@ -187,28 +188,28 @@ def _paste_cb(self, button):
self._vte.paste_clipboard()

def __key_press_cb(self, window, event):
if event.state & gtk.gdk.CONTROL_MASK and event.state & gtk.gdk.SHIFT_MASK:
if event.state & Gtk.gdk.CONTROL_MASK and event.state & Gtk.gdk.SHIFT_MASK:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an incomplete port, look for the correct method and make the necessary changes.
If you're not sure what the correct methods are you can check here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I will take a look at these ones.


if gtk.gdk.keyval_name(event.keyval) == "C":
if Gtk.gdk.keyval_name(event.keyval) == "C":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too.

if self._vte.get_has_selection():
self._vte.copy_clipboard()
return True
elif gtk.gdk.keyval_name(event.keyval) == "V":
elif Gtk.gdk.keyval_name(event.keyval) == "V":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

Comment on lines +239 to +240
self.set_colors(Gtk.gdk.color_parse (fg_color),
Gtk.gdk.color_parse (bg_color),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this too.

vte = cdll.LoadLibrary("lib/%s/libvte.so.9" % vte_path)
sys.path.append("lib/%s" % vte_path)
vte = cdll.LoadLibrary("/usr/lib/"+vte_path+"-linux-gnu/libvte-2.91.so.0")
sys.path.append("/usr/lib/"+vte_path+"-linux-gnu" )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why you added this to the PATH?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The activity was giving error before given in #4 in python2. The error is gone after changing . You can try this one also. by running on sugar-live-build. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The activity was written to work with early versions of Fedora that did not have up to date VTE package. Newer VTE was in the lib directory, for each architecture supported. It also happened in the Lemonade activity. This method was acceptable at the time but was not compliant with the license of VTE, and we no longer need to do it because VTE is sufficiently recent on any system that has Python 3 GTK support. So as you port to GTK 3, you should have assessed if the embedded copy of VTE was needed, and (as it is not) removed it and the code that refers to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And of course remove the lib directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I didn't know that. I thought the error was just related to not loading the required lib. So I just made necessary changes for working. I will take care about it further.

@chimosky
Copy link
Member

chimosky commented Mar 23, 2020

A good rule of thumb is to make sure the activity starts before you start making any changes to it, some changes you made above should be in a separate PR because they help fix the activity before a port to python3, also do test your changes before making a PR.

I normally use the sugar-live-build to test my changes, how did you test yours?

@Saumya-Mishra9129
Copy link
Member Author

The activity with python 2 was not starting before as It was giving that bundle_id not found error and import vte error (#4 ). I have made that changes but stuck at #6 which was not because of python 3 port.
I am using sugar-live-build to test my changes. If you can help me with #6 it will be great. Thanks for review.

@quozl
Copy link
Contributor

quozl commented Mar 24, 2020

The activity with python 2 was not starting before as It was giving that bundle_id not found error and import vte error (#4 ). I have made that changes but stuck at #6 which was not because of python 3 port.
I am using sugar-live-build to test my changes. If you can help me with #6 it will be great. Thanks for review.

Our guide to porting asks that you fix the activity properly on Python 2 before you try porting it to Python 3. We want working code for our users to use; porting to Python 3 is a minor step in that goal.

@Saumya-Mishra9129
Copy link
Member Author

@chimosky @quozl I have checked the activity is working properly with python 2 . I am closing this pull request and will send a new one with necessary changes. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants