sugarlabs / showntell-activity

ShowNTell expands the capabilities of ClassroomPresenter to enable users to create presentations on the XO and to record audio descriptions of the slides.
GNU General Public License v2.0
0 stars 6 forks source link

Combine different Activity paths. #11

Closed chimosky closed 5 years ago

chimosky commented 6 years ago

@quozl, kindly review.

quozl commented 6 years ago

Please explain further. I don't understand why you are asking to merge your restore branch into the restore branch here.

chimosky commented 6 years ago

Where should i ask to merge it to?.

quozl commented 6 years ago

Remind me, what is it you are doing?

chimosky commented 6 years ago

In #8 i said;

So v12 will contain everything, including commit history of v2 to v11?.

you said;

Thank you, that would be ideal.

You also said.

I'm saying the source code here is like a wandering child that has stepped off a path, and rather than encourage them to wander further, it would be better to bring them back to the path.

And that's what I'm doing.

quozl commented 6 years ago

Okay, thanks. Please mention next time, and link to other resources as necessary.

What you have done is taken the commits from master after v2, tried to resolve conflicts, and applied them to the restore branch.

You haven't tested?

You haven't run flake8 to find problems?

What I did; set the commits side by side and compared the differences;

https://github.com/sugarlabs/showntell-activity/commit/4a404b1dc30f8f2921d3f0cb3ea7bd41d42b3019 was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/38c3e4ab52f407bcf41509eeb009486acdbc067f

https://github.com/sugarlabs/showntell-activity/commit/ae5568a88b7c5c8e8fcdce9654b300c21d016d63 very different to https://github.com/sugarlabs/showntell-activity/pull/11/commits/6736812510b7c4893392e291acd1610010e2e813

https://github.com/sugarlabs/showntell-activity/commit/1a4bf39c8081ab5732f8277a62ec8fc4473f51e4 has no equivalent

https://github.com/sugarlabs/showntell-activity/commit/dab4d050768882713d612a4683b7094105d5dda7 was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/4e3068b270e316ad506596bb9e10e1071b4500ad

https://github.com/sugarlabs/showntell-activity/commit/2944be81ad3d49805b89d68a3e57e4a498809a32 was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/7f9664595476e4759fcaeb67e3a6832a1885d297

https://github.com/sugarlabs/showntell-activity/commit/6858ee77f83cb3943767b6e5316535186e5dad50 very different to https://github.com/sugarlabs/showntell-activity/pull/11/commits/9082607f2078688c02c8eebadc9eb4e0dc75ab9a

https://github.com/sugarlabs/showntell-activity/commit/9dc1b941c5d331c30f26b35feb5a0476e5fec808 somewhat different to https://github.com/sugarlabs/showntell-activity/pull/11/commits/b9a5fdaa4e649788466b98f45dacc0ec80f49b36

https://github.com/sugarlabs/showntell-activity/commit/00daf740f45e7e408f53fef155272525139a550b was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/284e9eeab0cc67c81c8af218570c0e4bab276ab0

https://github.com/sugarlabs/showntell-activity/commit/3ea47fba4b32c20507455e7142ab511d45d5c7c5 was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/19a982c864a975fc171050e9ee2a42d949625598

https://github.com/sugarlabs/showntell-activity/commit/3ceb04f64ac54ef1246bdbbacabb8f05fc80e679 was cherry picked with several differences as https://github.com/sugarlabs/showntell-activity/pull/11/commits/970b42269e294bb6b05097ec24a4b7402d96f13e (this is where self.Instructor_Remove_Ink(uid, idx) was removed, but instead of fixing this commit you added a new one https://github.com/sugarlabs/showntell-activity/pull/11/commits/09d0a5fff3b524bdee0d0a71d373e7ca093fb93e ... and there are other instances like this, as if you didn't resolve conflicts in 3ceb04f by running flake8 each time).

https://github.com/sugarlabs/showntell-activity/commit/436fce487b10972060e811c2036ebaf2c853ac9f was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/374ec0fc7ad4df6d0db751948e8d48dbdef8dc65

https://github.com/sugarlabs/showntell-activity/commit/a7b815bc4d527aecdcc6d70dcaab5c263999b88d was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/bed708af11c653997bde5467b774809e35058d37

https://github.com/sugarlabs/showntell-activity/commit/db62130bc9e95ad230d6d6f9459ff916a4e0eea0 was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/f01622543bf3b89415213aee43beca70a51d6ae3

https://github.com/sugarlabs/showntell-activity/commit/c32fb04a6eee1acbfe1b7de15ba5370cc0acaa0a was cherry picked as https://github.com/sugarlabs/showntell-activity/pull/11/commits/5205e1fb099b4cd6b6e2cd802fbc47c8faa0d005

https://github.com/sugarlabs/showntell-activity/pull/11/commits/09d0a5fff3b524bdee0d0a71d373e7ca093fb93e is new

Perhaps you did not set the old commits side by side with your new commits as you made the changes.

What I did; git diff master, expecting to see the changes from v2 GTK+ 3 to a possible future v12 GTK+3;

At this point I stopped because I had run out of time, and the work doesn't look finished. Please continue, and test.

chimosky commented 6 years ago

Restore branch stops at 8a8b41a, so any changes I've made was committed after that. I'll continue and test. Thanks.

