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

Update metadata | Add Screenshots #82

Merged
merged 6 commits into from
Jul 21, 2018
Merged

Conversation

vipulgupta2048
Copy link
Contributor

Updated following parameters
url, license, tags

Corrected Address in LGPL and GPLv2 licenses.
Added screenshots.

@quozl
Copy link
Contributor

quozl commented Jul 12, 2018

Thanks. Reviewed to 27d2e1e. Please consider;

@vipulgupta2048
Copy link
Contributor Author

@quozl

  • Added the COPYING files
  • I didn't understand what changes were to be made in the collabwrapper.
  • Also, I didn't understand what the collabwrapper is for? I read the docs then, but what is the collaboration system for Sugar. Looking interesting for sure. Is it the thing that makes participating in other activities possible?

@quozl
Copy link
Contributor

quozl commented Jul 15, 2018

Added the COPYING files

Thanks.

didn't understand what changes were to be made in the collabwrapper.

You made changes to a collabwrapper file; so you ought to be able to explain what changes you made. Make these changes first in the other repository.

didn't understand what the collabwrapper is for

As far as this activity is concerned, it is a Python module. See the other repository for what collabwrapper is.

@vipulgupta2048
Copy link
Contributor Author

Made a PR - link - sugarlabs/collabwrapper#6

@quozl
Copy link
Contributor

quozl commented Jul 17, 2018

Merged
sugarlabs/collabwrapper#6

However, your changes here to collabwrapper are different, please resolve.

@vipulgupta2048
Copy link
Contributor Author

@quozl The difference in changes are just of whitespace. I can fix that as well but it's not that critical. Should I?
I think this PR is ready to go, at least.

@quozl
Copy link
Contributor

quozl commented Jul 20, 2018

Yes, I know they are whitespace changes, but you did make them here and did not make them there, so the files won't match, and there's a risk of future patches to collabwrapper not being applied easily here. You can either remove the whitespace changes here, or make the whitespace changes there.

@vipulgupta2048
Copy link
Contributor Author

@quozl That's what I am trying to reason here, there are no problems left with Collabwrapper/texteditor.py, I checked it again with my editor plugins and with both flake8 and pylint that I used. No whitespace errors found. (Your code has been rated at 8.19/10)
My editor might have automatically removed the whitespace in browse/Collabwrapper/texteditor.py which is modified a little. Here, time package is being imported but the original script in collabwrapper repo has no reference to that. So it's safe to say that we are good. I think.

Signed-off-by: James Cameron <quozl@laptop.org>
Signed-off-by: Vipul Gupta (@vipulgupta2048) <vipulgupta2048@gmail.com>
Signed-off-by: Vipul Gupta (@vipulgupta2048) <vipulgupta2048@gmail.com>
Signed-off-by: Vipul Gupta (@vipulgupta2048) <vipulgupta2048@gmail.com>
An activity will often maintain a list of buddies.

For a leader, it is easy to maintain the list, after sharing the
activity, by receiving the buddy_joined and buddy_left signals.

For a non-leader, it was not easy to maintain the list, after joining a
shared activity, without calling underneath CollabWrapper into sugar3.

When an activity has been joined, iterate through the buddies and emit
buddy_joined for each.

Also rewrite documentation accordingly, and simplify.

Taken from collabwrapper:84be1509d7e289829e7e542c6bd33b3997e9036
@quozl quozl merged commit 7cfd9f2 into sugarlabs:master Jul 21, 2018
@quozl
Copy link
Contributor

quozl commented Jul 21, 2018

Looking deeper;

  • texteditor.py wasn't necessary,
  • collabwrapper.py was out of date,
  • placement in a subdirectory made applying collabwrapper patches less automatic.

@vipulgupta2048
Copy link
Contributor Author

@quozl It's very odd, indeed. If the activity has a maintainer, maybe he/she can work towards improving it.

@quozl
Copy link
Contributor

quozl commented Jul 23, 2018

I did, pushed to your branch, then merged. You're welcome to review.

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.

None yet

2 participants