Screen Module, window titlebar, position, size, DPI

[rhzk]

Changes:

All changes are only implemented on Windows (but have skeletons for other platforms)

• Added Screen module, where you can get infomation about the screen and monitors.

• Added setting window size, position, maximized and minimized state.

• Added option to disable the default titlebar.

• Added option to let windows handle a custom titlebar like the default one (move, double click to maximize, etc (based on win-settings)).

• Changed window creation to use window's default window size if the user does not specify a window size.

• Implemented a solution for re-entrancy on Windows. Re-entrancy and RefCells #1068

• Improved DPI on windows (10), should now work correctly across monitors with different dpi settings

This should close the Window's side of #1044, #778, #634, #427. And #626

Some use-cases I could think of for this:

• Restore window size/position on next launch, as they where when it was closed.
• Create custom look for titlebar.
• Layout multiple windows (I think GIMP does/used to do this: picture).
• Create window(s) on a particular monitor.
• Change size to fit content, or make content fit window size (This might require additional API).
• Create window at a spesific point (For example an app that shows information about files when users hovers over them or pop up windows, like confirm dialogs/error messages etc)

[xStrom]

This is interesting and I'll take a closer look in the coming days.
However for now I wanted to mention one thing.

There also needs to be a way to exit the GUI from a widget (Borderless window has no 'X' button)

There's the CLOSE_WINDOW command that can achieve this already.

[rhzk]

Oh I missed that, is there also a way to access the window inside a widget?

[luleyleo]

You mean this one?:
https://docs.rs/druid/0.6.0/druid/struct.EventCtx.html#method.window

[cmyr]

I definitely think we are going to want something like a screen module, will give this a scan soon!

[rhzk]

I might be wrong but from my understanding of the current windows code it seems to be a circular reference between
WindowHandle -> WindowState -> WndProc -> WindowHandle.

This was solved by putting WindowHandle inside a RefCell. The issue now is that whenever a function inside WindowHandle is called for example:

pub fn invalidate(&self) {
        if let Some(w) = self.state.upgrade() {
            let hwnd = w.hwnd.get();
            unsafe {
                if InvalidateRect(hwnd, null(), FALSE) == FALSE {
                    log::warn!(
                        "InvalidateRect failed: {}",
                        Error::Hr(HRESULT_FROM_WIN32(GetLastError()))
                    );
                }
            }
        }
    }