chimosky commented 6 years ago

@quozl, kindly review. Added e8805d2.

quozl commented 6 years ago

Is tested?

chimosky commented 6 years ago

Yes.

chimosky commented 6 years ago

@quozl how would you like to proceed?.

quozl commented 6 years ago

What I did; git diff v2..v11, to get an idea of what changes Tony made for v6 and v11, of which the most important are;

That will do for a start.

What I did; git diff master..e8805d2, expecting to see the above changes from v2 GTK+ 2 to v6 and v11 ported to GTK+ 3 for a possible future v12 GTK+3; but they weren't there.

Looking just at change object destroy in cpxoview.py; you reversed this change in your https://github.com/sugarlabs/showntell-activity/pull/11/commits/e83adaf4ae65934fa788362124adf839652ea914#diff-dc695fc99dbef2308fc6b229800dc291R101

Why did you do that?

Perhaps when you cherry-picked and applied commits from the master branch to your restore branch you resolved conflicts without looking at the diffs in the commits.

chimosky commented 6 years ago

What I did; git diff master..e8805d2, expecting to see the above changes from v2 GTK+ 2 to v6 and v11 ported to GTK+ 3 for a possible future v12 GTK+3; but they weren't there.

The changes from v2 GTK+2 to v6 and v11 ported to GTK+3 are there; my changes start from 8a8b41a meaning the changes you're looking for are before the ones I've made - and I can see them -

Looking just at change object destroy in cpxoview.py; you reversed this change in your e83adaf#diff-dc695fc99dbef2308fc6b229800dc291R101

Why did you do that?

That was how @cristian99garcia change was - exactly what he did -, maybe that was a mistake on my part as I've stated below - see ae5568a -

Perhaps when you cherry-picked and applied commits from the master branch to your restore branch you resolved conflicts without looking at the diffs in the commits.

I did, it's just that I didn't look critically at changes from v2 to v11 to make sure same are applied in restore where necessary.

chimosky commented 6 years ago

@quozl how would you like to proceed?

quozl commented 6 years ago

Could you begin with v11, then port the work by @cristian99garcia into it, looking critically at each change as you go?

chimosky commented 6 years ago

What I did; git diff v2..v11, to get an idea of what changes Tony made for v6 and v11, of which the most important are; change of version number to v11, change icon, change object destroy in cpxoview.py, move a create_columns call into an exception handler in listview.py, reorder toolbar numbering in showntell.py, ...

I just noticed that my branch doesn't have any tags so the above is expected, my changes are after v11, so I'll check for any changes that were removed from v11 and before to see if it was added again.

quozl commented 6 years ago

I don't understand, sorry. Branches don't hold tags, repositories do. Tags are links to a specific git commit hash. You can check if your clone of this repository has tags using various commands; e.g.

chimosky commented 6 years ago

I'm saying git diff v2..v11 won't work because there are no tags in mine, since changes start from v11.

quozl commented 6 years ago

Again, I don't quite understand what you mean. The tags are present in this repository. This is shown by releases tab. Perhaps you don't know how to fetch tags? Look at git fetch and git pull commands, or make a new git clone in another directory.

chimosky commented 6 years ago

@quozl can you run git diff v2..v11 from my branch now and check if the requested diff is shown.

quozl commented 6 years ago

Yes, it does; your repository has both tags at equal git commit hashes. The git diff v2..v11 continues to show changes made by Tony that were omitted by @cristian99garcia before porting to GTK+ 3. The changes look to be important and as a result of feedback or testing following v2 release.

chimosky commented 6 years ago

@quozl the changes shown are as you made here.

quozl commented 6 years ago

So nothing's changed since I last reviewed this on 4th July;

What I did; git diff v2..v11, to get an idea of what changes Tony made for v6 and v11, of which the most important are;

  • change of version number to v11,

You've still somehow lost Tony's changes. Why? Your proposed branch has metadata activity_version = 2.

  • change icon,

You've still somehow lost Tony's changes. Why? Here's the v11 icon, and your proposed branch has the old icon.

You haven't merged the two histories; instead you've apparently just destroyed one history.

I think you need to start over.

chimosky commented 6 years ago

I think you need to start over.

I'll do that.

chimosky commented 6 years ago

@quozl kindly review.

quozl commented 6 years ago

Thanks. Reviewed as at 3da3c2d. Method; side by side comparison of three diffs;

You don't appear to have completed merging the two histories. It looks like you haven't understood the changes made by Tony, and haven't ensured they remain in your branch.

I don't think I need to enumerate each of the differences, as they are obvious from the above git diff commands, but the first one I hit this time was that Tony removed the prints in class Cpxoview, but you put them back. Why did you put them back? How did that happen? Looking at https://github.com/chimosky/showntell-activity/commit/a3b71a3c74fd40f0742ec5fb7a972f33899ed4ac you said that @cristian99garcia did it, but they didn't; the print was already there because it came from v2.

It seems you are attempting to use git to mechanically merge the history without preserving the source code changes. Instead, you have to look at each old commit diff and apply the changes by hand until your new commit diff looks about the same.

Hope that helps.

chimosky commented 6 years ago

Thanks.

rhl-bthr commented 5 years ago

Stale, closing