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 fullscreenState toggling behaviour #7288

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

txkyel
Copy link
Contributor

@txkyel txkyel commented Aug 11, 2024

Describe your PR, what does it fix/add?

When the user toggles the either the internal or client fullscreen states (using -1 to not alter the other state) the relevant state is toggled.

e.g. when the initial state is 1 2 and 1 -1 is dispatched, the state is now set to 0 2 instead of syncing the states to 2 2.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

None

Is it ready for merging, or does it need work?

Ready for merge

fyi @MightyPlaza

Thread on previous commit relevant to changes: 2b52057#r145242308

@txkyel
Copy link
Contributor Author

txkyel commented Aug 11, 2024

I'd like to add documentation for this behaviour but I'm not too sure whether it'd be more appropriate to place on the dispatchers page or the rules page.

Any suggestions?

@MightyPlaza
Copy link
Contributor

I don't think setting syncFullscreen default to false is a good idea, in most cases you'll want it set to true (for example when using a fullscreen button on a client itself)

src/Compositor.cpp Outdated Show resolved Hide resolved
@txkyel
Copy link
Contributor Author

txkyel commented Aug 11, 2024

It looks like I've completely misunderstood the functionality and importants of syncFullscreen. I expected syncFullscreen to only affect the behaviour of the fullscreenState dispatcher. I'll revert these changes.

I still don't quite agree with the toggle behaviour of the fullscreenState. Perhaps it would instead make sense to add an additional parameter determine the toggle behaviour to toggle the individual state or to sync the states. What do you think?

@MightyPlaza
Copy link
Contributor

MightyPlaza commented Aug 11, 2024

It looks like I've completely misunderstood the functionality and importants of syncFullscreen. I expected syncFullscreen to only affect the behaviour of the fullscreenState dispatcher. I'll revert these changes.

I still don't quite agree with the toggle behaviour of the fullscreenState. Perhaps it would instead make sense to add an additional parameter determine the toggle behaviour to toggle the individual state or to sync the states. What do you think?

I think the best solution for now would be to toggle it to FSMODE_NONE individually and only re-sync if both modes get toggled to FSMODE_NONE.

But still don't know what the best way to implement toggling would be

@txkyel
Copy link
Contributor Author

txkyel commented Aug 11, 2024

re-sync if both modes get toggled to FSMODE_NONE

Do you mean we should set syncFullscreen to false in the cases where only one state is toggled? What would be the effect of leaving syncFullscreen as true when the internal and client states are not in sync?

@txkyel txkyel changed the title Fix fullscreenState interaction with syncfullscreen Fix fullscreenState toggling behaviour Aug 11, 2024
@MightyPlaza
Copy link
Contributor

re-sync if both modes get toggled to FSMODE_NONE

Do you mean we should set syncFullscreen to false in the cases where only one state is toggled? What would be the effect of leaving syncFullscreen as true when the internal and client states are not in sync?

I mean when toggling back to FSMODE_NONE set syncFullscreen to true, to fully toggle the behaviour of the dispatcher

when syncFullscreen is set to true the internal and client modes should be kept the same

@vaxerski vaxerski merged commit c7b7279 into hyprwm:main Aug 12, 2024
10 checks passed
@txkyel txkyel deleted the fix-sync-fs-behaviour branch August 13, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants