Winit: Tracking Issue: DPI Usability Upgrades

[Osspial]

This issue tracks the implementation of the HiDPI API changes that were discussed in #837. PRs implementing this change should be made against the dpi-overhaul branch, and we'll merge that branch onto master once it's implemented on all platforms.

• [x] Public API changed
• [x] Implemented on all platforms
  
  
  
  • [x] Windows - #895
  
  
  • [x] macOS - #997
  
  
  • [x] iOS - #1223
  
  
  • [x] Linux X11 - #1098
  
  
  • [x] Linux Wayland - #1098
  
  
  • [x] Web - #1233
  
  
• [ ] Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• [ ] Updated feature matrix, if new features were added or implemented.
• [ ] Update DPI module docs to properly describe Winit's new DPI handling system - #1350
• [ ] Implement HiDpiFactorChanged on the web

[Osspial]

Mentoring Instructions

For the most part, implementing this should be a matter of following the error messages and converting to physical or logical coordinates as necessary.

Once the main implementation for a platform is complete, this may be a good place to look into fixing #940. However, that isn't strictly necessary if you aren't feeling up to it.

[goddessfreya]

So, I had to make this diagram to explain to someone why things got a lifetime, enjoy folks:

[felixrabe]

@ZeGentzy Awesome!

Could you somehow scale it down a tiny little bit (like, 10x)? I can't even fit it on one screen at max zoomed-out level in Firefox (30%). (https://imgur.com/a/yy6Qvk6) I don't have Inkscape installed right now to do it myself.

(I guess it is ironic that a diagram about DPI issues has ... DPI issues :) )

[goddessfreya]

[goddessfreya]

@murarth Do you have an interest in picking up this issue?

[murarth]

I have a question about the addition of new_inner_size: &mut Option<PhysicalSize> to the HiDpiFactorChanged event: What is the advantage of allowing applications to pass a size value this way versus calling Window::set_inner_size?

[goddessfreya]

It allows smooth resizing. Otherwise HiDpiFactorChanged would change the size once, then the users would change it once, causing the window's size to flicker.

[murarth]

What if, for platforms that support setting window size at the time of DPI change, the implementation were to track DPI change state and the event loop could use a size value provided by a Window::set_inner_size call made during the handling of the DPI change event? I've provided a pseudocode example to demonstrate what I mean by this:

struct Window {
    /// Tracks whether the event loop is currently handling a DPI change for this window
    is_handling_dpi_change: bool,
    /// Contains a new size for the new DPI, provided by the application
    dpi_change_size: Option<PhysicalSize>,
    // ...
}

impl Window {
    pub fn set_inner_size(&self, size: PhysicalSize) {
        if self.is_handling_dpi_change {
            // If a DPI change event is currently being handled,
            // provide the size value to the event loop.
            self.dpi_change_size = Some(size);
        } else {
            // Otherwise, the window is resized in the usual manner
        }
    }
}

impl EventLoop {
    fn run<F>(&self, callback: F) {
        loop {
            match receive_platform_event() {
                DpiChanged(window, new_dpi) => {
                    window.is_handling_dpi_change = true;

                    callback(WindowEvent::HiDpiFactorChanged(new_dpi));

                    window.is_handling_dpi_change = false;

                    let new_size = window.dpi_change_size.take()
                        .unwrap_or(/* Use size value suggested by the platform */);

                    // Respond to platform DPI change event with `new_size` value
                }
                // ...
            }
        }
    }
}

I feel that, while this solution may add some complexity to internal platform code, it may be less than the complexity of adding a lifetime parameter to the Event type. Additionally, it may be worthwhile to keep the public API simple.

I will admit, though, that I'm being a bit selfish in prosposing this, as I think the X11 event loop in particular may need significant changes to adapt to a lifetime parameter in the Event type. I'm certainly willing to implement the API as currently designed, but such changes mean the possibility of introducing new bugs and that's not good for anyone.

[goddessfreya]

@murarth I'm going to have to disagree with that proposal. As you mentioned that solution would add some considerate complexity to the internal code. In addition to that, I imagine it would be confusing for users that their calls to set_inner_size didn't change the size immediately.

Plus, it might not be obvious to users of the api that they need to call set_inner_size inside the dpi changed event for smooth resizing. Users could easily move it out of that call, and be confused why their window is flickering in size, ect.

[murarth]

@ZeGentzy Yeah. Fair enough. I'll get started on implementing this soon.

[murarth]

In the interest of avoiding more work down the road, I'd like to merge master into the dpi-overhaul branch. Would that be acceptable? If so, I can submit this as a separate PR.

[murarth]

I've submitted a PR for the merge with master (#1095). I can submit the X11/Wayland implementation once that is merged.

[murarth]

Never mind about merging master into dpi-overhaul. I don't think it would have reduced the work of the eventual merge, as both branches are still changing. Plus, I realize it would likely cause more headaches for those developing on that branch.

I've submitted a PR implementing this API for X11 and Wayland: #1098

