bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

Hi Morten, maintainers,

Recently, in https://github.com/joaotavora/eglot/issues/990, we found
out that users of both eglot.el and desktop.el were getting errors when
restarting Emacs.  That's because desktop.el attempts to re-enable the
eglot--managed-mode minor mode which was "on" when they saved the
session (presumably on exit).

I confirmed this with a stack trace requested from the user and came up
with this workaround in the user's config:

    (add-to-list 'desktop-minor-mode-handlers
                 '(eglot--managed-mode . ignore))

This works, but we should come up with something better.

In Eglot, the eglot--managed-mode minor mode is an implementation
detail, it is NOT meant to be called by the user, since it requires a
number of preconditions (like firing up a successful server) to be met.

Therefore, I have named the symbol with the "internal symbol"
convention.  In this very simple patch, I teach desktop.el to watch out
for this convention and not restart that mode.

diff --git a/lisp/desktop.el b/lisp/desktop.el
index 1a4103e209..a93703a77e 100644
--- a/lisp/desktop.el
+++ b/lisp/desktop.el
@@ -1617,7 +1617,9 @@ desktop-create-buffer
 		   (let ((handler (cdr (assq minor-mode desktop-minor-mode-handlers))))
 		     (if handler
 			 (funcall handler desktop-buffer-locals)
-		       (when (functionp minor-mode)
+		       (when (or (functionp minor-mode)
+                                 (and (symbolp minor-mode)
+                                      (not (string-match "^[^-]+--" (symbol-name minor-mode)))))
 			 (funcall minor-mode 1)))))))
 	  ;; Even though point and mark are non-nil when written by
 	  ;; `desktop-save', they may be modified by handlers wanting to set


This probably works (though I haven't tested), but maybe we could come
up with some other way around this, like having eglot.el propertize its
'eglot--managed-mode' symbol so that desktop.el doesn't even write it
onto the user's save file.  Or something like that.

Thanks,
João

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Eli Zaretskii]

> Cc: Morten Welinder <terra@diku.dk>
> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 05 Jul 2022 20:34:16 +0100
> 
> Hi Morten, maintainers,
> 
> Recently, in https://github.com/joaotavora/eglot/issues/990, we found
> out that users of both eglot.el and desktop.el were getting errors when
> restarting Emacs.  That's because desktop.el attempts to re-enable the
> eglot--managed-mode minor mode which was "on" when they saved the
> session (presumably on exit).
> 
> I confirmed this with a stack trace requested from the user and came up
> with this workaround in the user's config:
> 
>     (add-to-list 'desktop-minor-mode-handlers
>                  '(eglot--managed-mode . ignore))
> 
> This works, but we should come up with something better.

But why is the above not good enough?  You could also use
desktop-minor-mode-table, which is a defcustom.

IOW, desktop.el already has the machinery to not restore some modes,
and I see no need to make a general change like you suggest just
because the mode in your case happened to have a symbol which looks
like an internal one.  Your change also precludes anyone to have such
a mode restored, ever -- why?

> This probably works (though I haven't tested), but maybe we could come
> up with some other way around this, like having eglot.el propertize its
> 'eglot--managed-mode' symbol so that desktop.el doesn't even write it
> onto the user's save file.  Or something like that.

Why invent new machinery, when we already have more than enough to
handle this problem (and many similar ones)?

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> I confirmed this with a stack trace requested from the user and came up
>> with this workaround in the user's config:
>> 
>>     (add-to-list 'desktop-minor-mode-handlers
>>                  '(eglot--managed-mode . ignore))
>> 
>> This works, but we should come up with something better.
>
> But why is the above not good enough?  You could also use
> desktop-minor-mode-table, which is a defcustom.

At first I thought it was a customization variable and that would make
it user-specific overwritable etc.  But I see now that evidently it is
not.  According to the docstring of d-m-m-handlers, eglot.el could just
just contain the above invocation, indeed.

But that has the downside that eglot.el must require "desktop.el" which
IMO opinion too strongly couples the two packages.

I think I like Lars's solution best.

João

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: 56407@debbugs.gnu.org,  terra@diku.dk
> Date: Tue, 05 Jul 2022 20:53:33 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I confirmed this with a stack trace requested from the user and came up
> >> with this workaround in the user's config:
> >> 
> >>     (add-to-list 'desktop-minor-mode-handlers
> >>                  '(eglot--managed-mode . ignore))
> >> 
> >> This works, but we should come up with something better.
> >
> > But why is the above not good enough?  You could also use
> > desktop-minor-mode-table, which is a defcustom.
> 
> At first I thought it was a customization variable and that would make
> it user-specific overwritable etc.  But I see now that evidently it is
> not.  According to the docstring of d-m-m-handlers, eglot.el could just
> just contain the above invocation, indeed.
> 
> But that has the downside that eglot.el must require "desktop.el" which
> IMO opinion too strongly couples the two packages.

I see no reason to require: you just add a value to the list, that's
all.

> I think I like Lars's solution best.

I don't: it makes the information spread out and harder to find.

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

On Wed, Jul 6, 2022 at 3:29 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Cc: 56407@debbugs.gnu.org,  terra@diku.dk
> > Date: Tue, 05 Jul 2022 20:53:33 +0100
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > >> I confirmed this with a stack trace requested from the user and came
> up
> > >> with this workaround in the user's config:
> > >>
> > >>     (add-to-list 'desktop-minor-mode-handlers
> > >>                  '(eglot--managed-mode . ignore))
> > >>
> > >> This works, but we should come up with something better.
> > >
> > > But why is the above not good enough?  You could also use
> > > desktop-minor-mode-table, which is a defcustom.
> >
> > At first I thought it was a customization variable and that would make
> > it user-specific overwritable etc.  But I see now that evidently it is
> > not.  According to the docstring of d-m-m-handlers, eglot.el could just
> > just contain the above invocation, indeed.
> >
> > But that has the downside that eglot.el must require "desktop.el" which
> > IMO opinion too strongly couples the two packages.
>
> I see no reason to require: you just add a value to the list, that's
> all.
>

Oh, it's an autoloaded variable.  OK then, it'll work. It'll load in
desktop.el
though.

> I think I like Lars's solution best.
>
> I don't: it makes the information spread out and harder to find.
>

Depends on whether one thinks using the global symbol table in Elisp is
counts as "spread out". I don't.

There's a nice upside to it, which is it prevents people like me not
interested in desktop.el at all from having it autoloaded just by loading
 eglot.el.  The things eglot.el is trying to say to desktop.el is "stay out
of
my minor mode" so it is strange that it has to pull in desktop.el every time
just to say that.

João Távora

[-- Attachment #2: Type: text/html, Size: 2961 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Wed, 6 Jul 2022 09:12:39 +0100
> Cc: 56407@debbugs.gnu.org
> 
>  I see no reason to require: you just add a value to the list, that's
>  all.
> 
> Oh, it's an autoloaded variable.  OK then, it'll work. It'll load in desktop.el 
> though.

I feel there's some misunderstanding here.  What I meant is simply add
eglot--managed-mode to the default value of the variable in
desktop.el.  Why would that require loading desktop.el?

>  > I think I like Lars's solution best.
> 
>  I don't: it makes the information spread out and harder to find.
> 
> Depends on whether one thinks using the global symbol table in Elisp is
> counts as "spread out". I don't.

What do you mean by "global symbol table"?

What I meant is that having all the modes which desktop.el treats
specially in one place in desktop.el makes it easier to find out which
modes are those, than if each of the modes had something like
"(put foo-mode 'desktop...)" in its own file.  Because in the latter
case, if I want to know which modes are handled specially by desktop,
I'd need to search the entire tree.

> There's a nice upside to it, which is it prevents people like me not 
> interested in desktop.el at all from having it autoloaded just by loading
>  eglot.el.  The things eglot.el is trying to say to desktop.el is "stay out of
> my minor mode" so it is strange that it has to pull in desktop.el every time
> just to say that.

See above: I don't think I understand why would you need to load
desktop.el.  The variable desktop-minor-mode-table is of interest only
when the desktop is saved or restored, and at that time desktop.el is
already loaded, of course.  No other code anywhere else should need to
consult desktop-minor-mode-table.  Or what am I missing?

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

On Wed, Jul 6, 2022 at 12:09 PM Eli Zaretskii <eliz@gnu.org> wrote:


> > Oh, it's an autoloaded variable.  OK then, it'll work. It'll load in
> desktop.el
> > though.
>
> I feel there's some misunderstanding here.  What I meant is simply add
> eglot--managed-mode to the default value of the variable in
> desktop.el.  Why would that require loading desktop.el?
>

Indeed, I misunderstood. I thought you meant adding that to eglot.el.

But then I'd say it is even worse, as you're informing desktop.el
about an implementation detail of eglot.el.  If I change that minor
mode's name, then I have to change desktop.el as well.

>  > I think I like Lars's solution best.
> >  I don't: it makes the information spread out and harder to find.
> > Depends on whether one thinks using the global symbol table in Elisp is
> > counts as "spread out". I don't.
> What do you mean by "global symbol table"?
>

The obarray.

What I meant is that having all the modes which desktop.el treats
> specially in one place in desktop.el makes it easier to find out which
> modes are those, than if each of the modes had something like
> "(put foo-mode 'desktop...)" in its own file.  Because in the latter
> case, if I want to know which modes are handled specially by desktop,
> I'd need to search the entire tree.
>

mapatoms is used all the time, it's fast and it can answer that.

But typically I think, the question would be: "Why isn't this mode X being
handled as I expect it to?", and then the answer would be easy.  Except
that even that question is hard to conceive in this particular case: why
would
someone be concerned about `eglot--managed-mode`, if it's an
implementation detail?

I think we use symbol properties very often and to good effect.  For example
to describe the file-local safety of variables.


> > There's a nice upside to it, which is it prevents people like me not
> > interested in desktop.el at all from having it autoloaded just by loading
> >  eglot.el.  The things eglot.el is trying to say to desktop.el is "stay
> out of
> > my minor mode" so it is strange that it has to pull in desktop.el every
> time
> > just to say that.
>
> See above: I don't think I understand why would you need to load
> desktop.el.  The variable desktop-minor-mode-table is of interest only
> when the desktop is saved or restored, and at that time desktop.el is
> already loaded, of course.  No other code anywhere else should need to
> consult desktop-minor-mode-table.  Or what am I missing?
>

See above. I thought you meant putting the line into eglot.el which would
work but needs loading desktop.el. Conversely, putting the eglot-specific
line
in desktop.el is putting eglot.el implementation details outside eglot.el,
which
is bad.

So, either way, using the desktop-minor-mode-table for this is a poor
choice,
which logically means that the information should be stored in the symbol,
which exists in the global symbol table (the obarray).

Interestingly, a hook variable doesn't have this drawback, btw.  In fact,
they seem to have been designed to avoid this class of problems. But
d-m-m-table is not a hook variable.

João Távora

[-- Attachment #2: Type: text/html, Size: 4603 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> I think we use symbol properties very often and to good effect.  For example
> to describe the file-local safety of variables.

Yes, I think they're ideal for this sort of thing -- loosely coupled
interaction between two packages.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Wed, 6 Jul 2022 12:30:28 +0100
> Cc: 56407@debbugs.gnu.org
> 
>  I feel there's some misunderstanding here.  What I meant is simply add
>  eglot--managed-mode to the default value of the variable in
>  desktop.el.  Why would that require loading desktop.el?
> 
> Indeed, I misunderstood. I thought you meant adding that to eglot.el.
> 
> But then I'd say it is even worse, as you're informing desktop.el 
> about an implementation detail of eglot.el.  If I change that minor 
> mode's name, then I have to change desktop.el as well.  

That's okay: it's desktop.el's job to know about some implementation
details.  Just look at how much it knows about what the various modes
and variables do in Emacs.

>  >  > I think I like Lars's solution best.
>  >  I don't: it makes the information spread out and harder to find.
>  > Depends on whether one thinks using the global symbol table in Elisp is
>  > counts as "spread out". I don't.
>  What do you mean by "global symbol table"?
> 
> The obarray. 

That's not a source-level feature, so it doesn't help maintenance.

>  What I meant is that having all the modes which desktop.el treats
>  specially in one place in desktop.el makes it easier to find out which
>  modes are those, than if each of the modes had something like
>  "(put foo-mode 'desktop...)" in its own file.  Because in the latter
>  case, if I want to know which modes are handled specially by desktop,
>  I'd need to search the entire tree.
> 
> mapatoms is used all the time, it's fast and it can answer that.

We are miscommunicating: I meant finding them without necessarily
running Emacs.  And mapatoms will only help if the corresponding
package was loaded, it won''t help me to find all the packages that
need something from desktop.el.

> I think we use symbol properties very often and to good effect.  For example
> to describe the file-local safety of variables.

Yes, and try finding, for example, all the possible uses of the
'delete-selection' property some day, for the purposes of documenting
what can be done with each value of the property.

> Conversely, putting the eglot-specific line
> in desktop.el is putting eglot.el implementation details outside eglot.el, which
> is bad.

Not in my book, it isn't.  desktop.el is by definition full of details
about different Emacs features, and it is very nice to have them all
in one place.

> So, either way, using the desktop-minor-mode-table for this is a poor choice, 
> which logically means that the information should be stored in the symbol, 
> which exists in the global symbol table (the obarray).

As someone who needs to look in desktop.el for similar details very
frequently, I disagree, sorry.

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

>
> > But then I'd say it is even worse, as you're informing desktop.el
> > about an implementation detail of eglot.el.  If I change that minor
> > mode's name, then I have to change desktop.el as well.
>
> That's okay: it's desktop.el's job to know about some implementation
> details.  Just look at how much it knows about what the various modes
> and variables do in Emacs.


Wait, you're saying it's "okay" to have to do a commit to Emacs's repo
everytime someone makes a third-party package that has a minor mode
that needs special handling?  Or everytime someone changes the name
or shape of a minor mode?

I can't possibly see how this is okay. There should simply be a generic
mechanism for minor-modes to tell desktop.el and other intrusive
packages to "stay out of my minor mode".

But we do have that mechanism. It's called symbol properties and it's a nice
feature of lisp. So let's use it, please. All the other solutions are
demonstrably
worse.  I'm not even saying to get rid of d-m-m-table, as Lars is.  I'm
just
saying: let's not type "name-of-eglot-symbol-that-joao-may-want-to-change"
into desktop.el.  It's really a hazard.

João

[-- Attachment #2: Type: text/html, Size: 1700 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Wed, 6 Jul 2022 13:59:51 +0100
> Cc: 56407@debbugs.gnu.org
> 
>  That's okay: it's desktop.el's job to know about some implementation
>  details.  Just look at how much it knows about what the various modes
>  and variables do in Emacs.
> 
> Wait, you're saying it's "okay" to have to do a commit to Emacs's repo 
> everytime someone makes a third-party package that has a minor mode 
> that needs special handling?  Or everytime someone changes the name
> or shape of a minor mode?

eglot is not a third-party package.  We intended to add it to core, I
think?

But if what I suggest isn't to your liking, you can always tell users
to customize desktop-minor-mode-table by themselves.  Or do what you
didn't want to do: cause desktop.el to be autoloaded by eglot.

> But we do have that mechanism. It's called symbol properties and it's a nice
> feature of lisp. So let's use it, please.

If you insist.  But then don't come back crying when this is broken by
some change in desktop.el that the "loosely coupled packages" didn't
pick up.

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

On Wed, Jul 6, 2022 at 2:12 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Date: Wed, 6 Jul 2022 13:59:51 +0100
> > Cc: 56407@debbugs.gnu.org
> >
> >  That's okay: it's desktop.el's job to know about some implementation
> >  details.  Just look at how much it knows about what the various modes
> >  and variables do in Emacs.
> >
> > Wait, you're saying it's "okay" to have to do a commit to Emacs's repo
> > everytime someone makes a third-party package that has a minor mode
> > that needs special handling?  Or everytime someone changes the name
> > or shape of a minor mode?
>
> eglot is not a third-party package.  We intended to add it to core, I
> think?
>

Indeed we do.  But it's not yet, because I've been so very busy.

 But if what I suggest isn't to your liking, you can always tell users

> to customize desktop-minor-mode-table by themselves.  Or do what you
> didn't want to do: cause desktop.el to be autoloaded by eglot.
>

Yes, those are alternatives. But not as good.


> > But we do have that mechanism. It's called symbol properties and it's a
> nice
> > feature of lisp. So let's use it, please.
>
> If you insist.  But then don't come back crying when this is broken by
> some change in desktop.el that the "loosely coupled packages" didn't
> pick up.
>

Thanks. I can't vouch for my future emotions, but I also can't see how
this mechanism, which I've used and seen used so often, could fail
in such ways.

I'll prepare a patch.

João

[-- Attachment #2: Type: text/html, Size: 2601 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 2070 bytes --]

Hmm,

I've just remembered there's yet another alternative which is to use

  (eval-after-load 'desktop
   ...)

 in eglot.el. This wouldn't have the drawback of "load desktop.el
inadvertently".
Then again, eval-after-load shouldn't be used in packages, so I've read
somewhere
(manual?).

Just mentioning this for completeness sake.

João

On Wed, Jul 6, 2022 at 2:19 PM João Távora <joaotavora@gmail.com> wrote:

> On Wed, Jul 6, 2022 at 2:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > From: João Távora <joaotavora@gmail.com>
>> > Date: Wed, 6 Jul 2022 13:59:51 +0100
>> > Cc: 56407@debbugs.gnu.org
>> >
>> >  That's okay: it's desktop.el's job to know about some implementation
>> >  details.  Just look at how much it knows about what the various modes
>> >  and variables do in Emacs.
>> >
>> > Wait, you're saying it's "okay" to have to do a commit to Emacs's repo
>> > everytime someone makes a third-party package that has a minor mode
>> > that needs special handling?  Or everytime someone changes the name
>> > or shape of a minor mode?
>>
>> eglot is not a third-party package.  We intended to add it to core, I
>> think?
>>
>
> Indeed we do.  But it's not yet, because I've been so very busy.
>
>  But if what I suggest isn't to your liking, you can always tell users
>
>> to customize desktop-minor-mode-table by themselves.  Or do what you
>> didn't want to do: cause desktop.el to be autoloaded by eglot.
>>
>
> Yes, those are alternatives. But not as good.
>
>
>> > But we do have that mechanism. It's called symbol properties and it's a
>> nice
>> > feature of lisp. So let's use it, please.
>>
>> If you insist.  But then don't come back crying when this is broken by
>> some change in desktop.el that the "loosely coupled packages" didn't
>> pick up.
>>
>
> Thanks. I can't vouch for my future emotions, but I also can't see how
> this mechanism, which I've used and seen used so often, could fail
> in such ways.
>
> I'll prepare a patch.
>
> João
>


-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 3610 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Stefan Kangas]

João Távora <joaotavora@gmail.com> writes:

>   (eval-after-load 'desktop
>    ...)

`with-eval-after-load' is nicer, IMO.

