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

Straighten graph lanes #9028

Merged
4 commits merged into from Mar 31, 2021
Merged

Conversation

mstv
Copy link
Member

@mstv mstv commented Mar 21, 2021

Fixes #5782

Proposed changes

  • Move lanes to the right if a branch would go to one lane left for one row
  • Use array instead of List<RevisionGraphRevision> in RevisionGraph

Screenshots

Before

grafik

After

grafik

Test methodology

  • manual

Test environment(s)

  • Git Extensions 33.33.33
  • Build b737b53
  • Git 2.27.0.windows.1 (recommended: 2.30.0 or later)
  • Microsoft Windows NT 10.0.19042.0
  • .NET Framework 4.8.4300.0
  • DPI 96dpi (no scaling)

✒️ I contribute this code under The Developer Certificate of Origin.

@mstv mstv self-assigned this Mar 21, 2021
@mstv
Copy link
Member Author

mstv commented Mar 21, 2021

Tests fail only on AppVeyor...

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is partly based on #5783

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to unit test this?

{
++_laneCount;
Validates.NotNull(_segmentLanes);
RevisionGraphSegment[] segmentsToBeMoved = _segmentLanes.Where(keyValue => keyValue.Value >= fromLane).Select(keyValue => keyValue.Key).ToArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RevisionGraphSegment[] segmentsToBeMoved = _segmentLanes.Where(keyValue => keyValue.Value >= fromLane).Select(keyValue => keyValue.Key).ToArray();
RevisionGraphSegment[] segmentsToBeMoved = _segmentLanes.Where(keyValue => keyValue.Value >= fromLane).Select(keyValue => keyValue.Key);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an iterator of the C++ std lib, which allows the modification of values.
Without, it throws InvalidOperationException "Collection has changed".

@mstv
Copy link
Member Author

mstv commented Mar 23, 2021

Is there a way to unit test this?

Yes, there is. I agree and will add tests for RevisionGraphRow.MoveLanesRight.

The algorithm for StraightenLanes in RevisionGraph.BuildOrderedRowCache is pretty straightforward.
The screenshot in the issue description #5782 and the comment in the function describe the simple but powerful idea.
Though a test would need extensive mock-ups.

My implementation - with moving all lanes right - avoids that independent branches use the same lane as in #5782 (comment).
Furthermore, it is important that the segments are processed in the order they have been added, not in the necessarily different order when the segments are drawn.

@mstv mstv marked this pull request as draft March 26, 2021 19:08
@mstv mstv force-pushed the feature/5782_straighten_lanes branch from db99fe5 to b737b53 Compare March 26, 2021 23:41
@mstv mstv marked this pull request as ready for review March 26, 2021 23:57
@mstv
Copy link
Member Author

mstv commented Mar 26, 2021

I have rebased and improved the performance. AppVeyor builds finish in normal time.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't run

@mstv
Copy link
Member Author

mstv commented Mar 28, 2021

@msftbot merge in 3 days

@ghost ghost added the status: auto merge label Mar 28, 2021
@ghost
Copy link

ghost commented Mar 28, 2021

Hello @mstv!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Wed, 31 Mar 2021 20:50:24 GMT, which is in 3 days

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@mstv mstv force-pushed the feature/5782_straighten_lanes branch from c11262f to ab29481 Compare March 31, 2021 16:01
@ghost ghost merged commit 049cc74 into gitextensions:master Mar 31, 2021
@ghost ghost added this to the 3.6 milestone Mar 31, 2021
@mstv mstv deleted the feature/5782_straighten_lanes branch March 31, 2021 20:52
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readability revision graph
3 participants