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 excusive zone when zwf_shell_manager_v2 is unavailable #199

Merged

Conversation

oldlavender
Copy link
Contributor

@oldlavender oldlavender commented Nov 29, 2023

Commit 6652635 fixed autohide issues when zwf_shell_manager_v2 isn't available, but changed, as a side effect, the behaviour of the window's auto_exclusive_zone, causing an unnecessary dependency on the zwf_shell_manager_v2 for the exclusive zone as well, affecting wf-panel when the compositor's wayfire-shell plugin is disabled (see issue #198 for details).

Fix it by refactoring the WayfireAutohidingWindow class, dividing some methods charged with too much roles (i.e the
WayfireAutohidingWindow::set_autoexclusive_zone()), transfering constructor code into new methods, introducing some new logic controls both in the class body and in the callbacks.

Testing

To ensure everything is working fine, I did several tests on both wf-panel and wf-deck to check the behavior under different conditions. Here's what I tested:

wf-panel

I tested it both with the wayfire-shell plugin enabled and disabled, and changing the autohide config on the fly, both by starting it enabled and disabled. I looked for some key behaviors, and it worked as expected (by me, at least) on different scenarios. Here I'm pasting my notes on it while I tested:

S->wayfire-shell plugin loaded at beginning
I->initial autohide value
C{1,2}->Changes of autohide value 1 and 2

Test Status S I C1 C2
1 Success 0 0 1 0
2 Success 0 1 0 1
3 Success 1 0 1 0
4 Success 1 1 0 1

Results:

Test Scenario Id #1
[y] Show zwf_shell_manager_v2 disabled warning at startup
[y] Reposition windows out of panel's area at startup
[n] Actually enable autohide and disable exclusive zone when autohide is set to true
[y] Show swf_shell_manager_v2 disabled warning when attempts to enable autohide
[-] Reposition windows on panel's area when enable autohide
[-] Reposition windows out of pane's area when disable autohide

Test Scenario Id #2
[y] Show zwf_shell_manager_v2 disabled warning at startup
[y] Reposition windows out of panel's area at startup
[n] Actually enable autohide and disable exclusive zone when autohide is set to true
[y] Show swf_shell_manager_v2 disabled warning when attempts to enable autohide
[-] Reposition windows on panel's area when enable autohide
[-] Reposition windows out of pane's area when disable autohide

Test Scenario Id #3
[n] Show zwf_shell_manager_v2 disabled warning at startup
[y] Reposition windows out of panel's area at startup
[y] Actually enable autohide and disable exclusive zone when autohide is set to true
[n] Show swf_shell_manager_v2 disabled warning when attempts to enable autohide
[y] Reposition windows on panel's area when enable autohide
[y] Reposition windows out of pane's area when disable autohide (*)

Test Scenario Id #4
[n] Show zwf_shell_manager_v2 disabled warning at startup
[n] Reposition windows out of panel's area at startup
[y] Actually enable autohide and disable exclusive zone when autohide is set to true
[n] Show swf_shell_manager_v2 disabled warning when attempts to enable autohide
[y] Reposition windows on panel's area when enable autohide
[y] Reposition windows out of pane's area when disable autohide (*)

(*) Sometimes it needs to move the mouse over the edge to make the panel visible again

wf-dock

Since here there aren't as much factors as in wf-panel, I just tested it by opening it with and without the zwf_shell_manager_v2 connected. In both cases it behaved as expected, with autohiding enabled when the shell manager is available, and disabled when not.

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

In general, I dislike adding more variables than necessary because we end up with duplicated information which may get out of sync .. Otherwise, the idea to move each functionality to an appropriate function is good.

