Skip to content

desktop: Add fullscreen keyboard shortcut#1510

Merged
Herschel merged 3 commits intoruffle-rs:masterfrom
relrelb:fullscreen
Jan 7, 2021
Merged

desktop: Add fullscreen keyboard shortcut#1510
Herschel merged 3 commits intoruffle-rs:masterfrom
relrelb:fullscreen

Conversation

@relrelb
Copy link
Copy Markdown
Contributor

@relrelb relrelb commented Nov 6, 2020

This PR adds Alt+Enter as a keyboard shortcut to toggle fullscreen.
Should more or less fix #186.
The major downside here is that the player will never receive events for Alt+Enter. I see 3 alternatives:

  1. Choose a different (less common) keyboard shortcut - but I think Alt+Enter is rare enough.
  2. Continue passing those events to the player - but this may cause unintended behavior.
  3. Add the fullscreen option to a context menu, so it doesn't interfere with any player events - but winit doesn't support context menus out-of-the-box.

@Herschel
Copy link
Copy Markdown
Member

Herschel commented Nov 6, 2020

Let's make it the same key to toggle in/out of fullscreen for now, because particularly Escape could be a commonly used key in games. Also probably better to use a combination like Alt+Enter or Ctrl+F (both commonly used, for example, Windows Media Player uses Alt+Enter).

@relrelb relrelb changed the title desktop: Add fullscreen keyboard shortcut desktop, web: Add fullscreen functionality Nov 6, 2020
@relrelb relrelb changed the title desktop, web: Add fullscreen functionality desktop: Add fullscreen keyboard shortcut Nov 7, 2020
@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Nov 7, 2020

Split the web fullscreen to #1515, so this PR is only for the desktop fullscreen.

@Herschel
Copy link
Copy Markdown
Member

Herschel commented Nov 7, 2020

Unfortunately on my windows 10 machine, exiting fullscreen mode seems to keep the fullscreen window size, with the border off the screen so that I can't move/resize it. So I guess we have to resize and reposition the window when exiting fullscreen. Something like:

window.set_inner_size(movie_size);
if let Some(monitor) = window.primary_monitor() {
    let monitor_position = monitor.position();
    let monitor_size = monitor.size();
    let window_size = window.outer_size();
    let center = PhysicalPosition {
        x: (monitor_position.x + monitor_size.width as i32
            - window_size.width as i32)
            / 2,
        y: (monitor_position.y
            + monitor_size.height as i32
            - window_size.height as i32)
            / 2,
    };
    window.set_outer_position(center);
}

Not sure if there is a simpler way to center a window via winit.

@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Nov 14, 2020

After investigating a bit, this seems to happen only when entering fullscreen with maximized window. This doesn't happen when entering fullscreen without maximizing the window.
Ideally I think that exiting fullscreen should restore the window state (size, position, minimized/maximized) to what it was before entering it, so centering the window is not the proper solution in my opinion.
It feels to me like a misbehavior of winit, because it always unmaximizes the window when exiting fullscreen, and it also happens in their example fullscreen.rs.

@Herschel
Copy link
Copy Markdown
Member

On my home PC, the problem occurs regardless of whether I was in the maximized state when entering fullscreen (i.e., if I start the app and then press Alt+Enter twice, the window size still takes up the fullscreen and is difficult to resize). Possibly related to # of monitors or desktop size. Would be good to file an issue on the winit repo.

In the meantime we would have to work around it on our side. Remembering the previous position when entering fullscreen would be better, I agree. You could store the window dimensions when toggling into fullscreen.

@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Nov 15, 2020

Opened rust-windowing/winit#1765. Since this feature is just QOL and is low priority, I prefer to wait and merge this PR once winit is updated.

@Herschel
Copy link
Copy Markdown
Member

Herschel commented Nov 16, 2020

I investigated a little more because the winit fullscreen example does not have the same issue for me, where the window is not properly sized even if I Alt+Enter with a normal sized window. The issue is that I am using the dx12 wgpu backend, and DXGI (used by dx11 and 12) seems to automatically install its own built-in Alt+Enter fullscreen handler, and this interacts badly with our own Alt+Enter handler. We'd want to disable this (https://docs.microsoft.com/en-us/windows/win32/api/dxgi/nf-dxgi-idxgifactory-makewindowassociation). I think this should be filed as an issue on gfx-rs.

Alternatively we could just use a different key combo for now.

@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Nov 16, 2020

Interesting, I see two options:

  1. Disable the Alt+Enter hook of DXGI as you suggested.
  2. I wonder how Alt+Enter works for you on master. If it works as expected (or even better), we might consider to disable our Alt+Enter response and let DXGI handle it.

Since it would be very confusing to have two different fullscreen shortcuts, and since the fullscreen functionality is not urgent at all, I think choosing a different key combo is not the ideal solution for both short and long terms.

@Herschel
Copy link
Copy Markdown
Member

I tossed a PR to gfx-rs (gfx-rs/gfx#3479), so we should get the Alt+Enter fix whenever wgpu bumps its gfx-rs version.

@ousia
Copy link
Copy Markdown
Contributor

ousia commented Nov 28, 2020

@relrelb, I wonder whether a keyboard shortcut for fullscreen could be also added to web.

I saw that this could be extremely useful to read the error message in rather small windows (such as the one reported in #1755).

I know that there is a context menu for that, but it seems to be tricky to use with a touchpad.

@ousia
Copy link
Copy Markdown
Contributor

ousia commented Nov 28, 2020

Even for standard presentations, having a keyboard shortcut to play them in fullscreen would be extremely helpful.

@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Dec 4, 2020

I tossed a PR to gfx-rs (gfx-rs/gfx#3479), so we should get the Alt+Enter fix whenever wgpu bumps its gfx-rs version.

I have updated wgpu, so it now contains your fix. @Herschel Can you please verify this works as expected?
But anyway, the winit issue still needs to be resolved.

@bbb651
Copy link
Copy Markdown
Contributor

bbb651 commented Dec 7, 2020

This PR in winit should fix the issue when it's merged.

@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Dec 11, 2020

This PR depends on #1927.

@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Dec 13, 2020

Since rust-windowing/winit#1784 is merged, all we need is to wait for a new winit release.

@relrelb relrelb force-pushed the fullscreen branch 3 times, most recently from 25bd6ee to bc7af89 Compare January 7, 2021 08:25
@relrelb
Copy link
Copy Markdown
Contributor Author

relrelb commented Jan 7, 2021

@Herschel Actually, we can merge this PR without waiting for the next winit release. I don't think winit really blocks us, and the bug will eventually be fixed, sooner or later.

@Herschel
Copy link
Copy Markdown
Member

Herschel commented Jan 7, 2021

Thanks!

@Herschel Herschel merged commit cb9bdcd into ruffle-rs:master Jan 7, 2021
@relrelb relrelb deleted the fullscreen branch January 7, 2021 20:57
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.

[desktop] Ctrl+F for fullscreen?

4 participants