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.

Sign up for GitHub

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

Jump to bottom

[In Progress] Port to TelepathyGlib #17

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

[In Progress] Port to TelepathyGlib #17

wants to merge 1 commit into from

Conversation

tonadev
Copy link

@tonadev tonadev commented Nov 17, 2018

Explanation

Fixes #13. This PR ports to TelepathyGLib.

Reason

The telepathy library does not have its bindings for Python 3, and porting Telepathy to its PyGObject binding is a pre requisite for the Port to Python 3 Project.

Test result

No error related to the port to TelepathyGLib.

tonadev@TDPC:~/Documents/Work/OpenSource/code_in/sugarlabs/cookie-search-activity$ sugar-activity

(sugar-activity:9813): Gtk-WARNING **: 13:31:59.407: Theme parsing error: gtk-widgets.css:16:32: The style property GtkExpander:expander-size is deprecated and shouldn't be used anymore. It will be removed in a future version

(sugar-activity:9813): Gtk-WARNING **: 13:31:59.407: Theme parsing error: gtk-widgets.css:17:35: The style property GtkExpander:expander-spacing is deprecated and shouldn't be used anymore. It will be removed in a future version

@rhl-bthr rhl-bthr mentioned this pull request Nov 17, 2018
Closed
@rhl-bthr
Copy link

Thanks. Reviewed.
Looks good to me

@quozl
Copy link
Contributor

quozl commented Nov 17, 2018

Thanks. Reviewed. Looks fine. I'll be able to test on Monday (Australia).

@quozl
Copy link
Contributor

quozl commented Nov 19, 2018

Tested on Fedora 18. No new errors in logs. Game playing board state is mostly shared, but not fully; differences are;

  • when a guess for empty is made, proximity indicators are only shown to the player that made the guess for empty,
  • when a guess for not-empty is made, the cookie only appears on the player that made the guess for non-empty,
  • when a win occurs, the array of smiley faces and the new game prompt is shown to the player that made the last move, but not the other player,
  • after a new game begins, the proximity indicators from the previous game may still be present,
  • if both players complete the game, and pick new game at different times, and one player makes a guess before the second player picks new game, the results are inconsistent.

Hope that helps!

@tonadev tonadev changed the title Port to TelepathyGlib [In Progress] Port to TelepathyGlib May 12, 2019
@tonadev
Copy link
Author

tonadev commented May 12, 2019

I am not working on this currently. Anyone who wishes to take this up, please feel free to do so.

@Aniket21mathur
Copy link

Aniket21mathur commented May 23, 2019 •
edited
Loading

Tried to test it using rdesktop. Successfully made two instances,but not able to connect the two activities. Using Ubuntu 18.04. In the neighbourhood view, after hovering on other user I am not getting an option of "Invite to Cookie-search", but I am getting it for Chat. And if any error results,are they seen through the log activity?
@quozl ?

Aniket21mathur
Aniket21mathur reviewed May 23, 2019
View reviewed changes
SearchActivity.py
@@ -206,28 +209,28 @@ def _joined_cb(self, activity):

def _new_tube_common(self, sharer):
""" Joining and sharing are mostly the same... """
if self._shared_activity is None:
if self.get_shared_activity() is None:
Copy link

@Aniket21mathur Aniket21mathur May 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is this change necessary? Can this change might be a reason for the port not to work properly? @quozl @tonadev .
I have seen other ports and didn't noticed it in any.

Copy link
Contributor

Choose a reason for hiding this comment

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

self._shared_activity is in the GTK 2 toolkit sugar-toolkit, and is not in the GTK 3 toolkit sugar-toolkit-gtk3.

self.get_shared_activity is in both toolkits.

My guess is that the code hadn't been changed during the port to GTK 3, before the port to TelepathyGLib. If we're not sure if collaboration worked for the GTK 2 release of the activity, the way to be sure is to test that release.

Copy link

Choose a reason for hiding this comment

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

Yes. I also think that collaboration should be tested for GTK 2 first, It seems that there are not much changes from telepathy to TelepathyGlib for this activity, and the differences @quozl pointed out might be seen before telepathy port.

Game playing board state is mostly shared, but not fully; differences are;

Now seen a couple of instances where collaboration code is not properly ported to GTK 3 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is endemic. Root cause is missing-in-action activity maintainers, and well-meaning volunteers during GCI or students in GSoC who have concentrated on a GTK 3 port without fully understanding the collaboration feature, and reviewers with the same limited focus. Another cause is activities copied from other activities that do collaboration, without implementing collaboration in the new activity. Is why we re-worded the project idea this time, and @nswarup14 is doing "Maintain 25 activities" rather than "Port to GTK 3".

@quozl
Copy link
Contributor

quozl commented May 24, 2019

I don't think it is worth testing collaboration with this activity, given that there is so little shared state. I suggest testing Write and Chat first.

@Aniket21mathur
Copy link

Reviewed it again. Comments -

  • telepathy is used in only SearchActivity.py as per the pull request.
  • Only constants are imported from telepathy. I again compared the constants of TelepathyGLib and telepathy, their values match.
  • I don't think the Port to TelepathyGLib provided by this pr breaks collaboration, their should be some thing broken before the port.
    Thanks!

@quozl
Copy link
Contributor

quozl commented Jun 12, 2019

Yes, the port is not yet needed because the activity does not yet work with collaboration. See also #18. If that issue can't be fixed, then the fragmentary collaboration should be stripped out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aniket21mathur Aniket21mathur Aniket21mathur left review comments

@quozl quozl quozl left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Port to TelepathyGLib
4 participants
@tonadev @rhl-bthr @quozl @Aniket21mathur