Center systemmonitor window properly on multi-screen setup
ClosedPublic

Authored by valeriymalov on Nov 24 2016, 9:49 AM.

Details

Summary

Do not force geometry or window hints when opening systemmonitor
This should let it position properly on a multi-monitor setup and reduce interference with user-set KWin preferences

BUG: 368158
BUG: 356706

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
valeriymalov retitled this revision from to Center systemmonitor window properly on multi-screen setup.
valeriymalov updated this object.
valeriymalov edited the test plan for this revision. (Show Details)
valeriymalov added a reviewer: Plasma: Workspaces.
valeriymalov set the repository for this revision to R120 Plasma Workspace.
Restricted Application edited projects, added Plasma; removed Plasma: Workspaces. · View Herald TranscriptNov 24 2016, 9:49 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart accepted this revision.Nov 24 2016, 10:56 AM
mart added a reviewer: mart.
This revision is now accepted and ready to land.Nov 24 2016, 10:56 AM

I don't like the code. Neither the one which used to be there, nor the new one. Centering on the screen is not the task of a window, but of the window manager and that code won't work on Wayland.

I'm not familiar with KDE, but it seems that's how krunner manages it's position too (method View::positionOnScreen in krunner/view.cpp), except that it doesn't use windowHandle() and derives current screen from cursor position, and takes struts in account

KWindowSystem doesn't seem to provide methods for manipulating window position, and neither I can find a method for getting screen geometry, so I have no idea how the code should look like

KRunner is different. It's not "a normal window". But the systemmonitor is a normal window.

It's just me the grumpy window manager speaking. We regularly get bug reports about window not opening on the screen the user expect. Reason for that is that apps do "smart things" like here. So if I complain in other areas I also have to complain here.

aacid requested changes to this revision.Feb 26 2017, 11:39 PM
aacid added a subscriber: aacid.

@graesslin so how do we move forward with this, i.e. how can this work with Wayland?

This revision now requires changes to proceed.Feb 26 2017, 11:39 PM
In D3484#90292, @aacid wrote:

@graesslin so how do we move forward with this, i.e. how can this work with Wayland?

It cannot. It is not the business of Windows to place themselves. That is also on X11 bad and breaking with e.g. filling window managers. It is also one of the main reasons why people complain that our window management sucks for multiple monitors. Windows placing themselves on multi screen mostly means positioning in a word way from user perspective.

aacid added a comment.Feb 27 2017, 7:59 PM

So we should remove the setGeometry call altogether then?

valeriymalov edited edge metadata.

So I've decided to revisit this and ended up deleting most of the code, dunno if I went overboard here.

Replaced resize and setGeometry calls with sizeHint() for the window. I've also removed call to minimumSizeHint() that as far as I understand calculates minimum size hint, but I don't think we need it anymore since sizeHint is set.

Got rid of code that saves/restores & applies "keep above" to avoid further interference with WM, since Kwin can do that on it's own.

Removed timer that was used to set window size and moved dbus registration from it to constructor. I know we use dbus to close this window and it doesn't break that, but otherwise I don't understand why dbus registration was timed, systemmonitor doesn't really export anything special.

So now by default the dialog opens centered on current display on it's own for some reason, which is close to original/intended behavior, and should be more manageable by kwin.

sebas added a subscriber: sebas.Jun 7 2017, 1:48 PM
sebas added inline comments.
systemmonitor/ksystemactivitydialog.cpp
78

This doesn't work with scaling, different font settings or different widget themes. Hardcoded sizes are a no-no in general.

davidedmundson added inline comments.
systemmonitor/ksystemactivitydialog.cpp
78

This doesn't work with scaling

Yes it does.

sebas added inline comments.Jun 7 2017, 1:50 PM
systemmonitor/ksystemactivitydialog.cpp
78

Right, was actually wondering right after I submitted. In that case, I'm not all that opposed.

aacid added a comment.Jul 28 2017, 8:42 AM

Should we commit this then?