src/util/wf-autohide-window.hpp Outdated Show resolved Hide resolved
src/util/wf-autohide-window.hpp Outdated Show resolved Hide resolved
@oldlavender oldlavender marked this pull request as draft November 30, 2023 11:51
@oldlavender oldlavender force-pushed the fix/bug-maximized-window-geometry branch from 5360b0e to 0e5f39d Compare November 30, 2023 12:25
Commit `6652635` fixed autohide issues when `zwf_shell_manager_v2` isn't
available, but changed, as a side effect, the behaviour of the window's
`auto_exclusive_zone`, causing an unnecessary dependency on the
`zwf_shell_manager_v2` for the exclusive zone as well, affecting
wf-panel when the compositor's wayfire-shell plugin is disabled (see
issue WayfireWM#198 for details).

Fix it by refactoring the `WayfireAutohidingWindow` class, dividing some
methods charged with too much roles (i.e the
`WayfireAutohidingWindow::set_autoexclusive_zone()`), transfering
constructor code into new methods, introducing some new logic controls
both in the class body and in the callbacks.

Signed-off-by: Ágatha Isabelle Moreira Guedes <me@agatha.dev>
@oldlavender oldlavender force-pushed the fix/bug-maximized-window-geometry branch from 0e5f39d to 3652736 Compare November 30, 2023 12:29
As suggested in PR WayfireWM#199 [1], this information is better retrieved by
checking whether `output->output` is not NULL.

[1] WayfireWM#199 (comment)

Signed-off-by: Ágatha Isabelle Moreira Guedes <me@agatha.dev>
As suggested [1], remove the `autohide_enable` boolean and rely on the
previously existing ones only.

[1] Comments for Pull-Request WayfireWM#199.
WayfireWM#199 (comment)

Signed-off-by: Ágatha Isabelle Moreira Guedes <me@agatha.dev>
@oldlavender
Copy link
Contributor Author

oldlavender commented Dec 2, 2023

I worked out the previous points in the previous review, so I'm reopening the PR

@marcusbritanicus
Copy link
Contributor

@ammen99 I cam confirm that this works perfectly well all the cases I tested:

  1. Autohide is off and wayfire-shell plugin in not loaded. (no autohide and exclusive zone)
  2. Autohide is off and wayfire-shell plugin in loaded. (no autohide and exclusive zone)
  3. Autohide is on and wayfire-shell plugin in not loaded. (no autohide and exclusive zone)
  4. Autohide is on and wayfire-shell plugin in loaded. (autohide and no exclusive zone)

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ammen99 ammen99 merged commit a750421 into WayfireWM:master Jan 11, 2024
ammen99 pushed a commit that referenced this pull request Mar 13, 2024
* Fix excusive zone when `zwf_shell_manager_v2` is unavailable

Commit `6652635` fixed autohide issues when `zwf_shell_manager_v2` isn't
available, but changed, as a side effect, the behaviour of the window's
`auto_exclusive_zone`, causing an unnecessary dependency on the
`zwf_shell_manager_v2` for the exclusive zone as well, affecting
wf-panel when the compositor's wayfire-shell plugin is disabled (see
issue #198 for details).

Fix it by refactoring the `WayfireAutohidingWindow` class, dividing some
methods charged with too much roles (i.e the
`WayfireAutohidingWindow::set_autoexclusive_zone()`), transfering
constructor code into new methods, introducing some new logic controls
both in the class body and in the callbacks.

Signed-off-by: Ágatha Isabelle Moreira Guedes <me@agatha.dev>

* Remove `zwf_support` boolean

As suggested in PR #199 [1], this information is better retrieved by
checking whether `output->output` is not NULL.

[1] #199 (comment)

Signed-off-by: Ágatha Isabelle Moreira Guedes <me@agatha.dev>

* Remove `autohide_enabled` boolean

As suggested [1], remove the `autohide_enable` boolean and rely on the
previously existing ones only.

[1] Comments for Pull-Request #199.
#199 (comment)

Signed-off-by: Ágatha Isabelle Moreira Guedes <me@agatha.dev>

---------

Signed-off-by: Ágatha Isabelle Moreira Guedes <me@agatha.dev>
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.

4 participants