evil-collection icon indicating copy to clipboard operation
evil-collection copied to clipboard

mu4e: Fix usages of internal functions and variables

Open hokomo opened this issue 1 year ago • 6 comments

evil-collection-mu4e.el is a little broken since part of the code relies on compatibility wrappers when accessing mu4e's internal functions and variables, and part of the code just accesses them directly. This leads to errors when it tries to override the Maildirs section in mu4e's main buffer.

I've introduced 2 new macros, evil-collection-mu4e-wrap-internal-function and evil-collection-mu4e-wrap-internal-variable, and fixed the issue by defining compatibility wrappers for all of the used internals.

I've kept the declare-function forms for the old internals. For some reason the file byte-compiles cleanly even without such forms for the new internals (although I didn't test it too thoroughly).

hokomo avatar Aug 23 '22 15:08 hokomo

Hmm, after doing some more testing, it seems that the code works perfectly fine with the latest stable release 1.8.9 of mu and mu4e (that's the version that introduced some of the internal functions that evil-collection-mu4e.el relies on, such as mu4e--maildirs-with-query), while I was using the older 1.6.6 version (my package manager's version).

This PR therefore makes part of the code work for 1.6.x again, but after a while I again ran into some issues. E.g. mu4e-search-bookmarks & co. have only been introduced in 1.8.x. and again cause problems with 1.6.x.

In general, how does evil-collection keep up with the different versions of packages and any breaking changes? Does it always only support the latest stable version of a package? Or does it try its best to provide some backwards compatibility, but offers no strong guarantees?

If it's only the latest stable release that's officially supported, perhaps this PR is not so useful and could be closed. Comments welcome. :-)

hokomo avatar Aug 23 '22 21:08 hokomo

In general, how does evil-collection keep up with the different versions of packages and any breaking changes? Does it always only support the latest stable version of a package? Or does it try its best to provide some backwards compatibility, but offers no strong guarantees?

evil-collection tries to be backward compatible if it's not so hard.

condy0919 avatar Aug 24 '22 18:08 condy0919

evil-collection tries to be backward compatible if it's not so hard.

In that case, I guess the patch is somewhat useful for people who are still using versions 1.6.x.

hokomo avatar Aug 24 '22 18:08 hokomo

When would we get rid of this compatibility layer?

jojojames avatar Aug 24 '22 18:08 jojojames

I am still using mu 1.6.6. The recent updates to evil-collection have made the user experience using mu4e much worse for me.

@hokomo For mu4e-search-bookmarks and friends, this sort of local mapping in my init.el "fixed" the issue.

(define-obsolete-function-alias 'mu4e-search-bookmark 'mu4e-headers-search-bookmark nil)
(define-obsolete-function-alias 'mu4e-search 'mu4e-headers-search nil))
(define-obsolete-function-alias 'mu4e-search-narrow 'mu4e-headers-search-narrow nil))

Perhaps we can hardcode resolution to those symbols for these 3 specific cases?

@jojojames asked:

When would we get rid of this compatibility layer?

Would it be possible to give it a year or so after the mu 1.6.x branch stops getting updates? 1.6.11 was released a few months ago: https://github.com/djcb/mu/releases/tag/1.6.11

omajid avatar Aug 30 '22 22:08 omajid

@hokomo For mu4e-search-bookmarks and friends, this sort of local mapping in my init.el "fixed" the issue.

(define-obsolete-function-alias 'mu4e-search-bookmark 'mu4e-headers-search-bookmark nil)
(define-obsolete-function-alias 'mu4e-search 'mu4e-headers-search nil))
(define-obsolete-function-alias 'mu4e-search-narrow 'mu4e-headers-search-narrow nil))

Perhaps we can hardcode resolution to those symbols for these 3 specific cases?

The problem with that is that the mu4e-search* functions are now the current ones (since 1.7.x), and the previous mu4e-headers-search* are defined as obsolete, so that would break for anyone using versions 1.8.x. The current code has this:

(define-obsolete-function-alias 'mu4e-headers-search 'mu4e-search "1.7.0")
(define-obsolete-function-alias 'mu4e-headers-search-edit 'mu4e-search-edit "1.7.0")
...

That's why I was asking how evil-collection handles key bindings for more than one version of a package. Perhaps some version checks could be introduced? I don't know how the maintainers feel about doing that. :-) In any case, the solution should be general enough so that the other missing functionality that I mentioned above could be fixed as well.

I've switched to mu 1.8.x since then and everything works as expected, although I did discover a couple more outdated key bindings for which I'll open a separate PR soon (edit: see #675).

hokomo avatar Aug 31 '22 07:08 hokomo

That's why I was asking how evil-collection handles key bindings for more than one version of a package. Perhaps some version checks could be introduced?

Usually we try to support the latest version of the package. If the compatibility is not too hard/easy, then we can leave some compatibility layer. I don't know too much about the specifics here though.

Would it be possible to give it a year or so after the mu 1.6.x branch stops getting updates? 1.6.11 was released a few months ago: https://github.com/djcb/mu/releases/tag/1.6.11

It's pretty hard to track like that for every package. I'll go to the middleground and go with the 'accept what contributors want to update the package to' approach. @omajid @hokomo Is this PR still useful for mu4e? If it is, I am happy to pull it in.

jojojames avatar Dec 17 '22 01:12 jojojames

Fedora 36 still seems to have an old enough version of mu4e, but Fedora 37 (which I am using these days) has mu4e 1.8.x, which is recent enough that this is no longer an issue for me.

omajid avatar Dec 18 '22 18:12 omajid

I'm assuming none of these are relevant anymore, monitoring the situation at https://github.com/emacs-evil/evil-collection/issues/695 .

jojojames avatar Mar 12 '23 01:03 jojojames