> Then again, eval-after-load shouldn't be used in packages, so I've read somewhere
> (manual?).

See the last paragraph in (info "(elisp) Hooks for Loading")

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

On Wed, Jul 6, 2022 at 2:40 PM Stefan Kangas <stefan@marxist.se> wrote:

> >   (eval-after-load 'desktop
> >    ...)
>
> `with-eval-after-load' is nicer, IMO.
>
> > Then again, eval-after-load shouldn't be used in packages, so I've read
> somewhere
> > (manual?).
>
> See the last paragraph in (info "(elisp) Hooks for Loading")
>

Don't hold me in suspense! :-) what does it say vis-a-vis use in libraries?

Maybe we should just use it. If it works, it's the easiest one and not
terribly
ugly.

Why is it the easiest one?  Because I've just remembered an obvious
drawback of the symbol-property approach: it will only work on Emacs trunk,
_unless_ we make the little dance (that I've done a few times) of making
desktop.el a GNU ELPA :core  package and making eglot.el depend on
that package (which is different from _requiring_ the load of that package).

Lars, what do you think?

João

[-- Attachment #2: Type: text/html, Size: 1481 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Stefan Kangas]

João Távora <joaotavora@gmail.com> writes:

>> See the last paragraph in (info "(elisp) Hooks for Loading")
>
> Don't hold me in suspense! :-) what does it say vis-a-vis use in libraries?

   Normally, well-designed Lisp programs should not use
‘with-eval-after-load’.  If you need to examine and set the variables
defined in another library (those meant for outside use), you can do it
immediately—there is no need to wait until the library is loaded.  If
you need to call functions defined by that library, you should load the
library, preferably with ‘require’ (*note Named Features::).

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> Why is it the easiest one?  Because I've just remembered an obvious 
> drawback of the symbol-property approach: it will only work on Emacs trunk, 
> _unless_ we make the little dance (that I've done a few times) of making 
> desktop.el a GNU ELPA :core  package and making eglot.el depend on 
> that package (which is different from _requiring_ the load of that package).
>
> Lars, what do you think?

Hm, I'd forgotten that eglot was also an ELPA package.

Making desktop a :core package is fine by me, but you're right that it'd
be easier to use with-eval-after-load here.  (I'm still in favour of
adding the symbol-plist thing for desktop as a more long term solution,
but we can do both.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

On Wed, Jul 6, 2022 at 2:53 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > Why is it the easiest one?  Because I've just remembered an obvious
> > drawback of the symbol-property approach: it will only work on Emacs
> trunk,
> > _unless_ we make the little dance (that I've done a few times) of making
> > desktop.el a GNU ELPA :core  package and making eglot.el depend on
> > that package (which is different from _requiring_ the load of that
> package).
> >
> > Lars, what do you think?
>
> Hm, I'd forgotten that eglot was also an ELPA package.
>
> Making desktop a :core package is fine by me, but you're right that it'd
> be easier to use with-eval-after-load here.  (I'm still in favour of
> adding the symbol-plist thing for desktop as a more long term solution,
> but we can do both.)
>

Yeah, the problem is if desktop.el uses recently added features of
some _other_ library.  Then I'd have to add that one to :core as well,
or somehow clarify the supported uses of desktop.el in that context.

I don't have time for that, so I think I'll go with `with-eval-after-load`
in
eglot.el and call it a day.

João

[-- Attachment #2: Type: text/html, Size: 1764 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>  Your change also precludes anyone to have such
> a mode restored, ever -- why?

I'm sorry, I realize I didn't answer this part of your email earlier.

eglot--managed-mode is a special minor mode: although the function is
created by define-minor-mode, it is not meant to be turned interactively
for example.  It needs certain preconditions to be true before turning
it on and off.  If this was all there it could perhaps be arranged with
some of those special function, but a very important detail here is that
these preconditions cannot be on per-buffer.  The most important of them
regards a connection to an LSP server which has a view over _all_
buffers in a certain major mode within a certain project.  Eglot has
machinery to carefully manage this, and I'm not sure it is easy or wise
to transfer or invoke that machinery in desktop.el's mode-restoring
functions.

Also, eglot--managed-mode is really an implementation detail that is
subject to change.  Not only theoretically, but practically, too, as I
ponder the ability to have more than one server active for any given
buffer at a time.

João

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: 56407@debbugs.gnu.org
> Date: Tue, 05 Jul 2022 23:52:35 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >  Your change also precludes anyone to have such
> > a mode restored, ever -- why?
> 
> I'm sorry, I realize I didn't answer this part of your email earlier.
> 
> eglot--managed-mode is a special minor mode:

I meant _any_ mode that has "--" in its symbol, not just
eglot--managed-mode.  I don't want to preclude restoration of any such
modes, from now to eternity, it's too heavy a promise to make.

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On Wed, Jul 6, 2022 at 3:34 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Cc: 56407@debbugs.gnu.org
> > Date: Tue, 05 Jul 2022 23:52:35 +0100
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > >  Your change also precludes anyone to have such
> > > a mode restored, ever -- why?
> >
> > I'm sorry, I realize I didn't answer this part of your email earlier.
> >
> > eglot--managed-mode is a special minor mode:
>
> I meant _any_ mode that has "--" in its symbol, not just
> eglot--managed-mode.  I don't want to preclude restoration of any such
> modes, from now to eternity, it's too heavy a promise to make.
>

I think there are no promises made on those modes anyway,
because they are implementation details, by their authors'
definition.

But I agree it's not the prettiest kludge.

João

[-- Attachment #2: Type: text/html, Size: 1522 bytes --]

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> This probably works (though I haven't tested), but maybe we could come
> up with some other way around this, like having eglot.el propertize its
> 'eglot--managed-mode' symbol so that desktop.el doesn't even write it
> onto the user's save file.  Or something like that.

Yes, I think it'd be better to add a mechanism like

(put 'eglot--managed-mode 'desktop-inhibit t)

or the like, and get rid of

(defcustom desktop-minor-mode-table
  '((defining-kbd-macro nil)
    (isearch-mode nil)
    (vc-mode nil)
...

which is used for the same thing.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#56407: 29.0.50; desktop.el shouldn't be saving/restoring eglot--managed-mode, which is not for interactive use

[João Távora]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yes, I think it'd be better to add a mechanism like
>
> (put 'eglot--managed-mode 'desktop-inhibit t)

Yes, I think I like this solution best.  Some desktop.el-agnostic name
that conveys "implementation-detail" would be even better, IMO.

João