bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

[Daniel Mendler]

rcirc-mode started to set `completion-cycle-threshold' to t. This
setting is intrusive and interferes with user completion setups. It is
better left to the user to tweak this setting in their user
configuration, since users may want to configure completions coherently
across the different modes in Emacs. Note that rcirc.el is the only
file/mode which sets `completion-cycle-threshold'. Therefore it is
better to revert the change which added this setting.

In GNU Emacs 28.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5,
cairo version 1.16.0)
 of 2022-02-09 built on projects
Repository revision: 82e74e4559b8becd44f3e7ac0134e2baddd69921
Repository branch: emacs-28
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

[Philip Kaludercic]

Daniel Mendler <mail@daniel-mendler.de> writes:

> rcirc-mode started to set `completion-cycle-threshold' to t. This
> setting is intrusive and interferes with user completion setups. It is
> better left to the user to tweak this setting in their user
> configuration, since users may want to configure completions coherently
> across the different modes in Emacs. Note that rcirc.el is the only
> file/mode which sets `completion-cycle-threshold'. Therefore it is
> better to revert the change which added this setting.

The change that introduced this was that the custom cycling
implementation was replaced with one based on completion-at-point
(0b367ec), mainly to simplify the code.  The reason
`completion-cycle-threshold' is set is to preserve the appearance of the
previous behaviour, as a complete reversion to completion-at-point
seemed like too much of a change.

I could imagine introducing a user option to decide what you want to
use.  My inclination would be to set it to "cycle by default", but it
doesn't need that way.  Perhaps we could test non-cycling (regular capf)
for a while, and see if there are any complaints or other feedback?

> In GNU Emacs 28.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5,
> cairo version 1.16.0)
>  of 2022-02-09 built on projects
> Repository revision: 82e74e4559b8becd44f3e7ac0134e2baddd69921
> Repository branch: emacs-28
> Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
> System Description: Debian GNU/Linux 10 (buster)

-- 
	Philip Kaludercic

bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

[Daniel Mendler]

> The change that introduced this was that the custom cycling
> implementation was replaced with one based on completion-at-point
> (0b367ec), mainly to simplify the code.  The reason
> `completion-cycle-threshold' is set is to preserve the appearance of the
> previous behaviour, as a complete reversion to completion-at-point
> seemed like too much of a change.

I agree with your change to remove rcirc-complete. I greatly appreciate
the simplicity of rcirc and it is good that you try to make it even
simpler! But rcirc-completion-at-point was already present as Capf on
Emacs 27 and there `completion-at-point` didn't lead to cycling. I would
not introduce a user configuration, it is easy enough to set
completion-cycle-threshold in a hook.

Furthermore there is the alternative to use completion-category-defaults
to specify the threshold per completion category. You could add the
override there. But personally I would  avoid doing that. I usually
prefer if packages avoid intrusive and opinionated settings and instead
try to ensure consistency across Emacs.

> I could imagine introducing a user option to decide what you want to
> use.  My inclination would be to set it to "cycle by default", but it
> doesn't need that way.  Perhaps we could test non-cycling (regular capf)
> for a while, and see if there are any complaints or other feedback?

Yes, I would go with normal Capf behavior, which is the usual behavior
across all of Emacs. If a user wants to restore rcirc-complete, it seems
easy enough to add this to a user configuration?

(defun rcirc-complete ()
  (interactive)
  (let ((completion-cycle-threshold t))
    (completion-at-point)))

Thanks!

bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

[Philip Kaludercic]

Daniel Mendler <mail@daniel-mendler.de> writes:

