Purpose fixes do not uninstall cleanly when purpose-mode is off

[wyuenho]

All those advices and others in window-purpose-fixes.el are not removed when purpose-mode is turn off, so the lingering purpose customizations are still interfering all of these other packages.

For the advices, we can probably introduce a purpose-advice-add util that memoize the advices, and removed en-mass in purpose--remove-advices. The other package specific purpose configs should be relatively easy to undo with some defvars as well.

[bmag]

Yes, my original intention was that purpose-mode is always enabled and can be temporarily disable by let-binding purpose--active-p or by using the macros without-purpose and without-purpose-command, hence the fixes don't have a removal mechanism. Still, I probably should've made some clean uninstall option.

Just to be clear, we want:

• fixes are active while purpose-mode is on.
• fixes are inactive while the mode is off.

We don't want, even as an option:

• fixes are inactive while the mode is on.
• fixes are active while the mode is off. (of course)

What I have in mind is the following:

• introduce functions purpose-enable-fixes and purpose-disable-fixes. These will mainly add/remove advices en-mass, but also whatever else is necessary to enable/disable each fix.
• purpose-mode will call purpose-enable-fixes (when turned on) and purpose-disable-fixes (when turned off), and won't call purpose-fix-install.
• the first time purpose-enable-fixes is called, it will call purpose-fix-install.
• purpose-disable-fixes doesn't have to revert all the variables changed by fixes - only those that will still have an effect when purpose-mode is off.

There is an issue with lazy-loading and when to call add-advice - currently most of the add-advice calls are wrapped in a with-eval-after-load, but this will have to change. I think the solution will be to use featurep in purpose-enable-fixes and purpose-disable-fixes. For example with popwin:

(defun purpose-enable-fixes ()
  (unless purpose--installed-fixes    ; a variable that is initialized to nil
    (purpose-fix-install)
    (setq purpose--installed-fixes t))
  ;; ...
  (when (featurep 'popwin)
    (advice-add 'popwin:replicate-window-config :around 'purpose--fix-popwin-replicate))
  ;; ...
  )

(defun purpose-disable-fixes ()
  (when purpose--installed-fixes    ; guard against turning off `purpose-mode` before even turning it on (technically possible)
    ;; ...
    (when (featurep 'popwin)
      (advice-remove 'popwin:replicate-window-config 'purpose--fix-popwin-replicate))
    ;; ...
    ))

[wyuenho]

Thanks for writing down your plan. Absolutely agree the fixes should be on when the mode is on, and off when the mode is off.

However, I don't agree with removing with-eval-after-load tho. Whenever these packages are loaded, they should respect purpose and work well with it. Switching to feature detection will result in situations where one will have purpose-mode turned on, and the fixes available, but not applied when say org or which-key are loaded. The reason you'll need this is because of use-package. Many packages aren't loaded until they are used when a keybinding is pressed.

I think if you want to go for explicitness, introducing purpose-enable-fixes and purpose-disable-fixes are absolutely the right way to go, but if you want a minimalist approach, I think just a purpose-advice-add that record all the advisors and advisees into a list for purpose--remove-advices to loop thru is enough. I agree the rest of the configurations that have no effect when purpose-mode is off can be left alone.

[bmag]

I opted to add a hook named purpose-fix-togglers, that is ran when purpose-mode is toggled. Each function in that hook either enables or disables a fix, depending on the value of purpose-mode. Installing a fix now means enabling the fix (lazily, and only if purpose-mode is active when the fix is evaluated), as well as adding a toggler function to that hook. The fixes are installed the first time purpose-mode is activated.

The code is actually simpler than how this sounds, just look in #180. I think the PR is fine, but just in case I marked you as a reviewer so tell me if you have any further suggestions.