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 fullscreen toggle bug #190

Merged
merged 3 commits into from
May 9, 2021

Conversation

wsldankers
Copy link
Contributor

@wsldankers wsldankers commented May 4, 2021

To reproduce:

  1. Bind a key to toggle_fullscreen(2):
[keybindings]
<Escape> { toggle_fullscreen(2); }
  1. Start pqiv with otherwise default options and keybindings, with a filename as argument

  2. Press Escape

  3. Press f (as often as you like)

Expected behavior: f in step 4 toggles fullscreen mode

Observed behavior: nothing happens.

Root cause:

a) The Escape in step 3 tries to leave fullscreen even when fullscreen is not active.

b) window_unfullscreen() then tries to set main_window_in_fullscreen to FALSE temporarily (even though it already was FALSE) then "restore" it incorrectly by setting it to TRUE.

c) This means main_window_in_fullscreen is now out of sync with reality.

d) From this point on, any attempt to toggle it will try to toggle it in the wrong direction.

Debug trace (pressed Escape once, then f twice):

pqiv.c:5748: action(): main_window_in_fullscreen=0 parameter.pint=2
pqiv.c:4191: window_unfullscreen(): main_window_in_fullscreen=0
pqiv.c:4230: window_unfullscreen(): main_window_in_fullscreen=1
pqiv.c:5748: action(): main_window_in_fullscreen=1 parameter.pint=0
pqiv.c:4191: window_unfullscreen(): main_window_in_fullscreen=1
pqiv.c:4230: window_unfullscreen(): main_window_in_fullscreen=1
pqiv.c:5748: action(): main_window_in_fullscreen=1 parameter.pint=0
pqiv.c:4191: window_unfullscreen(): main_window_in_fullscreen=1
pqiv.c:4230: window_unfullscreen(): main_window_in_fullscreen=1

To reproduce:

1) Bind a key to toggle_fullscreen(2):

	[keybindings]
	<Escape> { toggle_fullscreen(2); }

2) Start pqiv with otherwise default options and keybindings,
   with a filename as argument

3) Press Escape

4) Press f (as often as you like)

Expected behavior: f in step 4 toggles fullscreen mode

Observed behavior: nothing happens.

Root cause:

a) The Escape in step 3 tries to leave fullscreen even when
   fullscreen is not active.

b) window_unfullscreen() then tries to set main_window_in_fullscreen to
   FALSE temporarily (even though it already was FALSE) then "restore" it
   incorrectly by setting it to TRUE.

c) This means main_window_in_fullscreen is now out of sync with reality.

d) From this point on, any attempt to toggle it will try to toggle it in
   the wrong direction.

Debug trace (press Escape once, then 'f' twice):

	pqiv.c:5748: action(): main_window_in_fullscreen=0 parameter.pint=2
	pqiv.c:4191: window_unfullscreen(): main_window_in_fullscreen=0
	pqiv.c:4230: window_unfullscreen(): main_window_in_fullscreen=1
	pqiv.c:5748: action(): main_window_in_fullscreen=1 parameter.pint=0
	pqiv.c:4191: window_unfullscreen(): main_window_in_fullscreen=1
	pqiv.c:4230: window_unfullscreen(): main_window_in_fullscreen=1
	pqiv.c:5748: action(): main_window_in_fullscreen=1 parameter.pint=0
	pqiv.c:4191: window_unfullscreen(): main_window_in_fullscreen=1
	pqiv.c:4230: window_unfullscreen(): main_window_in_fullscreen=1
@phillipberndt
Copy link
Owner

Oh, I thought I had posted a response already - sorry for taking long to actually do..

Thanks a lot for taking the time and already doing the debugging & finding a fix!

If you like, add a commit to add this change to the changelog in the readme, and yourself to the contributors list. Otherwise I'll merge this one of the next days & add an entry to the changelog myself.

@wsldankers
Copy link
Contributor Author

Commit added!

Mark 2.13 as development version
@phillipberndt phillipberndt merged commit d2d7ebb into phillipberndt:master May 9, 2021
@phillipberndt
Copy link
Owner

Thanks again! Merged.

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.

None yet

2 participants