Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

[Robert Pluim]

>>>>> On Tue,  9 Jul 2024 12:32:12 -0400 (EDT), Mattias Engdegård via Mailing list for Emacs changes <emacs-diffs@gnu.org> said:

    Mattias> branch: master
    Mattias> commit ac797f60160848fb625db4855befc68352d6cbd2
    Mattias> Author: Mattias Engdegård <mattiase@acm.org>
    Mattias> Commit: Mattias Engdegård <mattiase@acm.org>

    Mattias>     ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.
    Mattias> ---
    Mattias>  lisp/net/shr.el | 8 ++++----
    Mattias>  1 file changed, 4 insertions(+), 4 deletions(-)

    Mattias> diff --git a/lisp/net/shr.el b/lisp/net/shr.el
    Mattias> index 4ccd8a5a85a..39271cc5296 100644
    Mattias> --- a/lisp/net/shr.el
    Mattias> +++ b/lisp/net/shr.el
    Mattias> @@ -233,10 +233,10 @@ can be one of the following symbols:
    Mattias>  * `fill-height': Display the image zoomed to fill the height of the
    Mattias>  current window."
    Mattias>    :version "31.1"
    Mattias> -  :type '(set (choice (const :tag "Fit to window size" fit)
    Mattias> -                      (const :tag "Original size" original)
    Mattias> -                      (const :tag "Full image size" image)
    Mattias> -                      (const :tag "Fill window height" fill-height))))
    Mattias> +  :type '(set (const :tag "Fit to window size" fit)
    Mattias> +              (const :tag "Original size" original)
    Mattias> +              (const :tag "Full image size" image)
    Mattias> +              (const :tag "Fill window height" fill-height)))

Wouldnʼt this look better:

  :type '(repeat (radio
                  (const :tag "Fit to window size" fit)
                  (const :tag "Original size" original)
                  (const :tag "Full image size" image)
                  (const :tag "Fill window height" fill-height))))

It makes selection easier, since it shows the available options for
each item.

Robert
-- 

Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

[Mattias Engdegård]

9 juli 2024 kl. 18.48 skrev Robert Pluim <rpluim@gmail.com>:

> Wouldnʼt this look better:
> 
>  :type '(repeat (radio
>                  (const :tag "Fit to window size" fit)
>                  (const :tag "Original size" original)
>                  (const :tag "Full image size" image)
>                  (const :tag "Fill window height" fill-height))))

That's something for the original author (J.P.) to decide; I just made the type agree with the value.

Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

[Robert Pluim]

>>>>> On Wed, 10 Jul 2024 10:46:56 +0200, Mattias Engdegård <mattias.engdegard@gmail.com> said:

    Mattias> 9 juli 2024 kl. 18.48 skrev Robert Pluim <rpluim@gmail.com>:
    >> Wouldnʼt this look better:
    >> 
    >> :type '(repeat (radio
    >> (const :tag "Fit to window size" fit)
    >> (const :tag "Original size" original)
    >> (const :tag "Full image size" image)
    >> (const :tag "Fill window height" fill-height))))

    Mattias> That's something for the original author (J.P.) to decide; I just made the type agree with the value.

The type agrees with the value, but not the documentation:

    Each element
    can be one of the following symbols:

    * `fit': Display the image at its original size as requested by the
      page, shrinking it to fit in the current window if necessary.
    * `original': Display the image at its original size as requested by the
      page.
    * `image': Display the image at its full size (ignoring the width/height
      specified by the HTML).
    * `fill-height': Display the image zoomed to fill the height of the
    current window."

The current type lets you choose to enable each option, but not to
reorder them without resorting to lisp code.

Robert
-- 

Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

[Philip Kaludercic]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Wed, 10 Jul 2024 10:46:56 +0200, Mattias Engdegård <mattias.engdegard@gmail.com> said:
>
>     Mattias> 9 juli 2024 kl. 18.48 skrev Robert Pluim <rpluim@gmail.com>:
>     >> Wouldnʼt this look better:
>     >> 
>     >> :type '(repeat (radio
>     >> (const :tag "Fit to window size" fit)
>     >> (const :tag "Original size" original)
>     >> (const :tag "Full image size" image)
>     >> (const :tag "Fill window height" fill-height))))
>
>     Mattias> That's something for the original author (J.P.) to decide; I just made the type agree with the value.
>
> The type agrees with the value, but not the documentation:
>
>     Each element
>     can be one of the following symbols:
>
>     * `fit': Display the image at its original size as requested by the
>       page, shrinking it to fit in the current window if necessary.
>     * `original': Display the image at its original size as requested by the
>       page.
>     * `image': Display the image at its full size (ignoring the width/height
>       specified by the HTML).
>     * `fill-height': Display the image zoomed to fill the height of the
>     current window."
>
> The current type lets you choose to enable each option, but not to
> reorder them without resorting to lisp code.

Isn't that solved by adding a :greedy (see (widget) checklist).
Arguably this should be enabled by default for `set' types.

