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

Convention for default key bindings #257

Closed
vivien opened this issue Mar 10, 2014 · 12 comments
Closed

Convention for default key bindings #257

vivien opened this issue Mar 10, 2014 · 12 comments

Comments

@vivien
Copy link
Contributor

vivien commented Mar 10, 2014

As the tigrc configuration will change for 2.0 (complete tigrc file, no more default keybindings), it may be a good time to make the default one more intuitive for the user and use kind of a convention. For instance:

  • lowercase keys are used for view-switching actions or toggles. Single keys should be safe and harmless.
  • uppercase keys are used for view-specific and modifying actions, such as spawing git commands (commit, revert, ...).
  • Ctrl+key used for (?) maybe stuffs less related to git/tig, but related to the terminal or navigation, such as redrawing (e.g. Ctrl+L), Page Up/Down, etc.

The idea is to avoid confusion for the default configuration. You should have a clue about what impact will have r, R or ctrl-R.

Some bindings I would propose:

  • m, F1: view-main
  • d, F2 : view-diff
  • l, F3: view-log
  • t, F4: view-tree
  • b, F5: view-blame
  • h, F6: view-branch
  • s, F7: view-status
  • y, F8: view-stash
  • p, F9: view-pager
  • ?, F10: view-help
  • j: move-down
  • k: move-up
  • ...

1 to 0: the most used toggle- actions?

Status specific:

  • U or S: status-update
  • R or !: status-revert

And also:

  • ^L: screen-redraw
  • ^F: move-page-down
  • ^E: edit
  • ^S: stop-loading
  • ...

We may sacrifice some actions in favor of a simple configuration if there's not enough keys. As the actions are well named, it will be easy to fill this need with the upcoming readline support, e.g. :toggle-<Tab>.

@jonas
Copy link
Owner

jonas commented Mar 10, 2014

I like the idea of having a convention for the default keybindings. At the same time, we have to be very careful to not completely remap everything. This I like:

  • Using F1-F10 keys for views
  • Moving less useful actions to Ctrl "space": screen-redraw, stop-loading, show-version (the last one could even be retired I think).
  • Using uppercase keys for view specific and modifying actions (which is already generally the case except for the status keys).

Questionable:

  • Remapping h from help to branch view and remapping ? from search-back to view-help.
  • Remapping any navigation key that makes tig compatible with pagers, e.g. b.
  • Remapping u and 1 in the status and stage views.

That being said, I actually have a couple of things in mind myself regarding keybindings for which I would like some feedback. The idea is to move certain keys to use prompt-like bindings, which are more flexible in certain cases.

The first one is to potentially retire the special stage-next code (bound to @) and replace with:

bind stage     @       :/^@@

This could involve improving the search code to say match # of # like the chunk searching does.

I'm also currently working on a patch that moves all toggle actions to use :toggle <option-name>. This new :toggle prompt command would act differently based on the type of option. For bool and enum options it would work as today. For argv options (e.g. diff-options), it will fix #69 by allowing to add/remove one or more arguments (similar to how toggle-ignore-space works). For int options I am not currently decided, but it would either allows to increment/decrement the value (e.g. author-width) or simply replace with a given value.

Ideally, such a change will not have to break any exiting bindings and will give users more power to add custom bindings that manipulate options. Also, we are starting to have lot of toggle-* actions and this would clean this up once and for all and decouple actions from the option code.

Here's how the :toggle bindings would be remapped.

# Bindings for toggling settings
bind generic  .       :toggle show-line-numbers
                                              # Toggle line numbers
bind generic  D       :toggle show-date       # Toggle date display
bind generic  A       :toggle show-author     # Toggle author display
bind generic  g       :toggle show-rev-graph  # Toggle revision graph visualization
bind generic  ~       :toggle line-graphics   # Toggle (line) graphics mode
bind generic  Hash    :toggle showfilename    # Toggle file name display
bind generic  F       :toggle show-refs       # Toggle reference display (tags/branches)
# bind generic        ???     :toogle show-changes    # Toggle local changes display in the main view
bind generic  W       :toggle ignore-space    # Toggle ignoring whitespace in diffs
# bind generic        ?       :toggle commit-order    # Toggle commit ordering
bind generic  X       :toggle show-id         # Toggle commit ID display
bind generic  $       :toggle title-overflow  # Toggle highlighting of commit title overflow
# bind generic        ???     :toggle show-file-size  # Toggle file size format
# bind generic        ???     :toggle status-untracked-dirs
                                              # Toggle display of file in untracked directories
# bind generic        ???     :toggle vertical-split  # Toggle vertical split

@jonas
Copy link
Owner

jonas commented Mar 11, 2014

On the positive side, now that tig supports a 'safe' mode, I am more comfortable adding new view specific keybindings. The question is if we can come up with a good convention, e.g.:

