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

Rework state management #322

Closed
minad opened this issue Jan 5, 2021 · 9 comments
Closed

Rework state management #322

minad opened this issue Jan 5, 2021 · 9 comments

Comments

@minad
Copy link
Contributor

minad commented Jan 5, 2021

See https://www.reddit.com/r/emacs/comments/kqutap/selectrum_prescient_consult_embark_getting_started/gi77l5e/?context=3

@clemera I took the freedom to create an issue here regarding this in order to keep it in mind.

@clemera
Copy link
Collaborator

clemera commented Jan 5, 2021

Good idea, thanks! To summarize: Remove the global state macro in favour of local session state variables.

@minad
Copy link
Contributor Author

minad commented Jan 5, 2021

One more: Declare all variables as defvar-local if they are only intended to be used in the minibuffer. This makes their scope more clear.

And I should add the motivation: This will fix all recursive minibuffer issues forever! Hopefully it will also require less code.

@clemera
Copy link
Collaborator

clemera commented Jan 5, 2021

For some variables we needed the first (global) value but when selectrum-repeat can be replaced by something better as discussed on the embark side this might not be the case anymore.

This will fix all recursive minibuffer issues forever!

I wonder if there an easy way to retrieve local values from the previous minibuffer session?

@minad
Copy link
Contributor Author

minad commented Jan 5, 2021

I wonder if there an easy way to retrieve local values from the previous minibuffer session?

I only know the general way, find minibuffer-buffers and extract the local value from them. Why do we need this? I would like the recursive sessions to be independent! Is this not possible or not desired in some cases?

@clemera
Copy link
Collaborator

clemera commented Jan 5, 2021

I don't think we need it but it can be handy at times, I don't remember exactly where I have seen it unfortunately. One benefit of the let bound variables was that the values are available whatever buffer is selected, when removing this we have to make sure to always query the values from the minibuffer.

@minad
Copy link
Contributor Author

minad commented Jan 5, 2021

One benefit of the let bound variables was that the values are available whatever buffer is selected, when removing this we have to make sure to always query the values from the minibuffer.

I don't understand this. You are also using setq-local at some places so you can never be sure. This is something I am very much against. This is like some kind of Heisen state, maybe being available maybe not. From my pov, the variables should be defined as local and should only be set in the minibuffer. This is how I am doing things in consult. And there I am only doing that when I cannot use closures.

@clemera
Copy link
Collaborator

clemera commented Jan 5, 2021

Yes when adding new variables I usually set them local since we already discussed removing the macro in the past. This is just how things evolved I have not said I like the situation but it also is possible to fix, we just need to be a bit careful not to break things. Would be nice to have tests already ;)

@minad
Copy link
Contributor Author

minad commented Feb 16, 2021

Happy to close this issue! Thank you for tackling the state management, @clemera 👍

@minad minad closed this as completed Feb 16, 2021
@clemera
Copy link
Collaborator

clemera commented Feb 16, 2021

Thank you, too for the discussion and help!

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

No branches or pull requests

2 participants