Repository navigation
Fix the history list selection fighting the sidebar branch - #579
Conversation
The window controller creates its sidebar into `_sidebarController`, but the `sidebarViewController` property is backed by `_sidebarViewController`, which nothing ever assigns, so the property always returned `nil`. - Rename the ivar to the name the property synthesizes - This revives `-selectedRef`, whose sidebar branch could never be taken
fbe9161 to
d143c29
Compare
|
@hannesa2, Two things worth your attention before looking at the code:
Every commit builds on its own, so the series bisects. Thanks! |
|
I'm neither a ObjectiveC developer. I just keep the repo alive.
Please can you do this. Or do we have a chance to automate this ? I'm in the mode, that I merge stuff which sounds at least reasonable. But I'll fast to probably revert them too, when it makes issues. To sum up: tests are the key ! |
|
@hannesa2, I'm 100% with you on that. This usability-bug was indeed harder to crack, and took about a day to identify, test properly (manual+automated) and resolve. |
Assert that the window hands out the three view controllers it builds, which is the seam the sidebar property was silently failing at, and that the ref a ref action acts on follows whichever list has focus. - Load the real window nib and let `-windowDidLoad` build the controllers, rather than assembling them the way the other suites do - Check the sidebar handed out is the installed one, not a second instance - Assert a focused sidebar names its branch, and a REMOTES row the whole remote, which is what fetch and pull are offered on - Assert the history list still names the single branch on the selected commit, and nothing when two branches share it - Back it with a repository created under `NSTemporaryDirectory`, since the nib does not survive a repository with nothing in it
Move the history-list to a branch tip only while a sidebar branch change is outstanding, instead of on every update the list happens to get. - Track the outstanding branch change, and clear it once the tip has actually been shown rather than on the first attempt - Send the list back to the branch tip whenever the sidebar is clicked, including a second click on the branch already selected - Report from -selectCommit: whether the requested commit was found - Add PBGitHistorySelectionTests over the reselect decision
Restore the commit the user picked once the history-list has been read in, instead of leaving the list with nothing selected. - Remember the OID behind the selection as it changes - Re-select it after an update that found nothing owed to the sidebar, which is what a checkout leaves behind - Cover both the restore and its precedence against a branch change
Follow a click on a ref label in the history-list with the matching sidebar branch, so that the two views agree on which branch is being looked at. - Move the outline selection rather than writing to `currentBranch`, which the sidebar undoes as it preserves its own selection over the reload - Pull the ref hit-testing out of the double-click handler, so both clicks read the label under the mouse the same way
|
Hmm I can't reproduce this issue on 1.5.5 |
d143c29 to
4b99faf
Compare
Added tests to cover the sidebar-focused arm of |
I've fix the first scenario to make it more obvious -
(basically, when clicking a green (or blue) tag in the history-list, the focus on the sidebar does not update to reflect that, so any later "refresh" of the history-list "stomps" the (out-dated) status of the sidebar to the history-list) |
Selecting a branch used to fight with selecting a commit: the history-list would drag its selection back to the sidebar's branch on any update, and the sidebar would not follow a branch clicked in the history-list.
What did not work
Making a branch active from the history-list
Symptom
branch_Bin the sidebar - the history-list focuses on that branch's tip-commit (Figure 1)branch_Cin the history-list - the history-list focuses on that line (Figure 2)(the sidebar does not update-back - it is still focused on
branch_B)branch_Clabel in the history-list -branch_Cis checked out correctly,but the history-list jumps back to
branch_B's commit - not the one just clicked (Figure 3)Figure 1:
Figure 2:
Figure 3:
Cause
The checkout makes the list update, and every update re-asserted the tip of whatever the sidebar had selected, discarding the commit the user picked.
Clicking a branch label
Symptom
Single-clicking a green (or blue) branch-label selected the row, but the sidebar stayed on the previous branch, so the two views disagreed about which branch was being looked at.
Behavior after the fix
branch_C's label checks it out and the list stays on the commit that carries the label.a fetch, a
git switchin a terminal, the repository watcher -keeps that commit selected instead of snapping to a branch tip.
including a second click on the branch already selected,
which is now the deliberate way to get back there.
The commits
sidebarViewControllerhas returned nil since 2018The first one is worth a look on its own:
60d23480(2018) added thesidebarViewControllerproperty and converted working code onto it,but named the ivar
_sidebarControllerwhile the property synthesizes_sidebarViewController.Two ivars, one of them never assigned.
Nothing crashed because messaging nil is silent, and the one consumer had a fallback - so
-selectedRefhas been quietly taking its history-list branch even when the sidebar had focus.That is restored here, which is a behavior change beyond the reported bug and deliberate.
Test plan
branch_A/B/C:git switchin a terminal while a commit is selected,Two things to know
PBGitWindowWiringTestsloads the real window nib and creates a repository underNSTemporaryDirectory(), so it is the first test here that touches the filesystem and needs a window server - the nib does not survive a repository with nothing in it.(CI has since run it on
macos-26: both cases pass, at a cost of 3.8s of the suite's 5.9s, which is the price of loading a real nib and shelling out to git on a shared runner)GitXTestsnow linksObjectiveGit.framework, which it needs to construct aGTOID.