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

Add description to key binds #6358

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Add description to key binds #6358

merged 7 commits into from
Jun 11, 2024

Conversation

Moerliy
Copy link
Contributor

@Moerliy Moerliy commented Jun 7, 2024

Describe your PR, what does it fix/add?

I'm bad at remembering key binds and was always a fan of nvim plugins like which key. For something like this, a way of describing your key binds is necessary.

My proposal would look something like this:

bind = SUPER, T, "open my terminal", exec, kitty

but obviously, it should be backward compatible so this should also be no problem:

bind = SUPER, T, exec, kitty

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Working with any bind the description would always come after the KEY (so at index 2) and the indicator would be double quotes. I think some kind of indicator is needed to distinguish it from the dispatcher options.

Is it ready for merging, or does it need work?

Feedback is wanted but it's merge ready from my end.

closes #6357

- add description attribute to SKeybind

- changed handeBind accordingly
@Moerliy Moerliy changed the title Add description do key binds Add description to key binds Jun 7, 2024
@vaxerski
Copy link
Member

vaxerski commented Jun 7, 2024

why not add a flag for description like bindd?

Also, this will break with descriptions with ,

@Moerliy
Copy link
Contributor Author

Moerliy commented Jun 7, 2024

why not add a flag for description like bindd?

Good idea, to get ridge of " inside the key bind itself

Also, this will break with descriptions with ,

Yeah. had that in the back of my mind too. First reaction was to just use the docs to let the users know they shouldn't use the delimiter in the description.
I couldn't come up with a solution. It's the delimiter.
As far as I'm aware you can only get away with using the delimiter in the args part of the bind.
@vaxerski

@vaxerski
Copy link
Member

vaxerski commented Jun 7, 2024

yes, because we limit the amount of arguments to 4, so anything after the 3rd , is treated as one. With this, you can't do it. I'm fine with mentioning that , is not allowed in descriptions, but a flag would be nice.

Also, no need to ping, I have notifications on, you know.

@Moerliy
Copy link
Contributor Author

Moerliy commented Jun 8, 2024

I've added hyprctl functionality too now but I have a question on how you want the description to be handled.

  1. It can either be always in the printout and if there is no description it will have an empty value. Similar to args.
  2. Or we use the description flag to find out if there should be a description Key-Value pair or not.

Pro of the lather: Would be more in line with how it should be in my opinion, especially with the JSON out.
Con: more complex code where it isn't needed and people don't care if it's an empty string or no Key-Value pair.

The current implementation is the first.

@vaxerski
Copy link
Member

vaxerski commented Jun 8, 2024

for json you can omit the k-v if it's not set, but it doesn't matter.

Request review when you've done it the way you prefer

@Moerliy
Copy link
Contributor Author

Moerliy commented Jun 9, 2024

for json you can omit the k-v if it's not set, but it doesn't matter.

then it's fine with me 👍

I did the docs real quick too so I would say this PR and the docs PR are ready to review.
I can't assign a review tho or maybe I'm just too blind.

src/config/ConfigManager.cpp Outdated Show resolved Hide resolved
src/debug/HyprCtl.cpp Outdated Show resolved Hide resolved
src/managers/KeybindManager.hpp Outdated Show resolved Hide resolved
* changed hasDescriptions to hasDescription
@Moerliy
Copy link
Contributor Author

Moerliy commented Jun 9, 2024

Alright, let's try this again.

@Moerliy Moerliy requested a review from vaxerski June 9, 2024 18:30
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest lgtm

src/config/ConfigManager.cpp Outdated Show resolved Hide resolved
@Moerliy Moerliy requested a review from vaxerski June 10, 2024 11:39
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@vaxerski vaxerski merged commit e1b05f8 into hyprwm:main Jun 11, 2024
11 checks passed
@gksudolol
Copy link

I get an invalid dispatcher error when firing up hyprland from this commit with:

bindm = $mainMod, mouse:273, resizewindow

specifically with resizewindow, any other dispatcher works fine (this doesn't occur with the commit before)

@Moerliy
Copy link
Contributor Author

Moerliy commented Jun 11, 2024

I'm not sure here, but there really is no dispatcher called resizewindow but one resizewindowpixel.
Does the second one do what you are after?

@MightyPlaza
Copy link
Contributor

I'm not sure here, but there really is no dispatcher called resizewindow but one resizewindowpixel. Does the second one do what you are after?

mouse dispatcher

@nonetrix
Copy link

Yep same

@Moerliy
Copy link
Contributor Author

Moerliy commented Jun 11, 2024

Oh yeah, my bad. Found it. I did a push to my fork and I can also provide a patch if you want.

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

Successfully merging this pull request may close these issues.

Bind description
6 participants