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

feat: minimize window on suspend #1971

Merged
merged 7 commits into from Oct 14, 2023

Conversation

SyedAhkam
Copy link
Contributor

@SyedAhkam SyedAhkam commented Aug 10, 2023

What kind of change does this PR introduce?

A feature! Hooks into :suspend, interprets it as a Minimize request and minimizes the window.

Fixes: #1496

Did this PR introduce a breaking change?

No

Copy link
Contributor

@MultisampledNight MultisampledNight left a comment

Choose a reason for hiding this comment

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

First of all, thank you!

Then, I'm not entirely sure about the interaction which is done with the command here and the already existent g:neovide_fullscreen variable. The advantage I see behind the variable is that it's not necessary to keep track of the variable separately if a more special scripted behavior is desired. At the moment it even looks like it's not possible to return from the minimized state.

On the other hand it'd be also kind of fair to say that returning from minimization must be done through the user, or through platform specific APIs in a separately running script.

@fredizzimo
Copy link
Member

I just noticed that the UI protocol provides suspend https://neovim.io/doc/user/ui.html. So I think a better implementation is to hook into that, sorry for misleading you @SyedAhkam

@SyedAhkam
Copy link
Contributor Author

In that case we do not need to expose a custom command right?

Also reading the documentation for suspend, it is mentioned that the behaviour of its invocation is implementation specific. I wonder if it is safe to interpret it as a minimize window request.

@fredizzimo
Copy link
Member

@SyedAhkam

That's correct, no custom command is needed, we just need to react to it, like we do for other UI events. The users can still bind :suspend to the keybinding they like, or even automate it if they want to.

I think for now, it's safe to assume minimizing the window. But I think the end goal would be to check if we started with --nofork and do a real suspend in that case on platforms that supports it. That would be especially handy for users of tiling window managers with terminals that swallow.

@SyedAhkam SyedAhkam changed the title feat: expose a NeovideMinimize command feat: minimize window on suspend Aug 19, 2023
@SyedAhkam
Copy link
Contributor Author

Thanks for the direction. I have now reverted the change which added a custom NeovideMinimize command and instead hooks into :suspend via UI commands.

We can now discuss about doing 'real suspend' further.

@MultisampledNight
Copy link
Contributor

MultisampledNight commented Sep 13, 2023

Do you want to rebase/merge main into this? Or should I do it?

@MultisampledNight
Copy link
Contributor

I'll do it now then.

@MultisampledNight
Copy link
Contributor

Thank you! Choooooooooo choooooooo 🚂

@MultisampledNight MultisampledNight merged commit f22b541 into neovide:main Oct 14, 2023
2 checks passed
@SyedAhkam SyedAhkam deleted the minimize-window-rpc branch October 14, 2023 20:32
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.

Minimize on ctrl+z
3 participants