[kennylevinsen]

PhysicalSize is converted to u32, which is very sensible. However, PhysicalPosition remains f64. I would assume that this is an oversight, as having support for non-integer offsets with only integer sizes seem illogical.

I apologize if I have missed any design discussions on that particular topic.

[vberger]

Some platforms provide sub-pixel precision for input events, so keeping the *Position types as floats allows to keep that precision, which might be useful for some applications.

[kennylevinsen]

Aha, that makes sense!

However, certain inputs also provide geometries (e.g. touch shape), which also have sub-pixel accuracy, and thus cannot be expressed with the current iteration of PhysicalSize.

Thus, to account for inputs, we must either:

1. Round inputs to integers
2. Let Physical* be all floats
3. Split off input coordinates to its own floating type

I'm a fan of the last solution. I feel like the other two are flawed generalizations.

[kennylevinsen]

Opened issue: https://github.com/rust-windowing/winit/issues/1135

[vbogaevsky]

@Osspial, I'm currently working on implementation for iOS.
Could you, please, mark it as in progress?

[goddessfreya]

@vbogaevsky I'm actually the one whose doing the marking :P

Okay, I've marked it as so, but couldn't find the PR. I assume it's not up, yet?

[vbogaevsky]

@ZeGentzy, oh, okay.
Yeah, I'm in a middle of a draft.

[Osspial]

@vbogaevsky What's the status on the iOS implementation?

[vbogaevsky]

@Osspial, I'm completing draft this week. I will also have to rework macOS implementation after #1173.

[vbogaevsky]

I've created PR for iOS implementation of HiDpiEventChanged #1223 . But I did not test it.
Can we update dpi-overhaul branch from master? Then I'll fix macOS implementation, so that it works correctly with #1173 changes.

[Osspial]

@vbogaevsky Yay! I'll rebase dpi-overhaul onto master, but you're likely going to have to fix up the macOS rebase. There were some merge conflicts, and I'm not confident I resolved them entirely correctly since I can't build for macOS.

[Osspial]

@vbogaevsky You can find the WIP rebase on dpi-overhaul-wip-rebase - there are enough merge conflicts on non-Windows platforms that I can't confidently resolve them without introducing errors. I ended up just restarting the rebase and only resolving the Windows conflicts. @murarth Could you also see about resolving the Linux merge conflicts on that branch?

[murarth]

@Osspial I've resolved the conflicts. Shall I push directly to the branch or submit a PR (which will fail CI on every platform, as cargo fmt chokes on <<<<<<< lines in macos)?

[Osspial]

@murarth force-pushing is fine

[murarth]

@Osspial I've pushed the changes to dpi-overhaul-wip-rebase.

[vbogaevsky]

@Osspial I'll then fix conflicts in dpi-overhaul-wip-rebase for macOS and open a PR since I can't push into original branches.

[Osspial]

@murarth Thanks!

@vbogaevsky Now that you're a member, you should have push access to branches in the main repo.

[vbogaevsky]

@Osspial, great! I'll finish with conflicts and push to dpi-overhaul-wip-rebase today.

[vbogaevsky]

@Osspial, I've resolved macOS merge conflicts in dpi-overhaul-wip-rebase.

[Osspial]

@vbogaevsky thanks! I've cleaned up the git history so we can avoid having commits with merge conflicts on master when dpi-overhaul gets merged, and pushed those changes to dpi-overhaul.

[tangmi]

I'm unsure if hidpi on the web is already well understood, but I hope this comment can be helpful!

I recently hacked together hidpi support for the web on winit-legacy (along with some other web changes, https://github.com/tangmi/winit/compare/341fe47c56ee11150b2b1fec4838410e642de1c3...2e116156963e94a7da3fa18183d20fb358939a2b).

What I found worked was:

• Treat CSS pixels as logical units
• Treat the CanvasElement's actual size as physical units.
• Use Window.devicePixelRatio to get the hidpi scale factor
  
  
  
  • e.g. If the desired dom size is 320x240 at a hidpi scale factor of 2x, the canvas element's size would be set to 640x480, while it's CSS size would be set to 320x240.
  
  
• Input events are tracked in CSS pixels and should be treated as in logical units

Here's a JSFiddle of it all coming together.

Edit: If this seems like a reasonable way to support hidpi on web, I'm interested in working this into @ryanisaacg's excellent recent web support (if I can find some time! Please, beat me to it if someone else is also interested!).

Edit 2: I have a draft PR (#1233), which is mainly missing tests (how were the other backend's hidpi settings tested?).

[Osspial]

While we're making all these changes to the API, should we rename hidpi_factor to scale_factor? Personally, I've always had a hard time remembering the exact name for the function, and scale_factor seems much more intuitive than the current name.

[tangmi]

@Osspial I don't have a strong opinion (as long as this value is documented as the scale from logical units to physical units!), but my gut feeling is that scale_factor might be a little ambiguous? Again, no strong opinion as long as we plan on keeping the (awesome) dpi module docs.