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 option to hide listBinds when pressing keys #1815

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

dazzywi
Copy link
Contributor

@dazzywi dazzywi commented Oct 21, 2024

Hello, I wanted to make an option that hides the bindings popup that shows up every time you press some sequence of keys.

Why? Because it can obstruct some files at the bottom of the screen.
And moreover the pressed keys are already shown by default if "rulerfmt" has %a expansion (as seen on the screenshots below, on bottom right of my window, when I press g)

Here is example, when the new drawbinds option is set to true (that is, by default)
before

And here is after, when the drawbinds option is set to false, and pressed any key
after

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contribution.

This sounds like a reasonable feature to add, and the changes looks good overall. I was just wondering whether it makes sense to call the option showbinds instead. To me, the word 'draw' indicates that the option is used when actually drawing the UI (as part of ui.draw). This is a minor point though.

@dazzywi
Copy link
Contributor Author

dazzywi commented Oct 22, 2024

I was thinking about the name of the option as well.
Mainly because there are already some toggleable options that are named in different convention, like hidden or preview, and no option (that I know of at least?) that has "show-" in the beggining of its name, but I think it sounds reasonable too.

Since the function in ui.go called listBinds I think it's more appropriate to go with "-binds," but might also consider showkeys or plain listbinds in all lowercase?

Do I need to submit another commit with changes here or open a new one?

@joelim-work
Copy link
Collaborator

I would say that the convention is to keep the option name as concise as possible, without being too ambiguous in its meaning. In the context of file managers, the meaning of hidden and preview should be self-explanatory, but the meaning of binds by itself is not clear.

So showbinds was the first thing that came to my mind, but I suppose listbinds is also okay too.

For the PR itself you can just keep adding new commits, no need to force-push. All the commits will be squashed into a single commit anyway when merging to the master branch.

@dazzywi
Copy link
Contributor Author

dazzywi commented Oct 22, 2024

I chose to go with showbinds, since it also was my idea from before I looked in the CONTRIBUTING note. Also changed placement of this option in documentation files since they all appear to be sorted alphabetically.

(And i'm not sure why but my editor keeps changing other lines in the file, like urls you see in diff files? Maybe something to do with indentation, sorry if this is inconvenient)

@joelim-work
Copy link
Collaborator

You must have your text editor configured to somehow delete \n characters at the end of the file, the CI check is currently failing.

The rest of the changes look fine, so if you can fix the line endings then I will approve the PR.

@dazzywi
Copy link
Contributor Author

dazzywi commented Oct 22, 2024

Should be done now I think, just did go fmt within my fork

@joelim-work
Copy link
Collaborator

Just out of interest, how did you add the \n characters back? I think if you do the same for doc.md, doc.txt and lf.1 then the diffs should disappear.

@joelim-work
Copy link
Collaborator

Actually it's probably fine I can fix it myself. Will merge it now then.

Thanks once again.

@joelim-work joelim-work merged commit 6cd5121 into gokcehan:master Oct 22, 2024
4 checks passed
@joelim-work joelim-work added the new Pull requests that add new behavior label Oct 22, 2024
@joelim-work joelim-work added this to the r33 milestone Oct 22, 2024
@joelim-work joelim-work mentioned this pull request Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new Pull requests that add new behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants