Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Symbol’s function definition is void: window-purpose/save-dedicated-windows #16413

Merged
merged 2 commits into from
May 30, 2024

Conversation

kassick
Copy link
Contributor

@kassick kassick commented May 24, 2024

Commit 1ce1b0e introduced a bug due to incorrect usage of define-advice.

This commit refactors the change by:

  • Extracting the advices to named functions spacemacs/window-purpose-{save,restore}-dedicated-windows
  • Using advice-add globally instead of a define-advice/advice-add mix

…ted-windows

Commit 1ce1b0e introduced a bug due to
incorrect usage of define-advice.

This commit refactors the change by:
- Extracting the advices to named functions spacemacs/window-purpose-{save,restore}-dedicated-windows
- Using advice-add globally instead of a define-advice/advice-add mix
Commit f610a48 fixed the usage of
`define-advice` in the `spacemacs-purpose` layer functions, but not in the
`spacemacs-purpose-popwin` package.

The error did not appear before because the package was already installed in
`.emacs.d/elpa/...` using the code before the change to define-advice. Once it
was reinstalled, the new error started popping up when trying to pop up (sic)
a new window.

This commit replaces `define-advice` by `advice-add` and only advises the
functions once the mode is activated (instead of during package load).
@kassick
Copy link
Contributor Author

kassick commented May 29, 2024

eb36a82 fixes an error that appeared once the spacemacs-purpose-popwin package was reinstalled (from layers/+spacemacs/spacemacs-purpose/local/spacemacs-purpose-popwin to ~/.emacs.d/elpa/${EMACS_VERSION}/develop/spacemacs-purpose-popwin-${BUILD_DATE}

If the error persist after integrating these commits, remove the spacemacs-purpose-popwin package from the elpa folder and restart emacs.

@kassick
Copy link
Contributor Author

kassick commented May 29, 2024

@heartnheart and @cormacc you may want to review your thumbs-up, since there are new commits.

@smile13241324 smile13241324 merged commit 03dfd17 into syl20bnr:develop May 30, 2024
7 of 8 checks passed
@sunlin7
Copy link
Contributor

sunlin7 commented May 30, 2024

Hi @kassick
Thanks for the contributions.
I didn't met an issue for

Commit 1ce1b0e introduced a bug due to incorrect usage of define-advice.

Could you give more details to help us understanding what happened? Or, why it releated to define-advice Thanks

@kassick
Copy link
Contributor Author

kassick commented May 30, 2024

Hi @sunlin7 !

define-advice requires the parameter list for the function defined in body.

The followig snippet creates a function named hello@before-hello and adds it as a before advice to the target function:

(defun hello () (message "Hello World"))

(define-advice hello (:before (&rest args) before-hello)
  (message "before"))

The name is optional, though, so it can be used as

(define-advice hello (:before (&rest args))
  (message "before"))

to create an anonymous function.

Calling define-advice without the argument list does not work as expected, as the function created by define-advice is invalid. The following snippet yields Debugger entered--Lisp error: (invalid-function ((t) before-hello (message "before")))

(define-advice hello (:before before-hello)
  (message "before"))

(hello)

(Also, the function does not have a name, so anywhere expecting to use before-hello would fail with some symbol definition void error.)

I could have kept the use of define-advice by adding a (&rest args) to the definition, but then we'd need to adapt the code to point to name@advice-fn-name (e.g. popwin:create-popup-window@window-purpose/save-dedicated-windows) here and elsewhere in the code. I've preferred to use a simpler defun with the existing advice-add/advice-remove that is already present in the code.


As to why you did not encounter any issue, I'm not sure... Are you using the spacemacs-purpose layer? describe-function popwin:create-popup-window does show the advices if you are running spacemacs at 1ce1b0e ?

I'm using emacs 29.3.50 built from sources -- could it be due some version-specific behavior ?

@kassick
Copy link
Contributor Author

kassick commented May 30, 2024

Btw, the Symbol’s function definition is void error happens because using define-advice did not create a function named window-purpose/save-dedicated-windows (it ended up defining an invalid, anonymous function). But here, the layer adds window-purpose/save-dedicated-windows as an advice to popwin:create-popup-window -- it ends up before the invalid, anonymous function.

When popwin:create-popup-window is called, it first tries to run window-purpose/save-dedicated-windows, which does not exist -- ergo function definition is void. If this advice-add was not used, then the error would be invalid function.

@sunlin7
Copy link
Contributor

sunlin7 commented May 31, 2024

👍 Got the key point, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants