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

Ability to customize keybindings #11

Open
farzadmf opened this issue Jul 21, 2021 · 16 comments
Open

Ability to customize keybindings #11

farzadmf opened this issue Jul 21, 2021 · 16 comments

Comments

@farzadmf
Copy link
Contributor

Hi,

I installed the plugin and I'm enjoying it, thank you 👍

However, I was wondering if you plan to allow customizing the bindings through options. For example, a tool that I'm using is using Alt + v to do something, and that key is used by tilish.

I know there's the possibility of using the "prefix mode", but that would be somehow slower than using the Alt key combination. So I was wondering if it's possible to have something like this:

There are some defaults defined for keys, for example v and V, but they can be overridden using an option; something like set -g @tilish-vlayout = ... and set -g @tilish-vshiftlayout = ... (ignore the option names 😄 )

@farzadmf
Copy link
Contributor Author

farzadmf commented Jul 26, 2021

Didn't hear from you, so I did something in my fork, and more specifically this commit

If you think it's useful in your repo as well, please let me know and I create a PR (of course I'm gonna remove the paragraph saying "changes in this fork" from there 🙂 )

@jabirali
Copy link
Owner

Sorry for taking some time to get back to you :)

I like the idea of making these configurable, since as you say many users might get conflicts between alt-keybindings used by tilish and other terminal apps. (Something that is difficult to avoid entirely...)

I am however a bit skeptical of adding individual config options for each keybinding; in that case, it might be just as clean for the user to just fork the repository and change the code directly. However, perhaps a compact solution to this could be to use a similar approach to how the shiftnum variable handles international keyboards: instead of defining tilish-vsplit_split, tilish-vsplit_only, etc. (as in your fork), the user could define one variable tilish-layoutkeys, where the default value sSvVtz would be used to populate the default keybindings for layouts (in both prefix mode and default mode).

What do you think? If you think this sounds reasonable, would you be interested in submitting a PR, or should I look into it? :) (The code implementing these keybindings would then look similar to this.)

@farzadmf
Copy link
Contributor Author

Good to hear from you @jabirali 🙂

I think it's a good idea. If I remember. I can look into this when I have some free time for sure

@jabirali
Copy link
Owner

Great! Thanks a lot for the suggestion and the help :)

@farzadmf
Copy link
Contributor Author

@jabirali , it's been a long time, but recently, I needed to install a vim plugin that had a lot of conflicts with my tmux bindings, so I took another stab at this.

I made quite a few changes to make things configurable:

  • I added the layout_keys (as you suggested) to contain the keys used to change layouts (here)
  • I made the "easy mode" accepting two characters to enable/disable arrows/vim keys for pane splitting and movement (here)
  • I created a variable refresh_hooks to enable auto-refresh upon pane creation/deletion (here)

The README is up-to-date in my fork. If you get a chance, please take a look, and let me know if you want me to open a PR (I will remove my fork-specific content from the README and open a PR)

@kohane27
Copy link

kohane27 commented Jun 10, 2022

update: issue moved to farzadmf/tmux-tilish

docs: add a list of configurable keys in README

@farzadmf
Copy link
Contributor Author

Hey @kohane27 , I enabled issues in the fork. Can you please open the issue there? 🙂

@jabirali
Copy link
Owner

Hi @farzadmf,

Sorry for the late reply, and thanks a lot for implementing this!

I've now finally gotten around to go over your changes, and would be happy to merge it into master if you open a PR.

The only part I'm skeptical about is changing the easy-mode to a two-argument option, I think I want to revert that part before merging to master. That would help keep backwards compatibility with people who already setup easymode using the old format, and I think most users might want consistent keybindings for selection and movement as that is part of the core UX of i3wm-like keybindings. (Users with more particular needs might anyway want to fork and modify the plugin.)

I'm currently on vacation, but will be back in office in mid-August, and then plan to test your changes on a few different tmux versions just to make sure that there are no bugs across the supported versions. After that, I'll merge it if you open a PR. Are there are other breaking changes that users should be aware of (except the two-letter easymode argument)?

