-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Replace hints.uppercase with hints.labels #7661
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the description is sufficient here. I only understand it because I have read the discussion leading to this feature. Also, I would assume that most users do not use a config.py
and setting one up to preserve the previous functionality seems a bit too much IMHO. Couldn't we use some special value that would be interpreted as hints.chars.upper()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can always just set it to ASDFGHJKL
(assuming hints.chars
set to asdfghjkl
) - in fact, that's what the config migration does too!
Yeah, it's a bit cumbersome because you need to change things at two places - but at the same time, having a special <upper>
value or whatever seems weird to me as well.
As for the description, what would you suggest? Would something like a "For every character in hints.chars
, the character of this setting at the same position will be shown as the corresponding hint text." or so suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe the description could be more concrete. I think the line proposed by @The-Compiler is good and descriptive, but I'll throw my hat into the ring: "In hint text, characters of this settings will be shown instead of characters of hints.chars
." A little less descriptive, but a bit more concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can always just set it to ASDFGHJKL
(assuming hints.chars
set to asdfghjkl
) - in fact, that's what the config migration does too!
Yeah, it's a bit cumbersome because you need to change things at two places - but at the same time, having a special <upper>
value or whatever seems weird to me as well.
As for the description, what would you suggest? Would something like a "For every character in hints.chars
, the character of this setting at the same position will be shown as the corresponding hint text." or so suffice?
('abcdefghijklmnopqrstuvwxyz', None), | ||
(None, None), | ||
]) | ||
def test_hints_uppercase(self, yaml, autoconfig, chars, uppercase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super happy with this test. I dislike the use None
values, but I couldn't figure out how to do this in a nicer way. The problem is the interdependence of hints.chars
and hints.uppercase
. We have to test for the possibility that one or both of them is unset. Is there a better way to represent this than the use of None
values?
Thanks for the updates @tcftbl and sorry for the radio silence! I'll try to prioritize this once I get back around to merging PRs. Right now I'm focused on getting Qt 6 and qutebrowser v3.0.0 out of the door first. |
No worries! Let's revisit this later. |
This PR introduces a new configuration setting
hints.labels
. This allows user to use different symbols for onscreen hints and keys. The primary motivation of this is to make vim navigation keys 'hjkl' correspond to arrows '←↓↑→' on screen. Closes #7642This PR introduces the following changes:
hints.labels
setting and the accompanying documentation (including how to replicatehints.uppercase
behaviour)hints.uppercase
and providing the migration code inYamlMigrations
inqutebrowser/config/configfiles.py
I'm pretty new to this kind of programming and there are some things I'm not very confident about and would like to get some comments on:
qutebrowser/config/configdata.yml
including tohints.chars
to complement the completion options inhints.labels
. Is this an ok addition?YamlMigrations
inqutebrowser/config/configfiles.py
but I had some issues removinghints.uppercase
. When removing this options I had to replace it with something so that the yaml data remains valid (maybe I missed something here), so I replaced it withhints.labels
although that doesn't do anything. I didn't complete understand howYamlMigrations
works so I might be doing this in a dumb way. Is there a nice way to do this better or is this method sufficient?Any other feedback is also greatly appreciated! Thank you for your time.