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

Cleanup usage of frame parameters #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

nnicandro
Copy link
Contributor

The main change of this pull request is to hide all modification of frame parameters behind the setf macro. So that instead of having (set-frame-parameter nil 'persp-foo ...) we have (setf (persp-foo) ...). It also adds optional frame arguments to a few functions to avoid the use of with-selected-frame and also removes persp-let-frame-parameters as it seemed unnecessary.

@@ -482,7 +508,8 @@ If NORECORD is non-nil, do not update the
(if (null name) (setq name (persp-prompt (and (persp-last) (persp-name (persp-last))))))
(if (and (persp-curr) (equal name (persp-name (persp-curr)))) name
(let ((persp (gethash name (perspectives-hash))))
(set-frame-parameter nil 'persp--last (persp-curr))
(when (and (persp-curr) (not (persp-killed (persp-curr))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not (persp-killed (persp-curr))) makes the persp-let-frame-parameters in persp-kill unnecessary.


(defadvice recursive-edit (around persp-preserve-for-recursive-edit)
"Preserve the current perspective when entering a recursive edit."
(persp-protect
(persp-save)
(persp-let-frame-parameters ((persp--recursive (persp-curr)))
Copy link
Contributor Author

@nnicandro nnicandro Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this persp-let-frame-parameters since its usage is simple enough that we can just expand out what its doing.

@nnicandro
Copy link
Contributor Author

This is something that I've had in my local copy of perspective for almost a year, just never made a pull request, so I don't think there should be any issues it introduces.

@gcv
Copy link
Collaborator

gcv commented Sep 7, 2019

This is really interesting, thanks! I'll need more time to think about this patch.

There's an argument for eliminating frame parameters for maintaining perspective state altogether. In any case, I'd like to merge a few other PRs before this one and deal with potential conflicts in that order.

@nnicandro
Copy link
Contributor Author

Yes, I saw your comment in #90 which is why I thought it was the right time to make a pull request since it would make that transition extremely simple.

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.

2 participants