>> The change that introduced this was that the custom cycling
>> implementation was replaced with one based on completion-at-point
>> (0b367ec), mainly to simplify the code.  The reason
>> `completion-cycle-threshold' is set is to preserve the appearance of the
>> previous behaviour, as a complete reversion to completion-at-point
>> seemed like too much of a change.
>
> I agree with your change to remove rcirc-complete. I greatly appreciate
> the simplicity of rcirc and it is good that you try to make it even
> simpler! But rcirc-completion-at-point was already present as Capf on
> Emacs 27 and there `completion-at-point` didn't lead to cycling. I would
> not introduce a user configuration, it is easy enough to set
> completion-cycle-threshold in a hook.

Hmm, ok, I get your point.  My rationale is that rcirc-complete (bound
to TAB) used to cycle, and that it was that behaviour that was supposed
to be preserved.

> Furthermore there is the alternative to use completion-category-defaults
> to specify the threshold per completion category. You could add the
> override there. But personally I would  avoid doing that. I usually
> prefer if packages avoid intrusive and opinionated settings and instead
> try to ensure consistency across Emacs.

I agree.

>> I could imagine introducing a user option to decide what you want to
>> use.  My inclination would be to set it to "cycle by default", but it
>> doesn't need that way.  Perhaps we could test non-cycling (regular capf)
>> for a while, and see if there are any complaints or other feedback?
>
> Yes, I would go with normal Capf behavior, which is the usual behavior
> across all of Emacs. If a user wants to restore rcirc-complete, it seems
> easy enough to add this to a user configuration?
>
> (defun rcirc-complete ()
>   (interactive)
>   (let ((completion-cycle-threshold t))
>     (completion-at-point)))

Or, this could be added as rcirc-complete, binding TAB back to this new
command and documenting that if you want to complete using the default
CAPF, you should rebind it to completion-at-point?

> Thanks!

-- 
	Philip Kaludercic

bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

[Daniel Mendler]

>>> I could imagine introducing a user option to decide what you want to
>>> use.  My inclination would be to set it to "cycle by default", but it
>>> doesn't need that way.  Perhaps we could test non-cycling (regular capf)
>>> for a while, and see if there are any complaints or other feedback?
>>
>> Yes, I would go with normal Capf behavior, which is the usual behavior
>> across all of Emacs. If a user wants to restore rcirc-complete, it seems
>> easy enough to add this to a user configuration?
>>
>> (defun rcirc-complete ()
>>   (interactive)
>>   (let ((completion-cycle-threshold t))
>>     (completion-at-point)))
> 
> Or, this could be added as rcirc-complete, binding TAB back to this new
> command and documenting that if you want to complete using the default
> CAPF, you should rebind it to completion-at-point?

That's an option too. But then you will still keep the old cruft around.
I would rather break backward compatibility here and document that
binding `completion-cycle-threshold=t` will restore the old behavior.
Most users will appreciate that a Capf is directly available, which is
more capable than `rcirc-complete`!

Daniel

bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

[Philip Kaludercic]

Daniel Mendler <mail@daniel-mendler.de> writes:

>>>> I could imagine introducing a user option to decide what you want to
>>>> use.  My inclination would be to set it to "cycle by default", but it
>>>> doesn't need that way.  Perhaps we could test non-cycling (regular capf)
>>>> for a while, and see if there are any complaints or other feedback?
>>>
>>> Yes, I would go with normal Capf behavior, which is the usual behavior
>>> across all of Emacs. If a user wants to restore rcirc-complete, it seems
>>> easy enough to add this to a user configuration?
>>>
>>> (defun rcirc-complete ()
>>>   (interactive)
>>>   (let ((completion-cycle-threshold t))
>>>     (completion-at-point)))
>> 
>> Or, this could be added as rcirc-complete, binding TAB back to this new
>> command and documenting that if you want to complete using the default
>> CAPF, you should rebind it to completion-at-point?
>
> That's an option too. But then you will still keep the old cruft around.
> I would rather break backward compatibility here and document that
> binding `completion-cycle-threshold=t` will restore the old behavior.
> Most users will appreciate that a Capf is directly available, which is
> more capable than `rcirc-complete`!

I honestly don't know.  I understand both sides, but cannot decide.  One
also has to consider the current stage of Emacs 28 development, and that
these kinds of changes aren't appreciated right now.  That means
disabling completion-cycle-threshold would have to be installed in 29.1.

-- 
	Philip Kaludercic

bug#53897: 28.0.91; regression: rcirc overwrites completion-cycle-threshold

[Philip Kaludercic]

This issue should have been resolved by fdd8b591 and the addition of
rcirc-cycle-completion-flag (disabled by default).

Daniel Mendler <mail@daniel-mendler.de> writes:

> rcirc-mode started to set `completion-cycle-threshold' to t. This
> setting is intrusive and interferes with user completion setups. It is
> better left to the user to tweak this setting in their user
> configuration, since users may want to configure completions coherently
> across the different modes in Emacs. Note that rcirc.el is the only
> file/mode which sets `completion-cycle-threshold'. Therefore it is
> better to revert the change which added this setting.
>
> In GNU Emacs 28.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5,
> cairo version 1.16.0)
>  of 2022-02-09 built on projects
> Repository revision: 82e74e4559b8becd44f3e7ac0134e2baddd69921
> Repository branch: emacs-28
> Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
> System Description: Debian GNU/Linux 10 (buster)

-- 
	Philip Kaludercic