Re: [Emacs-diffs] master 0245cc3: Improve accessibility of window dividers. (Bug#20183)

Re: [Emacs-diffs] master 0245cc3: Improve accessibility of window dividers. (Bug#20183)

[Stefan Monnier]

> +(defgroup window-divider nil
[...]
> +(defcustom window-divider-mode nil
[..]
> +  :group 'window-divider

This :group is redundant since it refers to the last created group.

> +(define-minor-mode window-divider-mode
[...]
> +  :group 'window-divider
> +  :global t
> +  :variable (window-divider-mode
> +             . (lambda (value)
> +                 (frame--window-divider-mode-set-and-apply
> +                  (and value
> +                       (or frame--window-divider-previous-mode
> +                           (default-value 'window-divider-mode)
> +                           'right-only))))))

I'd rather not abuse the (GETTER . SETTER) here.  This is only really
needed for those cases where the value of the mode is neither kept in
a global var nor a buffer-local var (e.g. it's kept in a window or frame
parameter instead).

So, we should drop :variable, which will also let us drop the
separate defcustom.


        Stefan

Re: [Emacs-diffs] master 0245cc3: Improve accessibility of window dividers. (Bug#20183)

[martin rudalics]

 >> +(defgroup window-divider nil
 > [...]
 >> +(defcustom window-divider-mode nil
 > [..]
 >> +  :group 'window-divider
 >
 > This :group is redundant since it refers to the last created group.

Interesting.  Is this documented somewhere?

 >> +(define-minor-mode window-divider-mode
 > [...]
 >> +  :group 'window-divider
 >> +  :global t
 >> +  :variable (window-divider-mode
 >> +             . (lambda (value)
 >> +                 (frame--window-divider-mode-set-and-apply
 >> +                  (and value
 >> +                       (or frame--window-divider-previous-mode
 >> +                           (default-value 'window-divider-mode)
 >> +                           'right-only))))))
 >
 > I'd rather not abuse the (GETTER . SETTER) here.  This is only really
 > needed for those cases where the value of the mode is neither kept in
 > a global var nor a buffer-local var (e.g. it's kept in a window or frame
 > parameter instead).
 >
 > So, we should drop :variable, which will also let us drop the
 > separate defcustom.

You mean the defcustom for `window-divider-mode'?  Then I completely
miss you.  Where would I specify the permitted values?

martin

Re: [Emacs-diffs] master 0245cc3: Improve accessibility of window dividers. (Bug#20183)

[Stefan Monnier]

> You mean the defcustom for `window-divider-mode'?  Then I completely
> miss you.  Where would I specify the permitted values?

The minor mode only allows two values (enabled/disabled).
So you'll need a second variable specifying what the "enabled" state
should look like.


        Stefan

Re: [Emacs-diffs] master 0245cc3: Improve accessibility of window dividers. (Bug#20183)

[martin rudalics]

 >> You mean the defcustom for `window-divider-mode'?  Then I completely
 >> miss you.  Where would I specify the permitted values?
 >
 > The minor mode only allows two values (enabled/disabled).
 > So you'll need a second variable specifying what the "enabled" state
 > should look like.

But how would a user customize switching from the `right-only' to the
`bottom-only' state?  How would a user specify the preferred initial
state?

I initially wanted to add just a simple menu entry for switching.  For
that I wanted an option which allowed to select one of four permissible
states.  Obviously, that option would have to be customizable via the
`customize-variable' interface as well.

Then I noticed that all items in the Show/Hide group are minor modes.
Some of them in a contorted sense - both `fringe-mode' and
`mouse-avoidance-mode' are simple defuns.  I would have preferred to do
it their style, possibly using completing read.  But I tried to be a
good citizen and specified `window-divider-mode' in a sense similar to
`scroll-bar-mode' - the only mode I found that was specified via
`define-minor-mode' and allowed more than two values.

Now if I'm not mistaken you seem to say that the approach I've chosen is
not TRT.  But how can I know what TRT is?  Neither the various modes one
can activate via the menu bar nor the manual provide useful guidance in
this regard :-(

martin

Re: [Emacs-diffs] master 0245cc3: Improve accessibility of window dividers. (Bug#20183)

[Stefan Monnier]

> Then I noticed that all items in the Show/Hide group are minor modes.
> Some of them in a contorted sense - both `fringe-mode' and
> `mouse-avoidance-mode' are simple defuns.  I would have preferred to do
> it their style, possibly using completing read.  But I tried to be a
> good citizen and specified `window-divider-mode' in a sense similar to
> `scroll-bar-mode' - the only mode I found that was specified via
> `define-minor-mode' and allowed more than two values.

Indeed, we have problems in there.  Minor modes are designed to be
booleans only.  For after-the-fact situations, we sometimes try to find
a contorted way to consider the feature as a minor mode, even tho it
doesn't quite fit.  But when defining a new feature I think it's
worthwhile to try and design the feature such that it fits.

> But how would a user customize switching from the `right-only' to the
> `bottom-only' state?  How would a user specify the preferred initial
> state?

With the second variable (e.g. could be called `window-divider-places'),
rather than the minor mode variable.


        Stefan

Re: [Emacs-diffs] master 0245cc3: Improve accessibility of window dividers. (Bug#20183)

[martin rudalics]

> Indeed, we have problems in there.  Minor modes are designed to be
> booleans only.  For after-the-fact situations, we sometimes try to find
> a contorted way to consider the feature as a minor mode, even tho it
> doesn't quite fit.  But when defining a new feature I think it's
> worthwhile to try and design the feature such that it fits.

I see.

>> But how would a user customize switching from the `right-only' to the
>> `bottom-only' state?  How would a user specify the preferred initial
>> state?
>
> With the second variable (e.g. could be called `window-divider-places'),
> rather than the minor mode variable.

Done, hopefully.

Thanks, martin