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

[emesterhazy]

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]

@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]

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]

I didn't try this out yet; is it the same as arxanas/git-branchless#1051 (reply in thread)?

[emesterhazy]

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]

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 #41

[arxanas]

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:

• Add keybindings to jump to the next/prev file (the same for sections would also be nice).
• Add keybindings to jump to the top-level file directly (and maybe the same to jump into the first/last line in the file would also be justified).
• Some kind of context-sensitive sequence where you press left -> f -> left and then the section stays open the second time. (This might be a bad idea in practice.)

[martinvonz]

• 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]

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:

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]

Ok, I went ahead and opened #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 #39 (comment), but it should be easy enough to add additional keybindings if we'd like.

[emesterhazy]

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]

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

[arxanas]

"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]

@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 #39 (comment). 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]

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 :)

[emesterhazy]

@arxanas I noticed that you pushed the 0.4 release yesterday. Nice! Although I am a little bummed because it would have been nice to have this change included in a release that jj can use. Are you still open to merging this? I just synced the branch to main, so this should merge cleanly.

Let me know if there's anything else I can do. Thanks!

[arxanas]

@emesterhazy sorry about that, I haven't had time to review due to being busy with work. I have had a little free time recently but am focused in fighting fires/getting out bugfix releases (for scm-record redrawing issues => git-branchless and jj as downstream consumers, plus git-branchless for Git 2.46+).

Hopefully I'll get a chance to review soon. I prefer not to release meaningful changes without giving them some time to "bake" on the main branch and get feedback from bleeding-edge users (in particular me 😁, sometimes others). Oftentimes there are bugs or I want to redo/rethink something altogether.

That being said, I haven't thought it through as much for this case, where we're publishing a library instead of a binary directly. That is, bleeding-edge jj and git-branchless users won't get the latest scm-record changes even if they're building the binaries from source.

• Maybe we could release scm-record more frequently, and rely on users installing jj and git-branchless without --locked?
  • Then cargo should try to install the latest compatible version for each dependency.
  • I do it as part of actual development, but the installation instructions advise --locked, since it's not always guaranteed to compile otherwise.
• Maybe we could set jj and git-branchless to depend on scm-record at main?
  • It would have to be undone before cargo publish IIRC.
• Something else?

[martinvonz]

• Maybe we could release scm-record more frequently, and rely on users installing jj and git-branchless without --locked?

That would work.

Maybe we could set jj and git-branchless to depend on scm-record at main?

I think that would make jj's security policy bot unhappy.

[emesterhazy]

No worries regarding not having time to review. We're all doing this for fun and to make the tools we use every day even better. You've already made a huge contribution in creating scm-record. But, if you do have time to review soon, that would be amazing since I'd really like to use this with Google's internal build of jj (I've already compiled it into the version I use with git).

Regarding the versioning, it sounds like Martin is on top of it, but if I'm understanding correctly, we'd still need to update the version listed in the Cargo.toml, right? That doesn't seem like a big deal if there's a 0.5 release to point to for example, I'm just not sure how building without --locked factors into things if the Cargo.toml specifies an exact version.

[arxanas]

I think that would make jj's security policy bot unhappy.

@martinvonz Does it make a difference if we were to pin the dependency to a specific hash and update that frequently? It might be easier than actually publishing new releases frequently. (I guess it could be triggered by CI.)

Regarding the versioning, it sounds like Martin is on top of it, but if I'm understanding correctly, we'd still need to update the version listed in the Cargo.toml, right? That doesn't seem like a big deal if there's a 0.5 release to point to for example, I'm just not sure how building without --locked factors into things if the Cargo.toml specifies an exact version.

@emesterhazy I was referring to https://doc.rust-lang.org/cargo/reference/resolver.html#semver-compatibility, in which cargo will just go grab more-recent semver-compatible packages, unless you pass --locked. (A missing operator is the same as ^.)

I forgot about this bit, though:

This convention also applies to versions with leading zeros. For example, 0.1.0 and 0.1.2 are compatible, but 0.1.0 and 0.2.0 are not. Similarly, 0.0.1 and 0.0.2 are not compatible.

It might be fine to regularly publish on the 0.4.x (etc.) series for bleeding-edge development purposes and only adopt 0.5.0 (etc.) for the locked resolutions of git-branchless and jj releases. We'd have to be careful to only adopt 0.x.0 versions in their Cargo.lock files, though (it's pretty easy to accidentally accidentally update dependencies in my experience 😬).

Possibly there's a better solution using some aspect of the version requirement syntax or prerelease versions.

[martinvonz]

@martinvonz Does it make a difference if we were to pin the dependency to a specific hash and update that frequently? It might be easier than actually publishing new releases frequently. (I guess it could be triggered by CI.)

Yes, that would work. I almost suggested doing that but it would still be mean that we don't get the new versions imported at Google (we only import released versions). But that's a separate problem.

OTOH, I don't think it sounds so bad to release versions that might have bugs, especially if you're saying that it would just impact anyone using scm-record as a library (as jj does). So if you release a new scm-record version soon, we'd have about 3 weeks to test it in jj before the next release (early november).

[emesterhazy]

I synced this with main and everything still works. Friendly ping hoping you'll get a chance to look at this soon. The changes are actually fairly minimal; a large portion of the diff is just a new test case.

[arxanas]

Thanks for your patience and for implementing this! This is probably a quality of life improvement for many users overall, so we can ship it even if it has some theoretical concerns.

[arxanas]

@emesterhazy Thanks for working on this! Sorry for the delay; I had been busy at work, and then a little burnt out afterwards, but I think I've more or less recovered. You can merge this when you think it's ready. I think it's just nits and idle long-term speculation on my end.

• If you want to get it into upstream jj ASAP, then we can just release a new version and upgrade in jj.
  • To release a new version, we need to update CHANGELOG.md, publish a new tag, publish release notes, and publish to crates.io.
• If you just want it for local use, you can probably build jj from source and set path or rev.

[emesterhazy]

Thanks for the review and great feedback. Let me address your comments before we merge this. After that, a new release would be great so that it can be incorporated into jj.

[emesterhazy]

I think GitHub lost one of my comments. I made the following changes:

• Replace ctrl-left etc with shift-left to avoid conflicting with the macOS window manager
• Rename the help menu items for consistency:
  • Outer item without folding
  • Outer item with folding
  • Inner item with unfolding

I'll merge this and work on a PR to push a new release later. Thanks again!