bind branch ! ?git branch -D %(branch)
bind stage  ! ?git stash drop %(stash)

@vivien
Copy link
Contributor Author

vivien commented Mar 11, 2014

I'll get back soon on your comments. In the meantime, I just wanted to add another question. Do we need to keep the = sign for the set action?

e.g.:

set show-rev-graph = yes
set show-line-numbers = no
set title-overflow = 42

would become:

set show-rev-graph yes
set show-line-numbers no
set title-overflow 42

Bonus question: would the following implementation make the code simpler? Would it be a good idea?

set show-rev-graph
unset show-line-numbers
set title-overflow 42

@jonas
Copy link
Owner

jonas commented Mar 11, 2014

The = is not needed per se. I mostly based tigrc on muttrc which permits setting boolean values without the = ... and using a no prefix for negative values. It won't make the code simpler but it will probably make some users happy. But unset is a bit misleading, and with the - separated names the no prefix looks kind of weird, IMHO.

@vivien
Copy link
Contributor Author

vivien commented Mar 11, 2014

F keys for views are great (make me think about tabs).
I agree for ? and /, they are more intuitive for search.
We may this r for view-brrranch and h for help then?
prompt-like bindings and :toggle are really great!

Just a thought: might it be simpler if we support this?

bind generic  d   :view-diff
bind main     c   :!echo \"%(commit)\"
bind stage    2   ?@@    # prev chunk
bind stage    @   /@@    # next chunk

You define the action as you would have typed it within Tig. Then It would be sent as is.

@jonas
Copy link
Owner

jonas commented Mar 11, 2014

I'm not sure I understand what the 'c' binding is supposed to exemplify.
But I like your idea of moving view switching bindings to use prompt
commands. Ideally we should come up with a way to do finally allow to open
for any view with arguments as was discussed in another issue.

Regarding using 'r' for the branch view. I think it makes sense. It is
already more like a refs view and I am probably going to change it to also
display tags .

@jonas
Copy link
Owner

jonas commented Mar 12, 2014

BTW ? is already used as the confirm flag, so search bindings needs to be prefixed with :.

@vivien
Copy link
Contributor Author

vivien commented Mar 12, 2014

This c example binding is totally pointless. This one is better (note the :):

bind generic | :!@sh -c "echo -n %(commit) | xclip -selection c"

I would imagine that the code might be simplified if what you enter as the binding action can be exactly what you would have typed within Tig. But I'm not the expert here ;)

🐕 I'm 200% happy with dropping the branch view in favor of a generic "refs" view!

@vivien
Copy link
Contributor Author

vivien commented Mar 12, 2014

BTW ? is already used as the confirm flag, so search bindings needs to be prefixed with :.

You mean for a command which is already prefixed with a bang?
Is there a conflict between those eventual bindings?

bind generic w :!?git gc    # or actually !?git gc
bind generic z ?foobar

@jonas
Copy link
Owner

jonas commented Mar 13, 2014

Yes. In the last release I decided to allow external commands to start with any of the flags: !, @, ? and <.

jonas added a commit that referenced this issue Mar 31, 2014
This implements the non-controversial parts of #257:

  view-branch		H -> r
  view-grep		G -> g
  view-status		S -> s

  screen-redraw		no longer bound to r
  scroll-page-down	s -> ScrollFwd
  scroll-page-up	w -> ScrollBack

 :toggle show-rev-graph	g -> G
jonas added a commit that referenced this issue Mar 31, 2014
Change the name of the branch view (as discussed in #257) in preparation
for enhancing the branch view to also show tags (issue #134).
@cirosantilli
Copy link
Contributor

In addition to using upper case for things that modify git state, if #67 moves forward I propose we group all destructive bindings under the exclamation mark !. E.g.:

  • !r for reset --hard
  • !d for deleting various things: rm, branch -d, etc.

All of those bindings should also ask for confirmation with ? of course.

jonas added a commit that referenced this issue Apr 26, 2014
This implements part of issue #257 to ensure that Tig has a more
consistent default set of keybindings.

 - Lower-case keys are used primarily for view switching; and should
   always be non-destructive.
 - Upper-case keys are used for view-specific actions including
   user-defined commands.
jonas added a commit that referenced this issue Apr 26, 2014
This implements part of issue #257 to ensure that Tig has a more
consistent default set of keybindings.

 - Lower-case keys are used primarily for view switching; and should
   always be non-destructive.
 - Upper-case keys are used for view-specific actions including
   user-defined commands.
@jonas jonas closed this as completed in 0b123b1 Apr 30, 2014
@cirosantilli
Copy link
Contributor

I recommend:

bind refs   !   ?git branch -d %(branch)

instead of capital D. D is too dangerous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants