Replace hints.uppercase with hints.labels #7661
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.labelssetting and the accompanying documentation (including how to replicatehints.uppercasebehaviour)hints.uppercaseand providing the migration code inYamlMigrationsinqutebrowser/config/configfiles.pyI'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.ymlincluding tohints.charsto complement the completion options inhints.labels. Is this an ok addition?YamlMigrationsinqutebrowser/config/configfiles.pybut 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.labelsalthough that doesn't do anything. I didn't complete understand howYamlMigrationsworks 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.