Refactor branch menu

[kgryte]

This PR resolves #513 and #514 by

1. completely refactoring how a user views, switches, and creates branches (the GitHub desktop app served as the reference design).
2. providing a means to filter branches when choosing a desired branch.
3. fixing a polling bug in the underlying extension model in which the model only refreshed the status, but not the list of branches, thus causing the dropdown to be out-of-sync if branches were created/deleted outside of the extension.
4. adding title attributes (and corresponding unit tests) to all buttons and UI elements in order to provide reasonably helpful hints to users not acquainted with the iconography or the UI.
5. leveraging Material UI React components for rendering lists and displaying dialogs in order to make components more consistent from a development perspective.
6. refactoring styles to better align with CSS best practice and to follow typestyle naming conventions.
7. splitting rendering logic into separate (smaller) methods to aid development and to assist developers in being able to "grok" rendering logic.
8. relying on the underlying extension model as the sole source of rendering truth, rather than a mixture of properties and state, thus allowing components to be more easily decoupled.
9. refactoring unit tests.
10. adding source code documentation to refactored components.

Demos

1. Basic usage (light theme)

Normal staging:

Simplified staging:

2. Basic usage (dark theme)

3. Branch menu filter (light theme)

4. Branch menu filter (dark theme)

5. Switching branches (light theme)

6. Switching branches (dark theme)

7. Creating branch (light theme)

8. Creating branch (dark theme)

9. Attempting to switch branches when branching with changes is disabled

10. Attempting to create a branch when branching with changes is disabled

Notes

• This PR has implications for Pin repository path #515, which, if this PR is accepted and the changes in that PR are desired, should be refactored to be based on the work in this PR.
• This PR has implications for Improve history panel #509, which, if this PR is accepted and the changes in that PR are desired, would need some minor modifications if based on the work in this PR.

[fcollonval]

@telamonian thanks for posting on this.
There is one change that should be done for me is the suppression of the button element for the repo menu and the right arrow. This gives the impression that some interaction is possible when it is not.

See L204 of components/Toolbar.tsx

Otherwise this is good for me.

[telamonian]

Mostly looks great! A few non-issues with docstrings and aria stuff. Also, the new branch dialog does not look right in dark mode (note the scrollbar):

I pushed a commit that adds the broad strokes in for dark mode by setting appropriate color and background-color in a couple of places:

You'll still need to tweak the style a bit to get it right (eg the branch icons have the wrong color, there's some strange white lines, etc).

There's also some styling defects related to the file list scrollbar. I played around with it a bit, and it seems like the right fix will be to prune/combine some existing DOM elements and then re-layout the top level components of the panel using display: flex and flex-direction: column. This will be a bit complex and the styling problem is minor, so I'm content to leave this for a future PR (it's also possible that #521 will help with the file list styling).

So, in summary:

• docstring fixes
• remove the two aria attrs
• fix the dark theme styling for the new branch dialog
• as per @fcollonval's comment, since the current repository button doesn't do anything right now, it shouldn't look/act like a button

and then the PR should be good to go!

[telamonian]

The bigger picture takeaway is that whether one uses ternary operators or short-circuiting is very unlikely to be a performance bottleneck. At this level of granularity, claiming performance benefits based on the spec is a fool's errand due to the nature of modern browser compiler technology.

@kgryte Well said, and the benchmarks provide a convincing object lesson. Thanks for doing those. Overall this has been a surprisingly instructive PR. It's been nice to see some typescript written with a different perspective/idiom from what I'm used to.

In the benchmarks, are the trivial conditionals:

if ( r !== r ) {
			b.fail( 'something went wrong!' );
		}

just there to ensure that the loop contents don't get optimized away?

[kgryte]

@telamonian Re: checks. That is correct. In this case, the goal would be to thwart "intelligent" compilers from skipping ahead to the last i and calling it a day. This sort of optimization is unlikely, as requires rather advanced speculation.

In general, however, when timing within loops, conditional checks are commonly necessary to prevent loop-invariant code motion, dead code elimination, and constant folding/propagation optimizations. It is also why an additional check outside of the loop is necessary in order to prevent the entire loop from being optimized away.

In this particular example, I used NaN checks based on the assumption that an optimizing compiler would be unlikely to reason about value ranges a priori and conclude that NaN is actually impossible. Based on the return value r being a double, a NaN check would be a valid test.

[kgryte]

@telamonian I have

• updated the docstrings
• removed the aria attributes
• fixed the dark theme styling for the branch dialog
• disabled the repo menu button and removed the arrow icon indicating that a button is functional

I believe this resolves the remaining issues.

[telamonian]

Yah, that's basically what I did (though sans import of classes)

NM, classes is already in the file

[kgryte]

@telamonian It is already imported: cac88a5#diff-f3df9dfa36cc17388ea613f8e3916891L2

[telamonian]

Other two commits? I have no idea what you're talking about

[kgryte]

@telamonian Must have been dreaming. ;)

[telamonian]

LGTM! Let's get this sucker pulled in

[kgryte]

@telamonian Hurray! Thanks for reviewing!