Change placement of new floating windows

[hbatagelo]

Fixes #3175 using a combination of cascading and random positioning.

Rather than replacing the original algorithm, it has been extended. If the new optically centered window shares the same top-left position as an existing window within a margin of tolerance, the new window is cascaded both vertically and horizontally. Following cascading, if any part of the window extends beyond the display area, it is randomly placed within a "feasible placement region" defined as the morphological erosion of the display region by the window region. If this region is empty, the window is resized horizontally and/or vertically to fit the display area and is also moved to the top and/or left of the display area.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a632621) 77.85% compared to head (f4b238a) 77.87%.

Files	Patch %	Lines
src/miral/basic_window_manager.cpp	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3187      +/-   ##
==========================================
+ Coverage   77.85%   77.87%   +0.01%     
==========================================
  Files        1070     1080      +10     
  Lines       74893    75360     +467     
==========================================
+ Hits        58311    58683     +372     
- Misses      16582    16677      +95     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AlanGriffiths]

I'm not sure there is a good algorithm for the initial placement of windows (if there were someone would likely have implemented it already).

My immediate reaction is that this algorithm involves a lot of work being done for marginal benefit.

1. I don't see any benefit of building a hashmap of the top-left corners of all windows (including hidden ones, tool tips, menus, ...).
   1.1. The cost of building a hashmap cannot be amortised over many lookups.
   1.2. Only visible, un-occluded, top-level windows need be considered.

2. Once the if (!display_area.contains(...)) check activates the placement becomes weird.

OTOH I'm making the above remarks without a good concrete suggestion.

Here are some (not necessarily good) thoughts:

Likely, WindowManagerTools::window_at(topleft) is simpler and good enough to detect duplicate placements (with a comparison of top_lefts).

It isn't necessary to form a cascade of new windows across the display. Just taking a rectangle and trying the centre and each corner in turn will provide a big improvement.

[hbatagelo]

Okay, I'm good with a simpler algorithm, checking the center and corners instead of cascading. Should I keep random placement as a fallback, and the resizing when the window is bigger than the display area?

[AlanGriffiths]

If you're only checking positions against visible windows then it is unlikely that all of the five positions are occupied. (Because, for example, one at NW probably occludes ones at centre, SW, SE and NE.) So fallback the centre is probably good enough, and less confusing than "random".

[AlanGriffiths]

There are a lot of tests failing

[AlanGriffiths]

There are a lot of tests failing

To run the tests locally:

cmake --build <build directory> --target ptest

[AlanGriffiths]

I tried installing and running this:

snap install mir-test-tools_pr3187 --channel=22/edge/mir-pr3187
WAYLAND_DISPLAY=wayland-99 mir-test-tools_pr3187.demo-server&
WAYLAND_DISPLAY=wayland-99 gnome-chess&

But see the error:

[2024-01-16 15:02:02.461418] < -warning- > frontend:Wayland: Exception processing Surface::commit() request: Dynamic exception type: std::out_of_range
std::exception::what: map::at

error in client communication (pid 25499)
Gdk-Message: 15:02:02.461: Error 71 (Protocol error) dispatching to Wayland display.

[AlanGriffiths]

I tried installing and running this:

Building locally doesn't show this effect (and the code looks like it shouldn't). Possibly, the snap store has an old version...

[AlanGriffiths]

The offset used is (obviously) arbitrary and may seem big or small depending on the screen dimensions. (Maybe a fraction of the screen width/height would be better?)

I think the conditional is a bit hard to read, but isn't wrong. I've suggested a rework that might be easier to read (but I am probably biased).

Anyway, does what was asked. So I'll approve and see if anyone else has an opinion.

(Feel free to land with or without my suggestion)

[mattkae]

I found a bug that is maybe unrelated to this. It looks like during the startup of miral-shell, all of the windows end up in the same position. In the attached video, I am opening up multiple windows in a row, but only the fourth one gets places at the offset. WDYT @AlanGriffiths , is this blocking?

Screencast.from.01-16-2024.12.15.06.PM.webm

[hbatagelo]

I think these first windows are placed at the same position because the splash screen is being displayed. Maybe the splash screen is picked by window_at instead of the terminal windows. I'll check.

[mattkae]

I think these first windows are placed at the same position because the splash screen is being displayed. Maybe the splash screen is picked by window_at instead of the terminal windows. I'll check.

Oh good call, that might be it. This could be a separate bug for the splash screen though. If so, we can do it separately :)

[AlanGriffiths]

WDYT @AlanGriffiths , is this blocking?

I to assume window_at is returning the splash screen (because it is on top). Which is a special case I wouldn't block on (you might have a different opinion).

[AlanGriffiths]

Oh good call, that might be it. This could be a separate bug for the splash screen though. If so, we can do it separately :)

Well, if it is a bug, it is in window_at() not the splash screen.

[mattkae]

I to assume window_at is returning the splash screen (because it is on top). Which is a special case I wouldn't block on (you might have a different opinion).

I think it's non-blocking too. As you said, probably a bug in window_at that we can record and fix later

[mattkae]

I approve, assuming that the splash screen bug is unrelated to your work. We shouldn't block on it, I assume

[hbatagelo]

I tested locally and can confirm that window_at (wrongly) selects the splash screen. Since it is a top-level, freestyle, fullscreen window, placement is allowed at the initial position.

[AlanGriffiths]

Well, if it is a bug, it is in window_at() not the splash screen.

I change my opinion: it is the splash window. This should be transparent to input, and that would ensure it isn't returned by surface_at.

But this is pre-existing (e.g. you can't drag a window until the splash animation ends), and not very important.