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

Add command to toggle automatic output #116

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

Conversation

halvarsu
Copy link

@halvarsu halvarsu commented Oct 3, 2023

Does what it says, toggling the automatic output feature. I personally found this to be useful, as automatic output can be either nice or a nuisance depending on the situation/notebook. I'm not sure if there is some other way of achieving this through nvim; this seemed like the easiest solution to me.

@benlubas
Copy link

benlubas commented Oct 4, 2023

This is an interesting PR, b/c there are two approaches here. One of them is the PR that you've submitted, just add a single purpose "toggle" function to magma.

But what if others want to change their image provider or whether or not they copy output automatically, or any other feature controlled by a configuration variable?

Then it might be better to re-write the way the MagmaOptions class works. Instead of grabbing global values only on load, perhaps we could grab them each time they're needed, or provide a general command: MagmaRefreshOptions which can then be used by users in a keymap or function.

@halvarsu
Copy link
Author

halvarsu commented Oct 5, 2023

Agreed, good point! Your approach sounds much better. I had to learn the hard way that this wasn't the way it was working already...

Which then begs the question, why would someone not want global values to be grabbed continuously? Do you have a use case where MagmaRefreshOptions is a better solution in mind?

@benlubas
Copy link

benlubas commented Oct 5, 2023

I'd imagine b/c this is a remote plugin fetching the config values and storing them is a little more performant.

I'm not 100% sure on that, but it just feels like it would be the case

@benlubas
Copy link

benlubas commented Oct 6, 2023

My version of the plugin is not nearly ready yet (there are new breaking changes every day at this rate), but I have added a function that lets users set options after initialization, here's the entry in the readme

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