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

Fix RepoObjectsTree (left panel) remaining blank when ReloadAsync is … #5779

Conversation

amaiorano
Copy link
Contributor

@amaiorano amaiorano commented Nov 15, 2018

…cancelled (e.g. by spamming refresh)

Problem is that if during ReloadAsync, a System.OperationCanceledException is thrown due to cancellation, we cannot use token anymore in the finally block to switch to main thread. Doing so causes another exception to be thrown, and we never end up executing the code below, which includes "treeMain.EndUpdate()" required to restore rendering of the tree view.

Fixes #5778

@amaiorano
Copy link
Contributor Author

This is to fix #5778. I think this is the right fix - not to use 'token' after CancellationException is thrown.

@amaiorano amaiorano force-pushed the amaiorano/fix-rot-blank-on-refresh-spam branch from a4bd667 to 480e617 Compare November 15, 2018 04:11
@codecov
Copy link

codecov bot commented Nov 15, 2018

Codecov Report

Merging #5779 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5779      +/-   ##
==========================================
- Coverage   36.95%   36.94%   -0.01%     
==========================================
  Files         619      619              
  Lines       47403    47404       +1     
  Branches     6320     6321       +1     
==========================================
- Hits        17519    17515       -4     
- Misses      29106    29108       +2     
- Partials      778      781       +3

@amaiorano
Copy link
Contributor Author

Amended the commit with your suggeston @RussKie

treeMain.BeginUpdate();
_rootNodes.ForEach(t => t.IgnoreSelectionChangedEvent = true);
var tasks = _rootNodes.Select(r => r.ReloadAsync(token)).ToArray();
await Task.WhenAll(tasks).ConfigureAwait(false);
}
finally
{
await this.SwitchToMainThreadAsync(token);
await this.SwitchToMainThreadAsync();
Enabled = true;

var selectedNode = treeMain.AllNodes().FirstOrDefault(n =>
Copy link
Member

Choose a reason for hiding this comment

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

The token protection has to only be excluded for EndUpdate the rest has to remain unchanged.

        public async Task ReloadAsync()
        {
            var currentBranch = Module.GetSelectedBranch();
            await this.SwitchToMainThreadAsync();

            var token = CancelBackgroundTasks();
            Enabled = false;

            try
            {
                treeMain.BeginUpdate();
                var tasks = _rootNodes.Select(r => r.ReloadAsync(token)).ToArray();
                await Task.WhenAll(tasks).ConfigureAwait(false);
            }
            finally
            {
                await this.SwitchToMainThreadAsync();
                if (!token.IsCancellationRequested)
                {
                    Enabled = true;
                    var selectedNode = treeMain.AllNodes().FirstOrDefault(n =>
                        _rootNodes.Any(rn => $"{rn.TreeViewNode.FullPath}{treeMain.PathSeparator}{currentBranch}" == n.FullPath));
                    if (selectedNode != null)
                    {
                        _rootNodes.ForEach(t => t.IgnoreSelectionChangedEvent = true);
                        treeMain.SelectedNode = selectedNode;
                        _rootNodes.ForEach(t => t.IgnoreSelectionChangedEvent = false);
                        treeMain.SelectedNode.EnsureVisible();
                    }
                }

                treeMain.EndUpdate();
            }
        }

Copy link
Contributor Author

@amaiorano amaiorano Nov 15, 2018

Choose a reason for hiding this comment

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

Yes, that makes sense @jbialobr. At first I wondered if we should include Enabled = true next to treeMain.EndUpdate, but I suppose it's fine since cancelling means another update is in flight. So during the time between cancelling and the next update, we'll see the tree's contents disabled. I'll upload an amendment with this version.

Edit: @jbialobr the code I will upload is slightly different from yours - I think you had an older version of this function that you modified? Anyway, you'll see the diff.

…cancelled (e.g. by spamming refresh)

Problem is that if during ReloadAsync, a System.OperationCanceledException is thrown due to cancellation, we cannot use token anymore in the finally block to switch to main thread. Doing so causes another exception to be thrown, and we never end up executing the code below, which includes "treeMain.EndUpdate()" required to restore rendering of the tree view.
@amaiorano amaiorano force-pushed the amaiorano/fix-rot-blank-on-refresh-spam branch from 480e617 to 5a95223 Compare November 15, 2018 06:07
@RussKie RussKie merged commit 61b97e7 into gitextensions:master Nov 15, 2018
@RussKie
Copy link
Member

RussKie commented Nov 15, 2018

Thank you all

@amaiorano amaiorano deleted the amaiorano/fix-rot-blank-on-refresh-spam branch November 15, 2018 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants