After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 700462 - [PATCH] Improvements about the commit history graph
[PATCH] Improvements about the commit history graph
Status: RESOLVED FIXED
Product: gitg
Classification: Applications
Component: gitg
git master
Other All
: Normal enhancement
: ---
Assigned To: gitg-maint
gitg-maint
Depends on:
Blocks:
 
 
Reported: 2013-05-16 14:22 UTC by Techlive Zheng
Modified: 2013-05-17 13:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Repect the mainline of merges (2.18 KB, patch)
2013-05-16 14:24 UTC, Techlive Zheng
needs-work Details | Review
Use the same color for every long-runing lane (1.11 KB, patch)
2013-05-16 14:24 UTC, Techlive Zheng
none Details | Review
Repect the mainline of the merges (3.63 KB, patch)
2013-05-16 18:11 UTC, Techlive Zheng
none Details | Review
Use the same color for every long-runing lane (1.31 KB, patch)
2013-05-16 18:12 UTC, Techlive Zheng
none Details | Review
Repect the mainline of the merges(fixed tab indentation) (3.62 KB, patch)
2013-05-16 18:18 UTC, Techlive Zheng
none Details | Review
Use the same color for every long-runing lane(fixed indentation) (1.31 KB, patch)
2013-05-16 18:19 UTC, Techlive Zheng
none Details | Review
Coding style fix (3.63 KB, patch)
2013-05-16 18:22 UTC, Techlive Zheng
none Details | Review
Repect the mainline of the merges(coding stayle fix) (3.63 KB, patch)
2013-05-16 18:23 UTC, Techlive Zheng
none Details | Review
0001 - Repect the mainline of the merges(coding stayle fix) (3.63 KB, patch)
2013-05-16 18:24 UTC, Techlive Zheng
none Details | Review
0001 - Repect the mainline of the merges(coding style fix) (3.63 KB, patch)
2013-05-16 18:25 UTC, Techlive Zheng
none Details | Review
0002 - Use the same color for every long-runing lane(coding style fix) (1.35 KB, patch)
2013-05-16 18:26 UTC, Techlive Zheng
none Details | Review
0001 - Repect the mainline of the merges (typo fix) (3.63 KB, patch)
2013-05-16 18:33 UTC, Techlive Zheng
none Details | Review
0001 - Respect the mainline of the merges (typo fix) (3.63 KB, patch)
2013-05-16 18:34 UTC, Techlive Zheng
needs-work Details | Review
0002 - Use the same color for every long-runing lane (typo fix) (1.35 KB, patch)
2013-05-16 18:34 UTC, Techlive Zheng
none Details | Review
Screenshots before the patch (26.72 KB, image/png)
2013-05-17 07:50 UTC, Techlive Zheng
  Details
Screenshots after the patch (26.41 KB, image/png)
2013-05-17 07:52 UTC, Techlive Zheng
  Details
0001 - Respect the mainline of the merges (Revised version) (1.86 KB, patch)
2013-05-17 11:10 UTC, Techlive Zheng
none Details | Review
0002 - Use the same color for every long-runing lane (Revised version) (1.35 KB, patch)
2013-05-17 11:11 UTC, Techlive Zheng
none Details | Review
0001 - Respect the mainline of the merges (fixed commit message) (1.86 KB, patch)
2013-05-17 11:18 UTC, Techlive Zheng
committed Details | Review
0002 - Use the same color for every long-runing lane (fixed commit message) (1.35 KB, patch)
2013-05-17 11:19 UTC, Techlive Zheng
committed Details | Review

