Allow forced reload of revisions #11205
Conversation
mstv
force-pushed
the
fix/i9418_force_reload
branch
from
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.
Where will this be used? another PR?
There was a problem hiding this comment.
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.
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.
_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.
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.
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.
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.
mstv
force-pushed
the
fix/i9418_force_reload
branch
from
4eae2d3
to
7d93b8e
Compare
mstv
force-pushed
the
fix/i9418_force_reload
branch
2 times, most recently
from
fa8398d
to
e414384
Compare
mstv
force-pushed
the
fix/i9418_force_reload
branch
from
e414384
to
3878b77
Compare
Review comments + exception safety
mstv
force-pushed
the
fix/i9418_force_reload
branch
from
3878b77
to
2c2d683
Compare
Do not let cancellation kill the application
| semaphoreUpdateGrid.Wait(cancellationToken); | ||
| semaphoreUpdateGrid.Wait(cancellationToken); |
There was a problem hiding this comment.
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.
[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|Sortingoptions not being ignored sometimes.Proposed changes
RevisionGridControl:
PerformRefreshRevisions: Allow forced reload!_isRefreshingRevisionsfromCanRefreshOperationCanceledExceptioninOnRevisionRead, 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.