search

sugarlabs / sugar-toolkit-gtk3

Sugar Learning Environment, Activity Toolkit, GTK 3.
GNU Lesser General Public License v2.1
21 stars 80 forks source link

Update toolbutton.py #345

Closed Hrishi1999 closed 7 years ago

quozl commented 7 years ago

Reviewed.

Further comments will be in the commits.

quozl commented 7 years ago

Thanks for your patches.

Please also include an example program for ToolComboBox. (a) so that we can see you know how to use the API you are documenting, and (b) so our example programs cover the class; currently there is no example.

See examples/combobox.py and examples/toolbuttons.py for inspiration.

Hrishi1999 commented 7 years ago

Thanks for the quick review. I will try my best to do all of it.

Hrishi1999 commented 7 years ago

Oh, the toolcombobox commit was from Jan 17, and currently, I was doing only for toolbutton.py. I will add an example later as you said. Thanks.

quozl commented 7 years ago

Oh, so you committed old changes. Thanks for letting us know. You should work on a branch instead.

Hrishi1999 commented 7 years ago

Thanks :D

Hrishi1999 commented 7 years ago

Updated.

Hrishi1999 commented 7 years ago

Thanks for reviewing. I am not working on toolcombobox.py as I said earlier. It's changes were from January. I will update it also soon though. Now, let's only see for toolbutton.py. Thanks :D

Hrishi1999 commented 7 years ago

But again, as I said, I would try to do all of it.

quozl commented 7 years ago

@Hrishi1999, big problem is you are working on your master branch of your sugar-toolkit-gtk3 repository which was cloned last year and has not been updated since, so it is going to cost you some time to fix.

See for instance Hrishi1999/sugar-toolkit-gtk3 which says "This branch is 5 commits ahead, 119 commits behind sugarlabs:master."

So please rebase against our master, collapse your patches, and force push. As a result the above link will say "1 commits ahead" and not behind, and this pull request will have one commit instead of several.

I expect you'll have questions about how to do that. Please ask.

Hrishi1999 commented 7 years ago

How can I do that?

Hrishi1999 commented 7 years ago

Should I select the other option ("Create a new branch for this commit and start a pull request.")?

quozl commented 7 years ago

@Hrishi1999, you still have a master branch which is way behind and you can't use it like that ... we can see you have a clone of sugar-toolkit-gtk3 on GitHub, but you will also need;

After the completing the above, you can easily rebase against our master by merging, something like this;

git checkout master  # your master branch
git remote add sugarlabs https://github.com/sugarlabs/sugar-toolkit-gtk3.git
git pull --rebase sugarlabs master  # merge changes from sugarlabs

Then force push your branch to your GitHub repository;

git push --force

Then check how far ahead or behind GitHub says you are, and recheck this pull request.

If you don't want your old changes to be included, then use git rebase -i to remove them and force push again.

Hrishi1999 commented 7 years ago

So, I have to gone Hrishi1999 one or sugarlabs one? Sorry for asking all this but I know and don't knows certain things :D Also, how can I use "git push --force", it needs a name. Edit : Followed

Hrishi1999 commented 7 years ago

Done, now what should I do?

quozl commented 7 years ago

Good, your master branch is now ahead of ours by six commits, of which three are to toolcombobox.py and three to toolbutton.py.

You said earlier you didn't want to propose the changes to toolcombobox.py, so please use git rebase -i to remove them.

Also, as a second step, use git rebase -i on the remaining three changes to toolbutton.py, but this time use the squash feature to collapse the three commits into one commit.

Then force push again.

See the git rebase --help for how it works in detail.

quozl commented 7 years ago

Using Git rebase on the command line may also be helpful in learning.

Hrishi1999 commented 7 years ago

