Python3

[JuiP]

@quozl @pro-panda please review.
Tested works without errors

• Port to python 3

• Fix PyGIWarnings

• Fix Robot mode - broken after port to Gtk+3

• Port telepathy static bindings to TelepathyGLib

• Update Collabwrapper with latest changes in collab test activity

• Port from GObject to GLib

• Collaboration seems to be broken before the port

   `` PathsActivity.py line 261
      AttributeError: 'PathsActivity' object has no attribute ' _shared_activity' ``
      Will open another PR to fix this
  

[chimosky]

You could also take the opportunity and port GObject to GLib.

[JuiP]

@chimosky should I port to GLib in the same PR or open another PR?

[chimosky]

You can add it here.

[quozl]

Tested works without errors
* [ ] Collaboration seems to be broken before the port
`` PathsActivity.py line 261 AttributeError: 'PathsActivity' object has no attribute ' _shared_activity' ``
Will open another PR to fix this

That doesn't look like testing without errors. Please fix this. I don't think it needs to be in a separate pull request. The error is common; failure to change to get_shared_activity during a port to GTK 3 during GCI 2016, possibly because of hasty review that didn't include collaboration testing.

[JuiP]

That doesn't look like testing without errors. Please fix this. I don't think it needs to be in a separate pull request. The error is common; failure to change to get_shared_activity during a port to GTK 3 during GCI 2016, possibly because of hasty review that didn't include collaboration testing.

It seemed to me like the previous port to collabwrapper was not completed and merged hastily. I have made the required changes in another branch Collaboration, which I have later merged in this PR.

• Tested the changes on two VMs connected over a virtual network. The logs are clean, however the collaboration is still broken.

• The shared activity does not share the same screen (activity grid). Maybe I should raise an issue for this?

• Port from GObject to GLib

Apart from that I added few new translations at https://translate.sugarlabs.org/projects/Paths/ a while ago, when I refreshed pot file and run the translation bash earlier, many translations were not fetched. What have I done wrong?

Please test and review. @quozl @chimosky

[quozl]

Thanks for the update.

• Tested the changes on two VMs connected over a virtual network. The logs are clean, however the collaboration is still broken.
• The shared activity does not share the same screen (activity grid). Maybe I should raise an issue for this?

An issue is for recording a problem that you are not going to fix. It is an invitation to others to fix it. For problems you are going to fix, just say so in the pull request and we'll wait until you're ready with a fix. When you need help to fix a problem while in the middle of a pull request, the best place to ask is the pull request (for context), and the second best place is sugar-devel@ mailing list (for wider audience).

It isn't clear what you plan to do; work on a fix, or give up the pull request for the time being until someone else has a fix.

If you are not sure about how collaboration works, look at source code of other activities that successfully collaborate. Maze is a good but complex example; using TextChannelWrapper but not CollabWrapper. Memorize is a good example using CollabWrapper. Level is another good example that goes a step further to use CollabWrapper with a TCP/IP UDP broadcast.

Apart from that I added few new translations at https://translate.sugarlabs.org/projects/Paths/ a while ago, when I refreshed pot file and run the translation bash earlier, many translations were not fetched. What have I done wrong?

You haven't finished updating the translations. I don't know what you did, so I can't critique your process. When I used my process on top of your branch here, I pushed the result as 72e9f4f ("translate.sugarlabs.org synchronisation"). My changes demonstrate what you hadn't updated. Your new translations are seen, thanks!

[JuiP]

It isn't clear what you plan to do; work on a fix, or give up the pull request for the time being until someone else has a fix.

I don't think fixing collaboration under the PR - port to python 3 is a good idea, since it was broken even before the port. I planned to club changes related to porting, for reference later(if required). However, I do understand that making changes ultimately needs to make the activity usable and the order of changes doesn't matter much. If you insist I could try fixing the broken collaboration under this PR. Maybe could you possibly test the changes made till the latest commit ( 7065fcc)?

You haven't finished updating the translations. I don't know what you did, so I can't critique your process.

I did use the same script you used to update translations. Let me elaborate:
Saved the script as trans.sh, made the script executable with command chmod +x trans.sh and then run the script using ./trans.sh then run python setup.py genpot. Pushed this as commit 703a0a4 It is important for me to know what could have possibly been a mistake as I plan to add translations to other projects too.

[quozl]

