-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Allow forced reload of revisions #11205
base: master
Are you sure you want to change the base?
Conversation
3db6f43
to
0cd07bc
Compare
/// <param name="forceRefresh">Refresh may be required as references may be changed.</param> | ||
public void PerformRefreshRevisions(Func<RefsFilter, IReadOnlyList<IGitRef>> getRefs = null, bool forceRefresh = false) | ||
/// <param name="forceRefreshRefs">Refresh may be required as references may be changed.</param> | ||
/// <param name="skipIfAlreadyRefreshing">If true and grid is already / yet being refreshed, do not cancel and restart.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will this be used? another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will this be used? another PR?
perhaps in the cases where one can explain why it is really necessary to ignore updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In those situations the guard should be used.
/// and <see cref="ResumeRefreshRevisions"/>. | ||
/// </summary> | ||
private bool CanRefresh => !_isRefreshingRevisions && _updatingFilters == 0; | ||
private bool CanRefresh => _updatingFilters == 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_updatingFilters is a very bad name of this guard, is no longer related to filters, just the initial start or new load.
Rename to _suspendUpdateCounter or something.
The _isRefreshingRevisions check was required as some operations started multiple refresh operations even if just one refresh was ordered. Many of these scenarios were eliminated in the update primarily Russkie did one or two years ago (I was involved too). Maybe loading a new module is the only valid scenario where this applies, maybe that handling can be removed too. Have you checked that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rename the unchanged variable.
I have tested the cancellation on switching to another repo a few times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch repo still has a guard. The potential problem is all other situations where refresh is called.
When I looked at it, I did not dare to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked (most of) the reload chain and am pretty confident.
E.g. quickly switch Alt+VO (topo order) with Before and After: bad UX vs. just works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could work
4eae2d3
to
7d93b8e
Compare
fa8398d
to
e414384
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Briefly used, do not see direct issues
Cancelation token handling may need tweaks
e414384
to
3878b77
Compare
Review comments + exception safety
3878b77
to
2c2d683
Compare
Do not let cancellation kill the application
semaphoreUpdateGrid.Wait(cancellationToken); | ||
semaphoreUpdateGrid.Wait(cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing this twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer is not far away:
// Wait for refs,CurrentCheckout and stashes as second step
this.InvokeAndForget(() => ShowLoading(showSpinner: false));
semaphoreUpdateGrid.Wait(cancellationToken);
semaphoreUpdateGrid.Wait(cancellationToken);
{ | ||
ThreadHelper.AssertOnUIThread(); | ||
|
||
if (!CanRefresh) | ||
{ | ||
Trace.WriteLine("Ignoring refresh as RefreshRevisions() is already running."); | ||
Trace.WriteLine("Ignoring refresh as RefreshRevisions() is suspended."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] For any trace I'd add a prefix that would help identifying the source, e.g.
Trace.WriteLine("Ignoring refresh as RefreshRevisions() is suspended."); | |
Trace.WriteLine("[PerformRefreshRevisions] Ignoring refresh as RefreshRevisions() is suspended."); |
Fixes #9418
Fixes #11453
And it makes
View
|Sorting
options not being ignored sometimes.Proposed changes
RevisionGridControl:
PerformRefreshRevisions
: Allow forced reload!_isRefreshingRevisions
fromCanRefresh
OperationCanceledException
inOnRevisionRead
, which killed the appScreenshots
Before
need to wait until loading of previous revisions has finished and to manually refresh when e.g. switching repo
After
Test methodology
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.