Description Techlive Zheng 2013-05-16 14:22:10 UTC
The first feature request of [the gitg's wishlist][1] is "Honour commit parent order",
this series of patches has fulfilled that wish."

Now, it should be much easier to see how the development goes on from the
history graph.

[1]: https://live.gnome.org/Gitg/Wishlist


  * Repect the mainline of merges
  * Use the same color for every long-runing lane
Comment 1 Techlive Zheng 2013-05-16 14:24:00 UTC
Created attachment 244410 [details] [review]
Repect the mainline of merges
Comment 2 Techlive Zheng 2013-05-16 14:24:53 UTC
Created attachment 244411 [details] [review]
Use the same color for every long-runing lane
Comment 3 Ignacio Casal Quinteiro (nacho) 2013-05-16 14:30:44 UTC
Review of attachment 244410 [details] [review]:

Initial comments, please respect coding style
if ()
{
}
else
{
}
Comment 4 Techlive Zheng 2013-05-16 14:42:58 UTC
This is still a
Comment 5 Techlive Zheng 2013-05-16 14:44:11 UTC
This is still a trivial change, after testing some repo, it does not always show what I expected, I will keep working on it.
Comment 6 Techlive Zheng 2013-05-16 18:11:53 UTC
Created attachment 244444 [details] [review]
Repect the mainline of the merges

Normally the 1st parent of a commit is the branch we're merging _into_,
so if you keep this on the left then you can always read a merge as
right-to-left. Previous it seems to be a bit random.

By introducing an auto-increased id to every lane, the earier a lane is
created, the more priority it has.
Comment 7 Techlive Zheng 2013-05-16 18:12:38 UTC
Created attachment 244445 [details] [review]
Use the same color for every long-runing lane
Comment 8 Techlive Zheng 2013-05-16 18:14:15 UTC
Okay, I have revised the both patches, it should be good to go now. All the repositories I have tested so far, the history graph was rendered as expected.
Comment 9 Techlive Zheng 2013-05-16 18:18:51 UTC
Created attachment 244447 [details] [review]
Repect the mainline of the merges(fixed tab indentation)
Comment 10 Techlive Zheng 2013-05-16 18:19:46 UTC
Created attachment 244448 [details] [review]
Use the same color for every long-runing lane(fixed indentation)
Comment 11 Techlive Zheng 2013-05-16 18:20:38 UTC
Third version, fixed tab indentations.
Comment 12 Techlive Zheng 2013-05-16 18:22:45 UTC
Created attachment 244449 [details] [review]
Coding style fix
Comment 13 Techlive Zheng 2013-05-16 18:23:38 UTC
Created attachment 244450 [details] [review]
Repect the mainline of the merges(coding stayle fix)
Comment 14 Techlive Zheng 2013-05-16 18:24:56 UTC
Created attachment 244452 [details] [review]
0001 - Repect the mainline of the merges(coding stayle fix)
Comment 15 Techlive Zheng 2013-05-16 18:25:50 UTC
Created attachment 244453 [details] [review]
0001 - Repect the mainline of the merges(coding style fix)
Comment 16 Techlive Zheng 2013-05-16 18:26:23 UTC
Created attachment 244454 [details] [review]
0002 - Use the same color for every long-runing lane(coding style fix)
Comment 17 Techlive Zheng 2013-05-16 18:27:16 UTC
Okay, the submitting process is a hell. Now, it's good to go.
Comment 18 Techlive Zheng 2013-05-16 18:33:57 UTC
Created attachment 244455 [details] [review]
0001 - Repect the mainline of the merges (typo fix)
Comment 19 Techlive Zheng 2013-05-16 18:34:20 UTC
Created attachment 244456 [details] [review]
0001 - Respect the mainline of the merges (typo fix)
Comment 20 Techlive Zheng 2013-05-16 18:34:51 UTC
Created attachment 244459 [details] [review]
0002 - Use the same color for every long-runing lane (typo fix)
Comment 21 Ignacio Casal Quinteiro (nacho) 2013-05-17 06:51:20 UTC
Review of attachment 244456 [details] [review]:

See comments inline. Also I would like to ask you for a screenshot or something showing the difference between what we have and using your patch, since it is not easy to realize what we are reviewing here :)

::: libgitg/gitg-lanes.vala
@@ +28,3 @@
 	public bool inactive_enabled { get; set; }
 
+	// last assigned lane id

If you need a comment it means that the name is not good.
what about, d_lane_id ?