Thanks. It's easy to change the title of the pull request, if that's the only doubt you have. Otherwise what is the procedural problem that would be solved by using a different pull request? Is it that you don't want to try fixing the collaboration? If so, I'm fine with that, and we can leave the pull request unfinished or merge it. We just need to know what you plan. I'll see if I can find time to test. I'm not familiar with the activity; do you have or know of a test plan?

Regarding translations, I've no idea why your results were different to mine, but I agree it is important to find out why. Your commit 703a0a4 when viewed shows only removal of some po/*.po files, and addition of trans.sh file; no changes to po/ach.po. Yet my commit 72e9f4f shows changes to po/ach.po and a list of other files. My guess is that you didn't complete a git add before git commit. You might test that theory by checking your bash history, any diary you may have, or by making a new branch from a commit before 72e9f4f and repeating the script and git add, git commit, and then comparing the commits with git diff.

[chimosky]

I did use the same script you used to update translations. Let me elaborate:
Saved the script as trans.sh, made the script executable with command chmod +x trans.sh and then run the script using ./trans.sh then run python setup.py genpot. Pushed this as commit 703a0a4 It is important for me to know what could have possibly been a mistake as I plan to add translations to other projects too.

The script requires that you pass an argument to it and the argument is the name of the project as listed on translate.sugarlabs.org and running the script without an argument would run the script but only remove empty translations without pulling in any translations.

[JuiP]

Otherwise what is the procedural problem that would be solved by using a different pull request? Is it that you don't want to try fixing the collaboration?

Initially, I didnot plan to fix the collaboration due to tight schedules. However, I have made some progress in fixing it, so I will add more commits if I get it working after testing, might take me some time; will finish it as soon as I can.

I'll see if I can find time to test. I'm not familiar with the activity; do you have or know of a test plan?

I started with playing a game and noted-

• New game can be started
• Robot mode works, with correct changes in the playbutton displaying if it is my turn
• Stop button works as expected
• Using the activity from both the journal and home view restores the initial game state
• Score is calculated after completing a path.
• When out of moves, the game is over.
• Collaboration- disables the robot mode for the sharer (with changes in tool-tips)
• Collaboration- disables the new game and robot mode for the joiner.
• Collaboration- seen as a collaboration instance in the journal with the icon of the joiner
• Collaboration does not share the same game board and the deck of tiles. Changes in one game board are not reflected in the other.
  The above actions have no errors in the logs for me.

[JuiP]

The script requires that you pass an argument to it and the argument is the name of the project as listed on translate.sugarlabs.org and running the script without an argument would run the script but only remove empty translations without pulling in any translations.

@chimosky That's right, it was such a stupid mistake! I seemed to have overlooked that! Thanks a lot :)

[quozl]

Thanks! Added new issue sugarlabs/sugar-tools#3.

[JuiP]

@quozl @chimosky Update on this PR:
I tested on two virtual machines, collaboration works exactly as expected.

Otherwise what is the procedural problem that would be solved by using a different pull request?

I just wanted to avoid it in this PR, because the work is a little messy. I am not sure if reviewing code commit wise will be of any help. Maybe it would have been easier to review directly as files changed, had it been done in another PR.

Many changes which have been done commit 38e8b8b onwards may be kept or undone in further commits. I found making changes and simultaneously testing them, more better to work with as a result there are multiple commits. (And also I made the mistake of merging a branch in the branch, might make reviewing code more difficult.... sorry for too many commits and the mess)

Please test the changes made till 9e5e3fe so I can go ahead with

• Using logging.debug instead of print
• flake8 fixes
• and finally a refreshed pot file with fetching translations

[quozl]

What you describe is a common hesitation, and I'm happy to explain the process further. I'll quote selectively.

First, remember that a pull request comes from a branch, and anything you do to the branch will be reflected in the pull request.

A branch is made up of a series of commits.

The commits describe your changes, but are independent of the changes.

Otherwise what is the procedural problem that would be solved by using a different pull request?

I just wanted to avoid it in this PR, because the work is a little messy. I am not sure if reviewing code commit wise will be of any help. Maybe it would have been easier to review directly as files changed, had it been done in another PR.

Second, remember that Files changed in the pull request is the equivalent of git diff master, assuming you haven't moved master to point to another commit since you began the branch. Otherwise it is the aggregate change since the branch began.

Many changes which have been done (in) commit 38e8b8b onwards may be kept or undone in further commits. I found making changes and simultaneously testing them, more better to work with as a result there are multiple commits. (And also I made the mistake of merging a branch in the branch, might make reviewing code more difficult.... sorry for too many commits and the mess)