&self is borrowed, meaning there is a borrow on WindowHandle.
The windows function "InvalidateRect" creates a WM_PAINT message and does a context-switch,
calling "fn window_proc" inside WndProc, that then tries to handle the WM_PAINT:

 WM_PAINT => unsafe {
                if let Ok(mut s) = self.state.try_borrow_mut() {

Here is tries to borrow WindowState, but fails because WindowState contains WindowHandle, and WindowHandle is already borrowed, inside its own &self function.

From my understanding this causes all Windows messages created inside WindowHandle to be dropped, and the current windows code might have to be rewritten / reorganized.

I have spent some days trying to find a simple solution but could not come up with one :/

[jneem]

Thanks for bringing this up, because it's reminded me that I intended two write down some thoughts on the interior mutability/borrowing/re-entrancy issue (which comes up in all platforms). However, I think that the specific example that you've given isn't really a problem. Unless I misunderstood, I believe you're conflating shared borrows like &self (which can be taken an unlimited number of times simultaneously) and the "interior-mutability" kind of borrows used in RefCell (which panic at runtime if taken re-entrantly). The WindowHandle::invalidate doesn't take any of these, so there's no issue if WindowHandle::invalidate causes another re-entrant call into WindowHandle.

[jneem]

Ah, ok, so I'd be willing to bet that the previous borrow came from handling the mouse click that triggered the minimize -- see #1068 for a more detailed description. Is there a way on windows to trigger a WM_SIZE asynchronously? Because that would fix it; otherwise, you might need a solution like the one described in #1068.

[jneem]

Yeah, this might be easier once #1068 is fixed. Alternatively, I did a bit of quick googling, and it looks like maybe SetWindowPos with the SWP_ASYNCWINDOWPOS flag might do what you want? The docs aren't really clear on what happens when you do it in the same thread as the window...

[jneem]

I'd feel better if this was looked at by someone more familiar with windows, but here are some comments anyway.

[rhzk]

Do you think it would be better to use display points? I feel like that could be less accurate and maybe problematic across monitors.

I will take a look at linux and mac later, work_rect might only be a thing that Windows reports, in that case I suppose it could just be removed all together. Its not that useful anyway.

Thanks for the feedback, I will take a look at it next week.

[jneem]

I'm not sure that display points is the right thing (actually, I'm not even sure how we plan to handle two monitors with different DPI in druid, generally speaking). What I'm sure of is that the units should be documented on all the public API, and also we need to think about (and document) what the position means in the presence of multiple monitors (like, is it per-monitor or is it a coordinate on some larger "virtual" screen).

It might also be worth taking a step back and documenting what we expect the screen module to be used for, and why. I'm probably a bad person for understanding the big picture here, because I usually use a tiling window manager and so my windows get very little say on how they get positioned...

[rhzk]

This is problematic for open_file_sync() and save_as_sync() as they need to return Option<FileInfo>

Dont know how we can handle get_file_dialog_path() here and have the return value in WindowHandle.

Maybe we could have people supply a callback/closure that gets called when the file is opened/saved ?

[jneem]

Rather than add a callback, I'd suggest adding methods to the WinHandler trait, one for every blocking operation that needs a "reply".

[rhzk]

Should it then trigger a Event? That would work for widgets, but not for button closure e.g:

let button = Button::new("Select File").on_click(|ctx, _data: &mut String, _env| {
        let i = ctx
            .window()
            .clone()
            .open_file_sync(FileDialogOptions::new());
    });

[jneem]

Hm, I might have changed my mind: a callback has certain advantages. But on the other hand, the two-call protocol is being introduced in #1039, and I think it's important to be consistent. @xStrom?

The way the two-call protocol would work, is that druid would call WindowHandle::open_file_dialog (or some name like that) and then when the file dialog is closed, druid-shell will call WinHandler::file_dialog_finished (or some name like that). Druid's implementation of WinHandler will take care of sending the OPEN_FILE (or OPEN_PANEL_CANCELLED, once #1061 lands) command down the widget tree.

Then we would deprecate open_file_sync. Users of druid should open files by sending a SHOW_OPEN_PANEL command and then listening for a OPEN_FILE command.

[jneem]

Sorry for the spam; I just realized the problem with a callback: it's hard to do anything useful in a callback, because most of the useful things (like modifying the data, or sending a command) won't pass the borrow checker.

[rhzk]

Agreed, We can hold of on changing open_file_sync() and save_as_sync() for later, does not have to be in this PR.

[rjwittams]

@rhzk Are you still looking at this? I did a half arsed version of some of this for popups/combos/tooltips on the mac. I will branch off your branch - so if you do look again, probably best not to force push

[rjwittams]

I think it would make sense for this to be a Rect - the bounding box of all monitors in virtual screen space.
On the mac at least you can have screens with origins at negative coordinates vs the 'main screen' which is always at 0, 0 ( bottom left corner as mac coordinates are upside down .. )

[rhzk]

Just to make sure I understand you correctly, lets say we have a virtual screen where the rect would be
top = -100, left = -100, bottom = 0, right = 0. currently this function would return a Size(100,100) but instead you suggest it should return the Rect(-100, -100, 0, 0) ?

[rjwittams]

I think so - that way you can still get the size (off the rect) but also know the extents of virtual screen coordinates

[raphlinus]

These changes look good, though I haven't done any testing yet; I'm planning to do that while merging for 1191. Thanks!

[raphlinus]

I ran this on Windows 10 and unfortunately the dpi scaling on the calc example is broken there. It seems like created window size is not being scaled up 2x.

[raphlinus]

Here's a screenshot of calc on Windows 10 with a 2x hi-dpi monitor:

It's fine at 1x. Also, the regression was introduced at 11838fc; the version before that did not exhibit the problem.

[rhzk]

Yeah I found the issue, it was related to dpi changes i did to support 8.1. Will fix it asap

[rhzk]

Should be resolved now

[raphlinus]

Unfortunately the "parse" example now seems to have a very tiny window. The "flex" example seems mostly fine though. I say "mostly" because at 2x dpi it starts up with some kind of incorrect rendering (I didn't analyze it) then flash to correct rendering quickly. I'm ok with that being addressed in a followup though.

The flashing doesn't happen in master. I didn't do any serious bisection to figure out which patch caused the regression.

[rhzk]

So many edge cases, a solution to one causes a problem for another..

Thanks for the report, should be fixed

[rhzk]

Yeah the flashing occurs because I need the hwnd to apply DPI correctly, and to get the hwnd I need to create the window first. I tried some solutions for this earlier that did not work, so I will need some time to find a good solution.

[raphlinus]

This is still not perfect. For windows created with default size (as in the "parse") example, it is initially created with a reasonable size, then a resize to a size that's 2x a reasonable size. For windows with given size (as in "flex") it's initially the 1x, ie half the intended size, then it resizes to 2x.

I'm going to click "approved" because this is a usable state and I really want to unblock other work, and think the fix probably won't cause conflicts. I remember researching this months ago and realizing it's a complicated problem - you don't really know what the dpi is until you know what monitor the window will appear on. This is one reason I didn't take on dynamic dpi switching back then :) If I were fixing this myself, I'd probably use the system dpi as a first guess, but that might create flashing and other issues in mixed-dpi configurations.

I'll start the simpler_d2d merge as soon as this is merged.

Thanks!

[raphlinus]

I've done some digging and see the problem pretty clearly: the handle.set_size(handle.get_size()) at the end of build() is taking a pixel size and interpreting it as scaled pixels. The fundamental problem here is that self.size is not a real Size value, but rather may have CW_USEDEFAULT values in it. A workable fix is to call handle.set_size(self.size) but only when it's a valid value.

That leaves a couple problems, one is the flashing, the other is that there's functionality for setting only one of width and height that we're not surfacing, but those are much smaller potatoes.

[rhzk]

Yeah that is the problem, currently the window is created with the specified size without any dpi-scaling, then we get the DPI for the window and fix the scaling. There is currently 2 problems with this, one being the "flashing" and the other being the size windows suggest as a default size, as this ends up way to big when scaled.

handle.get_size() returns the size in pixels and not "CW_USEDEFAULT", the problem you reported earlier with the parse window being tiny was that I original had handle.set_size(self.size) and that had CW_USEDEFAULT in it (which is a negative value causing the window to be 0x0 in size)

[raphlinus]

If you call SetWindowPos synchronously at the end of the build function I'm not seeing any flashing, but calling WM_CREATE with a wrong value still doesn't feel entirely right.

[rhzk]

I think there is still flashing, it just happens very fast. Ideally I want to avoid it all together.

I can prevent any dpi scaling at startup if CW_USEDEFAULT is used, but if the window is created with CW_USEDEFAULT at lets say 100% DPI, and then later on DPI is changed to 200% it will get scaled. I dont think we should not scale it in that case, but that would make it weird to not scale it in the first case...

Anyway I need some rest :) Will look at it tomorrow

[raphlinus]

FWIW I looked at the winit code, and they seem to create the window using CW_USEDEFAULT for all geometry, then use SetWindowPos after window creation to fix up the size.

[raphlinus]

My read of the code is that we'll still get a good size for the window on dpi change even if we start with CW_USEDEFAULT and don't ever set it explicitly, because it's based on the "suggested size" from the WM_DPICHANGE message. But we can't test that while Direct2D is all crashy.

[raphlinus]

As discussed on Zulip, I'm going to merge this now even though it has some rough ends, then merge #1191 on top of it, and propose a PR to fix the size behavior, all in the interest of getting master to a reasonably non-broken state quickly. Thanks for all the work on this PR, and apologies if that steps on your toes at all.