@@ +37,3 @@
 	class LaneContainer
 	{
+		// a unnique id to identify the lane, the later the lane

this comment does not seem very correct. the id is contained by the LaneContainer while in the comment you point that the id points to the specific lane

@@ +129,3 @@
 			// there is no lane reserver for this comit, add a new lane
+			LaneContainer newlane = new LaneContainer(myoid, null);
+			// we are creating a new lane, assign the id

no need for this comment, the previous one already points there is a new lane

@@ +214,2 @@
 				newlane.lane.from.prepend(pos);
+				// we are creating a new lane, assign the id

same here, remove the comment.
Comment 22 Techlive Zheng 2013-05-17 07:50:57 UTC
Created attachment 244488 [details]
Screenshots before the patch

As you can see, the order of master branch, gitg-0.2 branch and progress branch is random.
Comment 23 Techlive Zheng 2013-05-17 07:52:00 UTC
Created attachment 244489 [details]
Screenshots after the patch

As you can see, the master branch(mainline) is always displayed first.
Comment 24 Techlive Zheng 2013-05-17 07:56:48 UTC
Okay, take Gitg's repo as an example, here are the screenshots.

Also notice that in the second screenshot, the mainline lane of the development is displayed in the same color across the merge, this makes it easier to keep track of the evolvement of the project.

You can apply the patch, and try it with your other repositories and see how it is improved.
Comment 25 Techlive Zheng 2013-05-17 11:10:54 UTC
Created attachment 244508 [details] [review]
0001 - Respect the mainline of the merges (Revised version)
Comment 26 Techlive Zheng 2013-05-17 11:11:32 UTC
Created attachment 244509 [details] [review]
0002 - Use the same color for every long-runing lane (Revised version)
Comment 27 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:13:15 UTC
Review of attachment 244508 [details] [review]:

Commit message does not look like what it introduces the patch anymore...
Comment 28 Techlive Zheng 2013-05-17 11:14:35 UTC
Review of attachment 244508 [details] [review]:

It turns out that introducing an id for each lane is not necessary, the existing 'pos' variable serves the same purpose.
Comment 29 Techlive Zheng 2013-05-17 11:18:55 UTC
Created attachment 244510 [details] [review]
0001 - Respect the mainline of the merges (fixed commit message)
Comment 30 Techlive Zheng 2013-05-17 11:19:34 UTC
Created attachment 244511 [details] [review]
0002 - Use the same color for every long-runing lane (fixed commit message)
Comment 31 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:19:55 UTC
Review of attachment 244510 [details] [review]:

Patch looks good to me.
Comment 32 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:22:06 UTC
Review of attachment 244511 [details] [review]:

I guess it is fine.... :)
Comment 33 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:23:58 UTC
Comment on attachment 244510 [details] [review]
0001 - Respect the mainline of the merges (fixed commit message)

Thanks
Comment 34 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:24:23 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Comment 35 Techlive Zheng 2013-05-17 11:24:56 UTC
This is great, thanks.
Comment 36 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:26:18 UTC
Are you missing anything else to submit to this bug report?
Comment 37 Techlive Zheng 2013-05-17 11:28:57 UTC
https://live.gnome.org/Gitg/Wishlist might need to be updated.

And, Do I need to port the patch to gitg-2 branch?
Comment 38 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:30:21 UTC
not really, we are not making more releases for 0.2 unless there is a build break. I'm marking this then as fixed. Thanks for the patches.
Comment 39 Ignacio Casal Quinteiro (nacho) 2013-05-17 11:30:45 UTC
BTW in case that you want to join development you can usually find us in the gimpnet irc in the #gitg channel.
Comment 40 Techlive Zheng 2013-05-17 11:34:57 UTC
> not really, we are not making more releases for 0.2 unless there is a build
> break. I'm marking this then as fixed. Thanks for the patches.

Okay, then.

> BTW in case that you want to join development you can usually find us in the
> gimpnet irc in the #gitg channel.

That's great, I'd love to, I just started doing some gnome development mainly driven by improving this app, becasue I rely on it a lot. There is also some question about the gtk+ I need answers, the IRC is really great.
Comment 41 Sindhu S 2013-05-17 13:18:50 UTC
Added to wiki page in edit https://live.gnome.org/action/info/Gitg/Wishlist?action=diff&rev2=13&rev1=12

Thanks.