Third, making changes and immediately testing them is the best way to work, because (a) you get to reflect on the change during the commit message, (b) you can use git to transport the change to a test system, (c) we get to see your thought process and trials (increased trust), and (d) you can easily revert a failed trial change using git revert or git reset.

What you may be missing is practice in rewriting commits. This where you don't change the outcome; the files, but you do change the way that git reaches that outcome.

I can't be specific about the tools you should use to rewrite commits, because I don't know what tools you are using to write them in the first place. But the tools do eventually call git commands, and these commands are what do the work.

The tools that I use are emacs with magit and git. With reference to this Git Cheatsheat ... here's my workflow for writing a commit;

• a source file is edited with emacs, and saved into the workspace,
• magit is invoked and the change relative to index is reviewed in diff format,
• the whole change, or tiny parts of it are added to the index using magit keystrokes,
• magit is commanded to make a commit from the index.

Most people just use git add, or at worst git add -a. What I'm doing above is similar to git add --interactive.

For rewriting commits, I've several different workflows, and I choose one based on the nature of the historical error that I wish to make.

• git log -1 helps me remember the state of the branch in case I need to go back, (also available with git reflog if I lose that from scrollback),
• git rebase -i if I want to change the order of commits, squash some commits into other commits, or rewrite their messages (like git commit --amend for very old commits),
• git reset if I want to get my hands dirty and create a new series of commits from the changes.

After the commit history is tidied up, what I do next depends on whether I've had review comments that address specific commits. For the most part, I git push --force, and leave it to GitHub to show the new history. But when there has been lengthy discussion on specific commits, I'll stow the rewritten history in a new branch, restore my pull request branch to what it was, revert everything in one commit, and then merge in the new branch. So the pull request has;

• old commits, some of which aren't to be kept,
• a revert of all the old commits,
• a rewrite using new commits.

Without any change to Files changed.

Looking at your branch at present, if it were mine, I would try to rewrite and reorder commits to follow this sequence;

• Port to Python 3,
• Update CollabWrapper,
• Fix PyGIWarnings,
• Port telepathy static binding to TelepathyGLib,
• Port from GObject to GLib,
• Fix collaboration,
• Fix format specifiers, genpot, and translations,
• Anything else.

Remember you can't easily lose anything if you remember the git hash somewhere.

Hope that helps.

[quozl]

Tested 9e5e3fe.

• the toolbar suffers vertical expansion; caused by the label before the help button; the label should be allowed to be wider,
• the canvas has a grey area which covers a tile and the top border of the playing board,
• the tiles can only be moved or rotated by clicking the top left corner of the tiles; not sure if that is intended,

I've not tested collaboration fully. I need to win a game first. 😁 I've never seen this activity before.

[quozl]

Quick check of logs; found this but not sure how to cause;

Traceback (most recent call last):
  File "/usr/share/sugar/activities/Paths.activity/PathsActivity.py", line 306, in _message_cb
    self._processing_methods[command][0](payload)
  File "/usr/share/sugar/activities/Paths.activity/PathsActivity.py", line 370, in _play_a_piece
    if self._game.hands[self._game.whos_turn].hand[i] is not None \
IndexError: list index out of range

[JuiP]

• the toolbar suffers vertical expansion; caused by the label before the help button; the label should be allowed to be wider,

will get this fixed

• the canvas has a grey area which covers a tile and the top border of the playing board,

Issue #7 , will try fixing it!

• the tiles can only be moved or rotated by clicking the top left corner of the tiles; not sure if that is intended,

I can't reproduce this. For me the tiles are rotating by clicking on them multiple times and not necessarily only in the top left corner.

I need to win a game first. 😁 I've never seen this activity before.

Oh you just need to maximize your score by making a closed path and by using as many tiles as possible for the closed path. I actually kind of found this activity fun ! 😀

Quick check of logs; found this but not sure how to cause;

Regarding this error, I didn't get this error initially. Can you confirm if you have tried to restore a shared activity and continued to play to find this error? I'm not sure if there is any other way to reproduce this.
As far as what I noticed: if I attempt to rejoin a shared activity whose sharing was not initiated by me, then the activity restores but now I become the initiator. So now it is my turn (the initiator plays first), after each player plays one chance, the activity is stuck and returns this error. To fix this maybe I have to ensure that I'm not considered an initiator for this activity after restoring. Is this how you got that error?

[chimosky]

Tested 9e5e3fe.

