search

atlas-engineer / nyxt

Nyxt - the hacker's browser.
https://nyxt-browser.com/
9.65k stars 404 forks source link

Review hint mode style #3318

Closed aadcg closed 5 months ago

aadcg commented 5 months ago

Description

A follow-up to https://github.com/atlas-engineer/nyxt/pull/3302.

Fixes https://github.com/atlas-engineer/nyxt/issues/3179. Fixes https://github.com/atlas-engineer/nyxt/issues/3229.

@MaxGyver83 Feel free to review as well. See the sample config snippet below to play with the accepted values.

(define-configuration nyxt/mode/hint:hint-mode
  ((nyxt/mode/hint:hints-alphabet "KDJFLSAIEUROWPQCMVXZ")
   ;; (nyxt/mode/hint:hinting-type :vi)
   (nyxt/mode/hint:x-placement :right)
   (nyxt/mode/hint:x-translation 0)
   ;; (nyxt/mode/hint:y-translation -50)
   ))

Checklist:

MaxGyver83 commented 5 months ago

I have tested this branch/PR. I'm not sure if I have configured it wrong. But no matter if I use (nyxt/mode/hint:x-placement :right) or :left, the hints always cover the text: image In my original PR (#3226), the hints were always next to the text.

aadcg commented 5 months ago

I'm not sure if I have configured it wrong.

Yes, you're missing x-translation (take a look at its docstring). I've just edited a commit that makes the docstring slightly more informative. Give it a try:

(define-configuration nyxt/mode/hint:hint-mode
  ((nyxt/mode/hint:hints-alphabet "KDJFLSAIEUROWPQCMVXZ")
   (nyxt/mode/hint:hinting-type :vi)
   (nyxt/mode/hint:x-placement :right)
   (nyxt/mode/hint:x-translation 100)))
(define-configuration nyxt/mode/hint:hint-mode
  ((nyxt/mode/hint:hints-alphabet "KDJFLSAIEUROWPQCMVXZ")
   (nyxt/mode/hint:hinting-type :vi)
   (nyxt/mode/hint:x-placement :left)
   (nyxt/mode/hint:x-translation -100)))
MaxGyver83 commented 5 months ago

I'm not sure if I have configured it wrong.

Yes, you're missing x-translation (take a look at its docstring). I've just edited a commit that makes the docstring slightly more informative. Give it a try:

(define-configuration nyxt/mode/hint:hint-mode
  ((nyxt/mode/hint:hints-alphabet "KDJFLSAIEUROWPQCMVXZ")
   (nyxt/mode/hint:hinting-type :vi)
   (nyxt/mode/hint:x-placement :right)
   (nyxt/mode/hint:x-translation 100)))
(define-configuration nyxt/mode/hint:hint-mode
  ((nyxt/mode/hint:hints-alphabet "KDJFLSAIEUROWPQCMVXZ")
   (nyxt/mode/hint:hinting-type :vi)
   (nyxt/mode/hint:x-placement :left)
   (nyxt/mode/hint:x-translation -100)))

Sorry for not reading everything before trying. Indeed, this way it works!

This way, I always have to change two values when I want to switch between hints on the left and on the right side ;-). I can live with this.

MaxGyver83 commented 5 months ago

I noticed that you also removed the correction for hints that are not visible. In my original PR, the "C" hint was shifted in such a case: image

In this PR, the "C" is not visible. image (Sorry for the bad screenshot. The scroll bar covers the "C" link partially.)

Is this something you want to add again in a different PR?

aadcg commented 5 months ago

This way, I always have to change two values when I want to switch between hints on the left and on the right side ;-). I can live with this.

Well, :right and :left both mean drawing on top of the hinted element, which seems like a reasonable default and it is the model we have followed thus far (as explained above). You mean that your preferred customization involved setting 2 parameters: setting a non-default side and a non-default x-translation :)

aadcg commented 5 months ago

Is this something you want to add again in a different PR?

Indeed. It's a general issue that already existed so it's not directly related to the addition of these options. See #3250.

Although solving it is rather trivial, I've thought about it and I'm not even sure we should do it. Perhaps it should be an option as well. Well, we can discuss it in that issue.

MaxGyver83 commented 5 months ago

Well, :right and :left both mean drawing on top of the hinted element, which seems like a reasonable default and it is the model we have followed thus far (as explained above).

It's nice that this covers also the current behavior with just two options for x-placement. 👍

aadcg commented 5 months ago

Thanks @MaxGyver83!

Note that this change won't make into the 3 series releases. To use the feature, please compile from master.