Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document move_visual_line_* commands #6691

Closed
antoyo opened this issue Apr 10, 2023 · 19 comments · Fixed by #6918
Closed

Document move_visual_line_* commands #6691

antoyo opened this issue Apr 10, 2023 · 19 comments · Fixed by #6918
Labels
A-documentation Area: Documentation improvements C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR

Comments

@antoyo
Copy link
Contributor

antoyo commented Apr 10, 2023

Hi.
As part of the soft-wrap PR, the commands move_visual_line_down and move_visual_line_up were not documented.
It would be nice to add them to the documentation.
Thanks.

@antoyo antoyo added the C-enhancement Category: Improvements label Apr 10, 2023
@the-mikedavis the-mikedavis added A-documentation Area: Documentation improvements E-easy Call for participation: Experience needed to fix: Easy / not much labels Apr 10, 2023
@6lj6
Copy link

6lj6 commented Apr 10, 2023

I wanna a design discussion area where recorded all possible designs of helix:

  1. all the pros and cons of different models and methods.
  2. why we chose this, and omitted that.
  3. e.g. why there is no partial save: ws a.txt => write selection to new file

@sscheele
Copy link
Contributor

Newbie here and not 100% sure about this given the refactor, but there might already be some of what you're looking for here:

https://github.com/helix-editor/helix/blob/4dcf1fe66ba30a78edc054780d9b65c2f826530f/helix-term/src/commands.rs#L207-208

I agree that the descriptions don't seem super clear though - would be happy to take this on as a first issue and try to find a phrasing that would be easier to understand.

@antoyo
Copy link
Contributor Author

antoyo commented Apr 23, 2023

@sscheele: What I mean is that this is not documented on the website. It probably needs to be documented on this page.

@sscheele
Copy link
Contributor

Oh, I understand now. You're right - cargo xtask docgen doesn't seem to include the full list of commands in the generated documentation. I'll try to poke around and figure out why that is, but anyone else reading this is more than welcome to take up this task also and/or point me in the right direction :)

@pascalkuthe
Copy link
Member

only typed commands are aumatucally documented, keybindings are documented by hand

@sscheele
Copy link
Contributor

sscheele commented Apr 23, 2023

Hm, OK, I think I've played with that enough to understand it. It seems like there's currently no page on the site where unbound static commands are documented, but they do all have docstrings defined in the code. Would an extension to the docgen xtask routine that also documents the static commands (presumably in a different markdown file) be of interest, @pascalkuthe ? I think I could probably do that. Then they could all be added to the website as an easy reference.

@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 23, 2023

that seems reasonable but would be a slightly larger change and I think we want to also keep the manual keymap documentation aswell. The automatic documentation should be just a separate table of all commands IMO. So for this specific issue, I would still like to see the documentation adjusted manually (as the commands are bound in the default keymap). Adding the automatically generated table could be a separate PR

@sscheele
Copy link
Contributor

Makes sense to me! I can work on the docgen extension PR. I'm happy to do manual documentation as well, but one reason I suggested the docgen extension is that there doesn't seem to be a designated place to document unbound commands at the moment. The two most obvious options seem to be:

  • A new file in book/src
  • A new section in book/src/keymap.md

Happy to do either one!

@pascalkuthe
Copy link
Member

A new file seems better to me otherwise we endup with too many merge conflicts between auto generated text and handwritten documentation

@sscheele
Copy link
Contributor

I've started the manual documentation on a fork here. It also appears that the templating software used to generate the website has a {{#include <file>}} macro, so auto-generated documentation could be written to another file and then included in the handwritten page to avoid merge conflicts!

@pascalkuthe
Copy link
Member

Yeah inclydijg annexternal file instead of editing an existing file is all I meant with seperate file. I dont habe strong opinion whether it should be its own page but it seems a bit more structured. You can go with what you prefer and details can be discussed during PR review.

@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 24, 2023

BTW. move_visual_line_up is not inbound by default. Its bound to k by default and move_line_up is bound to gk by default (same for the down variant)

@gabydd
Copy link
Member

gabydd commented Apr 24, 2023

This was also being worked on here I think #3840

@sscheele
Copy link
Contributor

Oh thanks @pascalkuthe , I should have checked the actual software. Looks like perhaps that documentation also needs to be updated - the website says j/k are bound to move_line_down/up. I think the PR gabydd mentioned is a little further along, though, so perhaps this is unnecessary after all.

@pascalkuthe
Copy link
Member

I thought there was a PR for that but was only able to find #3690 and thought I missremenred. Thanks @gabydd .

Yeah that's what this issue was originally about: That the j/gj/k/gk documentation needs to be updated after the mappings were changed in #5420

@gabydd
Copy link
Member

gabydd commented Apr 24, 2023

I was thinking about the title of 3690 when I was searching at first through the PR's and couldn't find it either, I was able to find the correct issue though and the pr though that.

@sscheele
Copy link
Contributor

I think this issue can be closed, since it's addressed in the PR @gabydd found?

@the-mikedavis
Copy link
Member

This issue is separate from automatically generating docs for all commands (#3649).

This issue can be closed once these lines are updated to use the new commands

helix/book/src/keymap.md

Lines 35 to 36 in 77da0ae

| `j`, `Down` | Move down | `move_line_down` |
| `k`, `Up` | Move up | `move_line_up` |

@the-mikedavis the-mikedavis added the E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR label Apr 30, 2023
@sscheele
Copy link
Contributor

Ah, I understand. I've created the PR here: #6918

@the-mikedavis the-mikedavis linked a pull request Apr 30, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Documentation improvements C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants