arxanas / scm-record

`scm-record` is a UI component to interactively select changes to include in a commit. It's meant to be embedded in source control tooling.
23 stars 9 forks source link

Allow "left arrow" and "h" to collapse text sections #39

Open emesterhazy opened 4 months ago

emesterhazy commented 4 months ago

After this change the "left arrow" and "h" keypresses will collapse a text section if it is expanded and currently selected.

This allows users to navigate around a diff more easily since they don't need to scroll through all of the lines in a section to get to the next section. Instead, they can just collapse the section quickly and move to the next one.

This fixes one of my only gripes about scm-record compared to the builtin editor in Mercurial.

emesterhazy commented 4 months ago

@arxanas Please take a look :)

I'd love to have this functionality in jj, which I think will require a release after this is merged.

emesterhazy commented 4 months ago

This probably needs another commit to hide the context between sections when they're collapsed, but it's still useful as is. Let me see if I can figure out how to hide the context.

arxanas commented 4 months ago

I didn't try this out yet; is it the same as https://github.com/arxanas/git-branchless/discussions/1051#discussioncomment-7282233?

emesterhazy commented 4 months ago

Reading through that thread, yes, I think it is probably the same thing. Except perhaps that I think I would also like to hide the unchanged lines above a collapsed section (and below the last section, ideally).

To be honest, I didn't even realize that I could press f to collapse a section. It never occurred to me that I could click on the menu items to discover the shortcuts. Edit: If the menu could be reached with the arrow keys, that might help discoverability. I'm just not in the habit of clicking on the terminal.

But even knowing that f collapses a section, I still think this FR makes the editor more ergonomic (and also less confusing to people coming from hg). Your concern here is legitimate, but can't the user can simply press the up arrow to move up without collapsing a section?

emesterhazy commented 4 months ago

Actually, I think this PR can stand alone without hiding the context when a section is collapsed. Hiding the context may still be useful, but that could be a separate PR.

~It might also be nice to have a visual indication that a section is collapsed. Mercurial puts two ** next to the section toggle, like this: [x]**. That could also be a separate PR, if you're interested.~ Just realized there's an indicator box on the right hand margin.

Edit2: The PR for hiding context around collapsed sections is https://github.com/arxanas/scm-record/pull/41

arxanas commented 4 months ago

I'm not opposed to collapsing the section by pressing left, but there needs to be some way to efficiently navigate by keyboard between files without collapsing the sections (that is, one that doesn't require an indefinite number of ups/downs/keypresses). This workflow might be common for users with large screens who want to see all of the sections at once while navigating between them.

Some ideas:

martinvonz commented 4 months ago
  • Add keybindings to jump to the next/prev file (the same for sections would also be nice).

FWIW, that's PgUp/PgDn in Mercurial's builtin editor.

emesterhazy commented 4 months ago

For mercurial PgUp and PgDn move to the next item of the same type. That means that if you're in the middle of a diff section you have to press left once to get to the section header before you can skip to the next section. Is that acceptable? It would be nice if it is since that would allow us to use a single pair of shortcuts to move between files and sections.

Currently PgUp and PgDn move the view but not the cursor. What if we changed the current view shortcuts to ctrl-PgUp and ctrl-PgDn and made PgUp and PgDn move to the next item of the same type. I think this would be my preference.

Alternatively, maybe we could use "n" to move down to the next item and "N" to move up to the next item, similar to how jumping between search results works in vim.

Since we're talking about the scrolling keybindings, I don't really understand how they were chosen:

Screenshot 2024-05-06 at 11 03 43

ctrl-y,e,b,f are all sort of far apart on the keyboard. What if we changed scroll up/down to ctrl-up and ctrl-down. That would also have better symmetry with ctrl-PgUp and ctrl-PgDn if we repurpose the unqualified PgUp and PgDn to move between items.

emesterhazy commented 4 months ago

Ok, I went ahead and opened https://github.com/arxanas/scm-record/pull/45 to change a few of the current key bindings and add new ones to jump between items of the same kind (i.e. jump between sections, files, or lines). This may not be sufficient to fully address the concerns raised in https://github.com/arxanas/scm-record/pull/39#issuecomment-2094952638, but it should be easy enough to add additional keybindings if we'd like.

emesterhazy commented 3 months ago

Perhaps we'd also like to add a key binding to move outwards without collapsing the section. Maybe ctrl-left or shift-left? I'll take a more holistic look at the types of movement we might want and the potential keybindings.

arxanas commented 3 months ago

Sorry, meant to comment earlier: many of the current keybindings were lifted directly from Vim

arxanas commented 3 months ago

"Next of same kind" keybindings are good but they don't address the problem of getting back to the top-level without collapsing intermediate sections. I think my preference would be a keybinding that jumps to the containing file. ctrl-left sounds good to me for that for now; we should revisit all the keybindings again later.

emesterhazy commented 1 month ago

@arxanas Sorry for the very long delay on fixing up this PR. I've pushed a new draft that adds ctrl-left and ctrl-h as keybindings to move outwards without collapsing a section. I also added ctrl-right and ctrl-l as alternative (and undocumented) bindings to move inwards for symmetry.

I think those changes address your concerns in https://github.com/arxanas/scm-record/pull/39#issuecomment-2106039356. The unqualified version of left and h will collapse the text section if it is expanded, otherwise it will move the focus outwards to the file.

Please take a look and thanks for your patience.

emesterhazy commented 1 month ago

Hi @arxanas, just wanted to give this a friendly ping. Hope you'll have time to take a look in the next week or two. Many thanks :)