> Robert

-- 
	Philip Kaludercic on peregrine

Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

[Robert Pluim]

>>>>> On Wed, 10 Jul 2024 10:46:57 +0000, Philip Kaludercic <philipk@posteo.net> said:

    Philip> Isn't that solved by adding a :greedy (see (widget) checklist).
    Philip> Arguably this should be enabled by default for `set' types.

That doesnʼt help. Hereʼs what the current type gets you:

    Hide Shr Image Zoom Levels:
    Set:
    [X] Fit to window size
    [X] Original size
    [ ] Full image size
    [X] Fill window height

All you can do is enable or disable each of the four entries. With my
suggestion, you get:

    Hide Shr Image Zoom Levels:
    Repeat:
    [INS] [DEL] Choice:
                (*) Fit to window size
                ( ) Original size
                ( ) Full image size
                ( ) Fill window height
    [INS] [DEL] Choice:
                ( ) Fit to window size
                (*) Original size
                ( ) Full image size
                ( ) Fill window height
    [INS] [DEL] Choice:
                ( ) Fit to window size
                ( ) Original size
                ( ) Full image size
                (*) Fill window height
    [INS]

So you can change the type of each entry, and add/delete entries.

If you do "(repeat (choice" you get this:

    Hide Shr Image Zoom Levels:
    Repeat:
    [INS] [DEL] Choice: [Value Menu] Fit to window size
    [INS] [DEL] Choice: [Value Menu] Original size
    [INS] [DEL] Choice: [Value Menu] Fill window height
    [INS]

which offers the same possibilities, but I prefer the 'radio' version.

Robert
-- 

Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

[Jim Porter]

On 7/10/2024 2:04 AM, Robert Pluim wrote:
> The type agrees with the value, but not the documentation:
> 
>      Each element
>      can be one of the following symbols:
[snip]
> The current type lets you choose to enable each option, but not to
> reorder them without resorting to lisp code.

Ultimately, I'm more interested in ensuring the list has no duplicate 
elements than in allowing reordering (the code would behave strangely 
with duplicate elements), but if we got both that would certainly be 
nice. If we had to choose one or the other though, I think the current 
type (thanks Mattias!) is the way to go.

Re: master ac797f60160 2/2: ; * lisp/net/shr.el (shr-image-zoom-levels): Fix wrong type.

[Robert Pluim]

>>>>> On Wed, 10 Jul 2024 10:24:10 -0700, Jim Porter <jporterbugs@gmail.com> said:

    Jim> On 7/10/2024 2:04 AM, Robert Pluim wrote:
    >> The type agrees with the value, but not the documentation:
    >> Each element
    >> can be one of the following symbols:
    Jim> [snip]
    >> The current type lets you choose to enable each option, but not to
    >> reorder them without resorting to lisp code.

    Jim> Ultimately, I'm more interested in ensuring the list has no duplicate
    Jim> elements than in allowing reordering (the code would behave strangely
    Jim> with duplicate elements), but if we got both that would certainly be
    Jim> nice. If we had to choose one or the other though, I think the current
    Jim> type (thanks Mattias!) is the way to go.

The current type is `set', which is supposed to be orderless. If we
change it to `repeat' we can always check for duplicates in the
variableʼs `:set' function.

Robert
--