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

Process events before PerformRefreshRevisions #9906

Closed
wants to merge 2 commits into from

Conversation

mstv
Copy link
Member

@mstv mstv commented Mar 19, 2022

Fixes #9223
Fixes #9792

Proposed changes

  • before calling RevisionGrid.PerformRefreshRevisions:
    • process events e.g. from closing FormProcess, particularly after rebase / conflict resolution actions
  • reset RevisionGridControl._isRefreshingRevisions before calling LoadingCompleted and SetPage
  • minor tuning of RevisionGraph.Clear: clear Dictionary instead of creating a new one

Screenshots

Before

no graph or empty RevisionGrid after rebase / merge / conflict resolution operations

After

RevisionGrid is updated without need for manual refresh

Test methodology

  • manual, e.g.
    • start rebase with conflicts
    • close dialog
    • abort

Test environment(s)

  • Git Extensions 33.33.33
  • Build 10d51fd
  • Git 2.35.1.windows.1
  • Microsoft Windows NT 10.0.19044.0
  • .NET 5.0.12
  • DPI 96dpi (no scaling)

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


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

@mstv mstv self-assigned this Mar 19, 2022
GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved
@mstv mstv changed the title Process events and update states before PerformRefreshRevisions Process events before PerformRefreshRevisions Mar 19, 2022
@@ -596,6 +596,9 @@ private void RefreshRevisions(Func<RefsFilter, IReadOnlyList<IGitRef>> getRefs)
return;
}

// Process events e.g. from closing FormProcess, particularly after rebase / conflict resolution actions
Application.DoEvents();
Copy link
Member

Choose a reason for hiding this comment

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

This is a "smell", meaning we're doing something wrong...
Can Invoke instead?

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 a "smell", meaning we're doing something wrong...

Yes, but what?

Can Invoke instead?

No, Invoke((Action)(() => RevisionGrid.PerformRefreshRevisions(getRefs))); does not suffice.
And this is no surprise because UICommands_PostRepositoryChanged is executed in the UI thread already.

Copy link
Member

Choose a reason for hiding this comment

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

The calling was changed in #9897. RefreshRevisions() is no longer async, but calculations is in async anyway, except for #9899 if artificial that is worse (still issues before and still not async in other situations) after this - I missed that, the calling structure was too complicated.

Copy link
Member

Choose a reason for hiding this comment

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

And this is no surprise because UICommands_PostRepositoryChanged is executed in the UI thread already.

Perhaps this is the underlying problem - we use this event for I/O, and not just for the UI updates.
I'm not asking for a change, just making an observation.

Copy link
Member

Choose a reason for hiding this comment

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

💭 thinking out loud - may be we need to have two events - one is fired on a background thread for I/O (UICommands_PostRepositoryChanged) and another one (DataRefreshed) - one the UI thread for the UI updates.

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 PR can wait until after #9888 and perhaps other follow-ups, which might even make it obsolete.
(I just reproduced the addressed issue many times when testing #9907 and had a look.)

@mstv mstv added the 🚧 status: experimental temporary commits, e.g. in order to try the test behavior on AppVeyor label Mar 20, 2022
@mstv mstv marked this pull request as draft June 2, 2022 20:38
@mstv mstv closed this Mar 26, 2023
@mstv mstv deleted the fix/9223_grid_refresh branch March 26, 2023 07:12
@vbjay
Copy link
Contributor

vbjay commented Mar 26, 2023

Highly recommend not using DoEvents
https://blog.codinghorror.com/is-doevents-evil/

Async code over DoEvents. DoEvents introduces rentry problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧 status: experimental temporary commits, e.g. in order to try the test behavior on AppVeyor
Projects
None yet
4 participants