Update metadata | Add Screenshots

sugarlabs / browse-activity

Sugar activity to browse the internet; WebKit on GTK on Sugar Toolkit

GNU General Public License v2.0

8 stars 44 forks source link

Update metadata | Add Screenshots #82

Closed vipulgupta2048 closed 6 years ago

vipulgupta2048 commented 6 years ago

Updated following parameters url, license, tags

Corrected Address in LGPL and GPLv2 licenses. Added screenshots.

quozl commented 6 years ago

Thanks. Reviewed to 27d2e1e. Please consider;

[ ] make the collabwrapper changes first in https://github.com/sugarlabs/collabwrapper
[ ] add COPYING files for the LGPLv2 and GPLv3 licenses.

vipulgupta2048 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

Made a PR - link - https://github.com/sugarlabs/collabwrapper/pull/6

quozl commented 6 years ago

Merged https://github.com/sugarlabs/collabwrapper/pull/6

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

vipulgupta2048 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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.

quozl commented 6 years ago

Looking deeper;

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

vipulgupta2048 commented 6 years ago

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

quozl commented 6 years ago

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