Pub/sub for the left panel #5669

amaiorano · 2018-10-31T19:35:15Z

This is work based off the conversations from this other PR: #5617

Changes proposed in this pull request:

Rework RepoObjectsTree (ROT) so that instead of being "push" updated from FormBrowse via ROT.ReloadAsync, the Tree nodes (Branches, Remotes, and Tags) now "pull" for changes via UICommands.PostRepositoryChanged (i.e. "pubsub").

This refactor will allow for the Submodules tree work proposed in the other PR to be done more easily. In fact, I have a local commit on top of these changes for this, and it works very well now.

Screenshots before and after (if PR changes UI):

No screenshots as it behaves pretty much like before. Unfortunately, because each Tree node is responsible for updating itself independently, there's a bit more refresh glitchiness, but I tried to minimize it (see Tree.ReloadNodes).

What did I do to test the code and ensure quality:

This change removes the "push" update from FormBrowse, so I identified the places that used to perform this push, and made sure to address them, and test them. Namely:

Refresh (from button or menu): Refresh is used for when user does stuff in shell outside of GE, so we do care about this. This works by calling ROT.RefreshTree which acts as though the repo has changed.
FormReflog: Now uses GitUICommands instead of creating its own FormCreateBranch so that PostRepositoryChanged will get called.
Undo last commit: Nothing to do since undoing a commit shouldn't affect Branches/Remotes/Tags. (Note that with Submodules, we might revert a commit that affects submodule status, so maybe call SubmoduleProvider.Upate after? Or for now, just let user refresh until SubmoduleProvider can do its own polling for changes in the background?).
FormBrowse.RegisterPlugins: Nothing to do since ROT is not affected by plugins.

Apart from that, I tested:

Switching repos
Create/delete branches
Using FormReflog to create branches
Refreshing (both refresh button and "Reload" ROT context action) after making modifications outside the repo. Note that the "Reload" action should probably be removed since Refresh does the trick, and actually, if you create a branch outside of GE, then reload ROT, it will show the branch but FormBrowse will not, so probably better to just drop ROT.Reload and stick to Refresh only.

Has been tested on (remove any that don't apply):

GIT 2.19.1.windows.1
Windows 10

codecov · 2018-10-31T21:23:44Z

Codecov Report

Merging #5669 into master will increase coverage by 0.02%.
The diff coverage is 82.05%.

@@            Coverage Diff             @@
##           master    #5669      +/-   ##
==========================================
+ Coverage   45.13%   45.15%   +0.02%     
==========================================
  Files         652      652              
  Lines       49433    49453      +20     
  Branches     6576     6573       -3     
==========================================
+ Hits        22310    22332      +22     
- Misses      25862    25864       +2     
+ Partials     1261     1257       -4

gerhardol · 2018-10-31T22:59:55Z

Only browsed the code and see no breaking changes. I have been busy lately, will try to look closer in the next days.
It would be beneficial to rebase on master and to push the branch, to get more users trying this (at least me, it rebases cleanly on master right now). If the local branch is not in production status, just note that.

I tried this rebased on master and when pressing the refresh button the UI is frozen for like 30s..

The first discussion should probably be if this is the correct refactoring. Assuming so we need to convince @RussKie that this is an acceptable change for master. Hint: He is soft on tests. I have no suggestion really what to test though. There are really no tests on left side panel or submodule provider right now.

amaiorano · 2018-11-01T00:06:19Z

I'll rebase and push asap. 30 seconds? That's weird. I wonder if it's the increased async contention for the UI thread. Or maybe you refreshed while an update was in flight, which would cause cancellation. Though I tested that and it seemed to perform fine. Was it the GE repo?

Edit: rebased and pushed. Also, I've been trying to repro the long refresh, but have not been able to. I'm running a Release build without VS debugger attached. Were you testing Debug with VS attached? ?Maybe it's slower with VS's exception hooks.

Edit 2: Okay, I saw the slowdown compared to the previous version. Was not 30s for me, but sometimes around 5s. I compared to the previous code and there was a difference: formerly, LoadNodesAsync was awaited with ConfigureAwait(false), while in my version, I had not configured the awaiter. I added this, and now it runs much faster. If I understand correctly, ConfigureAwait(false) means we don't care about being on the same thread once the async function completes. In this case, we do want to get back on the UI thread, but I guess it's faster to ConfigureAwait(false) followed by await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();. Not sure why exactly, but it seems to do the trick. I squashed this fix and pushed the commit. Let me know if it's faster.

drewnoakes

Quick review in the browser but it looks good to me, modulo comments.

drewnoakes · 2018-11-01T08:51:58Z

GitUI/BranchTreePanel/RepoObjectsTree.Nodes.Branches.cs

                {
-                    TreeViewNode.NodeFont = new Font(AppSettings.Font, FontStyle.Bold);
+                    TreeViewNode.NodeFont = new Font(AppSettings.Font, IsActive ? FontStyle.Bold : default);


Consider caching Font objects behind some logic that creates new instances if AppSettings.Font changes. Then you can select between two fonts based on IsActive and do a ReferenceEquals check before calling the NodeFont setter (if that setter doesn't itself deal with setting to the current value).

Also, Font is disposable.

This makes sense, but I think perhaps should be tackled in a separate change? The reason I modified this code is that this fixes a bug with branch deletion where recycled TreeViewNodes wouldn't clear their bolded state.

drewnoakes · 2018-11-01T08:53:05Z