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

Refactor and fixup graph loading #10842

Merged
merged 6 commits into from Apr 1, 2023

Conversation

mstv
Copy link
Member

@mstv mstv commented Mar 29, 2023

Alternative to #10680

Proposed changes

  • Refactoring
  • Fixes
    • Fixup incomplete condition for graph cache loaded
    • Avoid unnecessary and incomplete lookahead for graph straightening
    • Fixup endless graph loading loop
  • Feature
    • Preload next graph page

Screenshots

N/A

Test methodology

  • manual

Merge strategy

Please do not squash merge the commits!


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

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.

+0
Proper review take a long time, seem reasonable though.

@RussKie
Copy link
Member

RussKie commented Mar 30, 2023

This type has some great comments, but I feel like additional inline comments for the changed lines could further help. E.g., I find this very useful:

// Try to detect this:
// | | |<-- previous lane
// |/ /
// * |<---- current lane
// | |
// | |
// | |
// |\ \
// | | |<-- lookahead lane
//
// And change it into this:
// | | |
// |/ |
// * |
// | |
// | |
// | |
// |\ |
// | | |
//
// also if the distance is > 1 but only if the other distance is exactly 1

A specific example: in lines:100-101 we're now multiplying by 2 - why and what does it mean?

@RussKie RussKie added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 👓 status: needs review labels Mar 30, 2023
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 30, 2023
@@ -458,36 +486,40 @@ static void StraightenLanes(int startIndex, int lastIndex, IList<RevisionGraphRo

bool moved = false;
IRevisionGraphRow previousRow = localOrderedRowCache[currentIndex - 1];
IRevisionGraphRow nextRow = localOrderedRowCache[currentIndex + 1];
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 was an unused leftover from the previous one-line straightening algorithm.

@@ -682,16 +683,19 @@ private async Task UpdateVisibleRowRangeInternalAsync()

if (AppSettings.ShowRevisionGridGraphColumn)
{
int scrollTo;
Copy link
Member Author

Choose a reason for hiding this comment

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

a pointless copy of newBackgroundScrollTo

@RussKie RussKie merged commit 171979f into gitextensions:master Apr 1, 2023
2 checks passed
@ghost ghost added this to the vNext milestone Apr 1, 2023
@mstv mstv deleted the fix/graph_lookahead branch April 1, 2023 07:44
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.

None yet

3 participants