@farzadmf
Copy link
Contributor Author

farzadmf commented Jul 28, 2022

Hi @jabirali , thank you for your reply

for the late reply

I'd say it's a bit later than late 😝 (about 6 months), joking of course

But on the serious side, reason for saying that: honestly, it's been a while and I've lost track of what I changed and potential breaking changes.

I'll try to remind myself about the changes, but that's a very good point about easy-mode and it being a breaking change.

I'll get back to you once I get the chance to properly go over things

@kohane27
Copy link

@jabirali glad to hear from you!

@farzadmf thank you for willing to consider merging this feature!

For me I'll keep an eye on this:)

@jabirali
Copy link
Owner

Indeed, it has become later than late! 😅 Between starting a new job 🧐, getting married 🎉, and finally catching Covid 🦠, it's been an eventful spring on my part – so I have had to put off working on side projects like this until things settled down a bit. But I hope to follow up with less latency during the next half year 😉

I completely understand that the details are a bit fuzzy after some time. If you have a chance to go properly over things yourself before opening a PR, then that would be ideal. But if not, it's also fine to just open a PR with your fork's code as-is, and I can try to isolate the parts we wish to merge myself (especially layout_keys) once I'm back from vacation.

@farzadmf
Copy link
Contributor Author

No worries at all @jabirali , I guess 2 congrats and 1 sorry for what happened 😆 , and I sincerely hope everything is OK with you now.

Next half year should be enough to get a PR ready 😅 , but for sure, I'm planning to go over the changes myself first and then create a PR, and we can talk through the things in more detail then.

Anyway, glad that you're back 🙂

@jabirali
Copy link
Owner

Thanks! Indeed, everything is great on my end now 😄

Perfect! Then I'm looking forward to going over your PR once you have time to review the changes. And I agree that we shouldn't need another half-year to get this merged 😉 Meanwhile, I hope that you both have a nice summer!

@farzadmf
Copy link
Contributor Author

farzadmf commented Aug 1, 2022

Hey @jabirali , so I took a quick look and caught up with the changes that I've made (hopefully 😆)

I think the only breaking change that's there is the easymode one being a two-character thing now. The other settings are mostly new/not mentioned originally in the README.

So I was thinking to maybe rename the "new" easymode option to something else and keep the existing one as is (honestly, I can't think of a name and facing the delimma of programmer's naming 😅 , but we'll find something)

Also, let me know how you prefer:

  • Discuss things a bit here and then create a PR
  • Create the PR and have the discussion there

@star-szr
Copy link

Thanks for working on this! I don't want to make more work for anyone, but it seems to me like the easymode change (and possibly others, I haven't gone through every change) could be split into its own PR. The easymode change/addition does not seem directly related to the scope of this issue.

@jabirali
Copy link
Owner

jabirali commented Sep 20, 2023

And I agree that we shouldn't need another half-year to get this merged 😉

It seems I spoke to soon; it has been a very busy year, culminating in moving between cities and jobs again this summer, so I haven't managed to actively maintain tilish over the past year. But I hope we can now finally get this merged :).

@farzadmf, I agree with @star-szr that it's perhaps best to just split out the "easymode" part and merge the rest for now. If you open a PR, I'll give it a quick test (but it shouldn't need much testing since you've been using it for a while already), and I'll try to merge it as soon as possible. If there's anything we need to fix first, we can discuss it in the PR directly, or I can try to fix it myself during the merge. Let me know if you need any help or input from my side :).

Off topic: If anyone is reading this, and would be interested in helping with the maintenance of the plugin, please feel free to reach out. I think the plugin is more or less "feature complete" for what it does, so it's mostly in maintenance mode at this point. But I haven't been able to very actively respond to issues and pull requests over the past couple of years, so I would be happy if someone else would like to assist with this :).

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

No branches or pull requests

4 participants