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 over multiple rows #9050

Merged
merged 1 commit into from Apr 17, 2021

Conversation

mstv
Copy link
Member

@mstv mstv commented Apr 4, 2021

Fixes #5782
Follow-up to / generalization of #9028

Proposed changes

  • Straighten lanes over multiple rows
  • Fix-up missing handling of RevisionGraphRow._revisionLane and RevisionGraph.GetCachedCount

Screenshots

Before After
grafik grafik

Remark: The part of the red branch starting at cff2c15 ending at the line with 03d75aa could be straightened, too. Though this is not detected by the current implementation because this part consists of multiple segments.

Test methodology

  • manual with GE and Linux repos
  • added NUnit test case

Test environment(s)

  • Git Extensions 33.33.33
  • Build 7835202
  • 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 Apr 4, 2021
Comment on lines +18 to +19
internal const int MaxLanes = 40;
private const int _straightenLanesLookAhead = 20;
Copy link
Member Author

Choose a reason for hiding this comment

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

These two constants could be turned into settings, which might solve #8498, too.

Copy link
Member

Choose a reason for hiding this comment

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

While I personally like options, I believe GE has too many options for most users so I will by default vote no to new options.
If this can be described well it could be added.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of another panel, the column could be made resizable. I am happy with the dynamic resizing.

This could be the configuration for MaxLanes

Copy link
Member

Choose a reason for hiding this comment

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

Can we detect automatically how many lanes required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we detect automatically how many lanes required?

For this, it would not suffice to load the revisions: The graph segments must be created, which is an expensive operation. That's why it is done in chunks of at most 1500 rows on user request.

MaxLanes is a hard-coded limit. (I have not introduced it. I moved it here from RevisionGraphColumnProvider in the predecessor PR.)

@mstv mstv force-pushed the feature/5782_straighten_lanes branch from d38d26b to dfb777d Compare April 15, 2021 15:02
@ghost
Copy link

ghost commented Apr 15, 2021

How do you like the idea to mirror the graph vertically?
There will be one line on the right.
On the left, hide, scroll or expand the graph in a separate panel.

@mstv
Copy link
Member Author

mstv commented Apr 15, 2021

How do you like the idea to mirror the graph vertically?

Pro: The main branch would be located directly beside the commit subject.
Con: Would be very nervous when the number of graph lanes changes and the graph width is adapted automatically.
Con: I read from left to right - also the graph.

There will be one line on the right.

I do not see the benefit of showing only one line of the graph.

On the left, hide, scroll or expand the graph in a separate panel.

The whole graph can be hidden using the View menu.
Instead of another panel, the column could be made resizable. I am happy with the dynamic resizing.

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.

:shipit:

@mstv mstv merged commit f840a0a into gitextensions:master Apr 17, 2021
@ghost ghost added this to the 3.6 milestone Apr 17, 2021
@mstv mstv deleted the feature/5782_straighten_lanes branch April 17, 2021 08:10
@RussKie
Copy link
Member

RussKie commented Jun 5, 2021

This looks awkward:
image

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