-
Notifications
You must be signed in to change notification settings - Fork 409
Keyboard shortcuts #363
Keyboard shortcuts #363
Conversation
|
@smashwilson Thanks for tackling this! It's been languishing for a while. Is this also a good time to modify the command names? In particular, they're all prefixed with One other thing to keep in mind is that soon the right pane will have a tabbed interface (one for Git and one for GitHub) so the names and interactions should make sense in that context. |
Sure, I'm all ears for better command names. Would you prefer a uniform
👍 Gotcha. I'm guessing that we'll have |
28372ff to
b91b9dd
Compare
I think I would... I'd be curious as to others' 💭 here (cc @atom/core). Namespacing commands means we can't accidentally collide with a package named |
|
👍 Uniform github |
|
👍 Works for me. I've added a "rename commands" task to the checklist. |
|
Niiiiice. Excited to try this out. |
|
Oh I see there are still some boxes to check. |
|
Yah, I'm still working my way through 😁 In retrospect, I probably should have broken these up into a few separate PRs... I think the division points weren't as obvious until I'd actually crunched through some of them, though. |
2fd91b7 to
a9ef546
Compare
|
Hah - I was having problems with the I think there's still a problem with |
|
Yup. It turns out |
|
@BinaryMuse, @kuychaco I think this is ready for 👀 when you have the time 🙇 |
smashwilson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review 📔 fueled by ☕️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm calling the act of navigating from a StagingView entry to its FilePatchView a "dive," so the inverse operation of refocusing the StagingView from a FilePatchView is "surfacing." This is in contrast to "focusing" and "blurring" which already have meaning in a browser environment, or "selecting" and "deselecting" which have meaning in the StagingView, or "activating" which we're using elsewhere in the navigation code.
It still looks weird to me, though. As always, I welcome better naming ideas!
🏊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of our components were registering commands on atom.commands; others were accepting a CommandRegistry through their props and registering there. I unified on the props approach where I could because it made the tests a bit nicer.
lib/controllers/git-controller.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do anything yet, because merge conflicts are opened in an ordinary editor, where core:move-right already has a defined meaning that we don't want to interfere with.
@nathansobo: Did you have any insights for a way to navigate to and from general-purpose editors to the StagingView? For merge conflict resolution I'd prefer to keep a full editor around rather than writing a dedicated component, because you often need to edit more than just the marked conflict itself.
Maybe we could use ctrl-left and ctrl-right to dive and surface instead? It looks like those are unbound in my keybindings...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're safe with ctrl-left/right, but make sure to consider non-Mac platform idioms as you're looking for available key combinations. @damieng is a good person to double-check with on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Idioms" here meaning even if it's not bound in Atom it could be a common OS shortcut of some kind. That was clear as mud. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctrl-arrows are safe on Windows - they're currently bound in Atom to previous/next word (left/right) and move line (up/down). I think what Ash is proposing here is that the regular non-arrow keys move the files between the controls for staging/unstaging which is fine if it has the focus.
One thing to check is that you can still use them (and ctrl versions?) to do normal editor operations in the commit message box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what Ash is proposing here is that the regular non-arrow keys move the files between the controls for staging/unstaging which is fine if it has the focus.
Not quite. These keybindings are for moving focus (and if necessary selection) from an editor that was opened on a file with merge conflicts to that file's entry in the "merge conflicts" list in the git panel.
For files with unstaged or staged changes, pressing left-arrow with focus in the git panel list will launch and focus the FilePatchViewer that shows the list of hunks within that file. With the focus in a FilePatchViewer, pressing right-arrow does the reverse, moving focus to the git panel (with the corresponding file selected). That's doable because both the StagingView and the FilePatchViewer are custom panes introduced by this package, so there aren't any bindings to conflict with.
Merge conflicts are trickier because pressing left-arrow with a merge conflict selected in the StagingView opens a proper TextEditor on the conflicting file, so there's no obvious key binding to take the inverse action that doesn't collide with all of the keybindings we support in TextEditors in general. I was trying to brainstorm a binding that we could apply uniformly across TextEditors-on-merge-conflicts and FilePatchViewers-on-(un)?staged-hunks to shift the focus back.
Maybe we could use a chord with a prefix? I know open-on-github uses a ⌥-G chord prefix to namespace its actions... is there an intuitive prefix we could use for this package?
lib/controllers/git-controller.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of soft-coding like this for the unit tests because refs aren't assigned. It's kind of a smell to me. Maybe we should stub these methods instead in the tests instead to keep them concise - in non-test situations, this.gitPanelController should never be absent, and if that's not true I'd kind of prefer to get the exception notification about it rather than silently do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Michelle] Yeah, you're right, especially about the silent failures. Failures should be 🚨 🎺 🔊 🔥 ! I like the idea of stubbing the methods during tests. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I did a quick pass through and eliminated a bunch of these.
lib/views/commit-view.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work even though the tests use an isolated atomEnv. Bit of an abstraction leak though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah. Well we plan on moving these over to React, where we'll use context for atom services. Feel free to just throw a FIXME in here for now and we'll take care of it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yeah I was wondering if the React conversion would obsolete this anyway. I've added a FIXME just the same.
lib/views/git-panel-view.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nathansobo Now that I'm looking at this, I'm wondering if esc should close the panel rather than merely blur it, for consistent behavior with other panels like find-and-replace. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think closing the panel on esc makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm waffling about this again. The tree-view blurs on esc and the command is called tool-panel:unfocus... and we have ctrl-9 to toggle the panel's visibility. Ehhh. I'll leave it like this for now, we can always change it later if it feels wrong to enough users.
I did rename onCancel() to blur() to follow your advice on handler method names.
lib/views/git-panel-view.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
focusin bubbles so this callback is fired every time focus arrives anywhere within the GitPanelView. Much nicer than catching focus on every individual element that can receive it.
lib/views/staging-view.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BinaryMuse, @kuychaco: here's an oddity that I hit. If I await the update promise or return etch.update(this).then(() => true) or anything like that, I get an error from etch and the promise never resolves:
[4193:1215/084217:INFO:CONSOLE(90)] "Uncaught TypeError: Cannot read property 'update' of undefined", source: /Users/smashwilson/code/atom/github/node_modules/etch/dist/component-widget.js (90)
Here's the modified method:
async activateNextList() {
if (!this.selection.activateNextSelection()) {
return false;
}
this.selection.coalesce();
this.didChangeSelectedItems();
await etch.update(this);
return true;
}And here's the modified test case:
// test/views/staging-view.test.js
it('selects the first item of the next list', async () => {
assert.isTrue(await view.activateNextList());
assertSelected(['conflict-one.txt']);
assert.isTrue(await view.activateNextList());
assertSelected(['staged-one.txt']);
assert.isFalse(await view.activateNextList());
assertSelected(['staged-one.txt']);
});None of the tests require a re-render and the operations that invoke activateNextList() all perform an update themselves, so I was able to move forward, but it feels off to me not to wait for that re-render.
Do you have any idea what's going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Michelle] Okay so I traced this exception down into Etch, and I didn't spend a ton of time on it, but I can't quite figure out what's going on here.
In Etch, we used a ComponentWidget to wrap components and interface with the virtual-dom library. In it, we have an update method for the widget where if the widget hasn't change type, we save off the wrapped component from before the update:
if (this.componentConstructor === oldWidget.componentConstructor) {
this.component = oldWidget.component;However, while oldWidget is defined and has the correct componentConstructor, oldWidget.component is actually undefined.
@nathansobo Do you have any idea what's going on here? I can try to turn this into a small test case if that helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Michelle]
None of the tests require a re-render and the operations that invoke
activateNextList()all perform an update themselves, so I was able to move forward, but it feels off to me not to wait for that re-render.
Also, if you don't need to rerender but only want to await something to make sure the result of the prop call completes, you can use the next update promise from the Etch scheduler as mentioned above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would y'all think about just switching to React across the board sooner rather than later. I think trying to integrate Etch with virtual-dom is creating a lot of these weird issues. The new reconciliation approach should simplify things (at the cost of other potential bugs) but if you just go all React then it makes dealing with it less urgent. A consistent implementation seems nice anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's on my internal roadmap haha. Just not the highest priority thing given how soon we want to staff ship... although maybe it should be. Perhaps I'll look at it over the break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of taking a look at the migration too, and maybe sending a few PRs that convert a single component at a time to minimize disruption.
Should we make an issue with a checklist of all of the components to track our progress, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah @smashwilson, that would be most welcome :)
lib/views/staging-view.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called as part of the "surfacing" flow, by the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the last item in the staged changes list. I should have named the files staged-1.txt and so on so that they'd sort differently...
|
Cool, once the up arrow behavior is changed, I say 🚢 it!! ✨ |
☝️ I want to take a look at this before we merge, too. I've updated the checklist up above 😁 |
Changes incorporated, thanks! ✨
|
🚀 Yay! |
|
💥 💥 💥 ⌨️ |
Introduce and shuffle around the keyboard shortcuts used by the git panel. Note that this'll break pretty much everyone's muscle-memory once it's merged.
ctrl-9👉git:toggle-git-panel👉 Toggle the visibility of the git panel without changing the current focus.ctrl-shift-9👉git:toggle-git-panel-focus👉 If the git panel is not visible, reveal it. If the git panel is focused, blur it. If the git panel is not focused, focus it, preserving subfocus from the last time it had focus.esc👉core:cancel👉 Blur the git or GitHub panels.tab👉core:focus-next👉 Advance focus among (unstaged files, merge conflicts, staged files, the commit editor).shift-tab👉core:focus-previous👉 Retreat focus through (the commit editor, staged files, merge conflicts, unstaged files) without cycling.up👉core:move-up👉 Navigate the unstaged files, merge conflicts, and staged files as a single list.down👉core:move-down👉 Navigate the unstaged files, merge conflicts, and staged files as a single list.left👉core:move-left👉 Transfer focus from a file list to the corresponding FilePatchView.right👉core:move-right👉 Transfer focus from a FilePatchView to the corresponding entry in the file lists.enter👉core:confirm👉 Toggles the staging status on a selected file and advances the selection consistently withcore:focus-next.upanddown.Implements most, but not all, of #174.