"It seems that there is already a rebase-merge directory, and I wonder if you are in the middle of another rebase. If that is the case, please try git rebase (--continue | --abort | --skip) If that is not the case, please rm -fr "C:/Users/Hrishi/Documents/GitHub/sugar-toolkit-gtk3/.git/rebase-merge" and run me again. I am stopping in case you still have something valuable there." This happens after I use the command.

quozl commented 7 years ago

Check the date and time on the .git/rebase-merge file and if it is long ago, use git rebase --abort. If it is not long ago, you've just got confused as to where you are up to in using rebase feature.

Hrishi1999 commented 7 years ago

I think I have done it as you said now.

quozl commented 7 years ago

It doesn't look done; your master branch on GitHub is still as it was. Check my instructions again step by step.

Hrishi1999 commented 7 years ago

Now?

quozl commented 7 years ago

Yes, well done. Now your master branch, which is the subject of this pull request, is exactly one commit in advance of ours. That helps us. Next step is for us to review the commit again.

quozl commented 7 years ago

@Hrishi1999, please look at the comments on the commit in context;

https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/345/commits/e04e608ac4f310ad9e86d0f8656450cf13d15d94

There are still things that @samdroid-apps had asked to be done that you have not done.

Edit the file, make all changes needed, then git commit --amend src/sugar3/graphics/toolbutton.py, then git push --force.

Hrishi1999 commented 7 years ago

I have added example.

quozl commented 7 years ago

@Hrishi1999, please collapse your three commits. Method is git rebase -i followed by git push --force.

Also, your example does not work. Please make it work. The example is clearly unfinished; it does not use the UI objects, and there is an undefined variable. Examples must work; they must not only use the API you are documenting, they must show it on screen.

Also, you have still not done the things that @samdroid-apps had asked to be done.

As you have missed these things several times, my guess is that you are not going back to read the previous comments in the pull request, so I'll put it in the form of task lists;

Documentation Editing

Example Development

Update Pull Request

Please don't feel insulted by this task list; I know I would, but I recognise that some people just can't cope with extreme complexity and detail; they don't fly aeroplanes until they can. It is a skill to develop.

Hrishi1999 commented 7 years ago

Sorry, I accidentally committed this :(

Hrishi1999 commented 7 years ago

Seems alright now :D ?

quozl commented 7 years ago

Something went wrong; while I saw your intervening commits which fixed the word event, the commits now do not fix it.

Hrishi1999 commented 7 years ago

OHOH, What happened. This latest commit is showing the older one why?

quozl commented 7 years ago

Perhaps you accidentally deleted commits instead of squash them during rebase.

Hrishi1999 commented 7 years ago

Now what? Do "git commit --amend src/sugar3/graphics/toolbutton.py" and force push?

quozl commented 7 years ago

I'll have to get back to you later, I've an appointment. You may wish to do your own checks by looking at the pull request "Files" tab and checking each point in the comments.

Hrishi1999 commented 7 years ago

Thanks :D

Hrishi1999 commented 7 years ago

See, this 7c6dcaa commit, before it I tried to force push, I goes back to normal :( Help

quozl commented 7 years ago

I'd guess git is doing exactly what you tell it to do, and perhaps you aren't checking what it does; use gitk or another visualisation tool to check the result of your git actions.

Can you add a screenshot of the example? I can't see how it could work.

Hrishi1999 commented 7 years ago

How can I test my example? PyGtk? Help me.

quozl commented 7 years ago

So many options; here's a few;

Execution by Emulation

Execution without Emulation with Packaging

Execution without Emulation without Packaging

Only you can decide which will be the most appropriate. You have the agency and the responsibility.

Hrishi1999 commented 7 years ago

I will open a new pr with final changes. Today is last day of task.

Hrishi1999 commented 7 years ago

Hey, I am getting an error, in both toolbar.py (line 12) and toolbutton.py (Theme parsing error...missing name of pseudo-class). What is that?

Hrishi1999 commented 7 years ago

This works! I am opening a new PR because I have very less time :D

Hrishi1999 commented 7 years ago

https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/347