valeriymalov edited the summary of this revision. (Show Details)

I've just realized that config save/load that I've deleted is pretty important since it also configures the KSysGuardProcessList widget (including update interval that defaults to 0)
Rolled it back but but without saving/loading window settings

valeriymalov edited the summary of this revision. (Show Details)

Rebased from master & fed through arcanist

Any objections if I'll just land it soon?

another rebase

This revision was not accepted when it landed; it landed in state Needs Review.Mar 11 2018, 2:36 PM
This revision was automatically updated to reflect the committed changes.
cfeck added a subscriber: cfeck.Apr 2 2018, 2:12 PM

But it does no longer remember window size now (and for whatever reason, I cannot get the kwin rules to force an initial window size to work).

In D3484#238571, @cfeck wrote:

But it does no longer remember window size now (and for whatever reason, I cannot get the kwin rules to force an initial window size to work).

I thought the consensus was that the window shouldn't set it's properties (size and keepabove) and it's up to Kwin to set them
I've booted into neon dev unstable and kwin from master seems to struggle applying rules (I only took a quick peek, though, I'll need some time to test stable kwin with plasma-workspace from master), does Kwin apply rules to other windows fine for you?

cfeck added a comment.Apr 4 2018, 3:37 PM

I thought the consensus was that the window shouldn't set it's properties (size and keepabove) and it's up to Kwin to set them

It is the application that sets the window size. Since the initial size is likely wrong, applications usually remember sizes of windows and important dialogs. You removed that code that remembers the window size.

does Kwin apply rules to other windows fine for you?

For whatever reason, kwin refuses to save changes to the rules file. I had to kill kwin, edit .config/kwinrulesrc manually, and restart kwin to add a size rule for the systemmonitor. The rule now works.

In D3484#239893, @cfeck wrote:

It is the application that sets the window size. Since the initial size is likely wrong, applications usually remember sizes of windows and important dialogs.

If the application will set it's own geometry, then it'll still cause conflicts with (potential) KWin rules, e.g. Kwin applied it's size rule → systemmonitor resized itself → it looks like kwin failed to apply the rule. That's how I understood very first comments to the diff.
I can try & restore previous behavior just by saving window size as a size hint instead of letting the application resize itself, if that works then it should make both parties happy.

cfeck added a comment.Apr 6 2018, 7:35 PM

If the application will set it's own geometry, then it'll still cause conflicts with (potential) KWin rules

No, the KWin rules are a mean to override (or ignore) what applications set. An application shouldn't rely on KWin rules to deliver the user experience.

I've tried to restore old behavior, so:

  • If I run KWindowConfig::restoreWindowSize from dialog's constructor, it doesn't work on multi-monitor setup very well. windowHandle()→screen() seems to be set to leftmost screen (which isn't even primary in my case; is this a bug?), so restoreWindowSize loads saved size for same display no matter on which display the window is opened. Which makes resizing the dialog on any other screen has no effect on the next time it's opened.
  • If I run KWindowConfig::restoreWindowSize in a timer (like it used to be), it just messes up window positioning because the window is resized after it's been centered by KWin. Probably would mess up KWin rules too.

Patch to demonstrate the issue:

broulik added inline comments.
systemmonitor/ksystemactivitydialog.cpp
79

Is it intentional you removed the KeepAbove?

valeriymalov added inline comments.Tue, Aug 28, 12:18 AM
systemmonitor/ksystemactivitydialog.cpp
79

Yes, per my interpretation of the comment that application shouldn't force it's window properties like that because it interferes with WM settings.

There doesn't really seem to be a consensus if that's right, though? From what I understand there doesn't seem to be any API to set preferred window geometry and keepabove AND respect specific kwin settings.

broulik added inline comments.Tue, Aug 28, 6:54 AM
systemmonitor/ksystemactivitydialog.cpp
79

setWindowFlag(Qt::WindowStaysOnTopHint)?

abetts added a subscriber: abetts.Tue, Aug 28, 2:27 PM