• When play button is clicked while player hasn't played, the toolbar expands and I think it's as a result of the length of this text that shows beside the button There are errors -it is still your turn.

Tested collaboration, sharer joins but game state is yet to be shared.

[JuiP]

Tested collaboration, sharer joins but game state is yet to be shared.

Game state is not shared unless the initiator starts a new game.

[chimosky]

Tested collaboration, sharer joins but game state is yet to be shared.

Game state is not shared unless the initiator starts a new game.

Game state still isn't shared, also noticed that for the joiner the new game button tooltip says they can't start a game but the button still works.

[JuiP]

Collaboration still isn't shared

Changes in one game are reflected in the other and they have shared game boards.... Have I misunderstood by what you meant by collaboration is'nt shared?

noticed that for the joiner the new game button tooltip says they can't start a game but the button still works.

Oh I know this problem still exists, I had tried self._new_game_button.set_sensitive(False) after line 280 in PathsActivity.py as a result when the sharer started a new game, the joiner could not refresh and share the gameboard as the sharer; it couldnot connect to the event new_game() .... still working on it

[chimosky]

Changes in one game are reflected in the other and they have shared game boards.... Have I misunderstood by what you meant by collaboration is'nt shared?

No you didn't misunderstand, I've edited my comment.

@quozl kindly test collaboration as game state isn't shared for me.

[srevinsaju]

Hey, I am back reviewing.

Quick file diff review and testing.

• Tiles do not fit screen resolution
  

Notice that the top tile is hidden under the toolbar

• Pressing the help button changes the orientation of the first tile in order ( I guess that's done on mistake)
• Add help to the help button. Otherwise, the help button might not be working after / before the port
• Change the tooltip of "Play with Robot" alternatively. i.e A student might not understand if a robot with open eyes means the Robot is playing, but a robot with closed eyes means a robot is not playing. Most of the time, a person new to the activity is most likely to hover over the button and see "Play with the robot" as the default tooltip. When the robot is enabled, the tooltip should change to "Disable Robot", etc.
• Why is the XO icon on the PathsActivity? I am curious

Tested on Arch Linux, Sugar 0.116

[quozl]

Thanks. Reviewed. In addition to comments, I've already made ...

• don't include the CollabWrapper test activity, it isn't needed in this activity,
• don't include the CollabWrapper text editor, it isn't needed here either,
• the accelerator for the StopButton is not needed, as the toolkit does it,
• there are two different joined signals, from different software layers, but you've linked them to the same callback; see the test activity for how they are different,
• there's a use of incorrect GTK signal name; key_press_event is now key-press-event, and underscores aren't the convention we've used elsewhere,

Tested in single-user mode.

• clicking on black parts of a card is ignored,

[srevinsaju]

@quozl is the XO icon kept on purpose in the paths activity?

[quozl]

@srevinsaju, it is used to show whose turn it is when collaborating. Colour and tooltip text changes. Have you got collaboration working yet on your setup?

@JuiP, something is wrong with collaboration, but I'm not sure what. Grid is shared. Card hands are unique, as they ought to be. When I play cards to the grid, sometimes the turn is not ended, sometimes it is. It can get into a state where nobody can play their cards. I'm not sure what the play and stop button is for in the toolbar. At one stage, after winning a path, pressing the play bar incremented the score repeatedly.

[quozl]

By the way, I've updated the checkboxes in previous comments here.

[chimosky]

You can remove this line as you connected the joined signal in collabwrapper to the same callback.

[quozl]

Signals mean different things, even though they are both called "joined". It is best to use the signals and API at the same level of the stack.

[chimosky]

@quozl could you clarify what you meant by "It's best to use the signals and API at the same level of the stack" as they're both different signals and both get emitted differently.

My statement above was because as CollabWrapper is used for collaboration, we should just call the joined event of collabwrapper and not the one from the sugar3.activity.activity.Activity class.

[quozl]

@quozl could you clarify what you meant by "It's best to use the signals and API at the same level of the stack" as they're both different signals and both get emitted differently.

CollabWrapper is a software layer, or stack level, above the Telepathy API exposed by the sugar3 classes. So whenever someone mixes signals and API between Telepathy API and CollabWrapper, there's an opportunity to bypass a layer, which makes code non-modular and confusing.

My statement above was because as CollabWrapper is used for collaboration, we should just call the joined event of collabwrapper and not the one from the sugar3.activity.activity.Activity class.

Yes, that's sort of the same thing, except I gave reasons? 😁