D3484: Center systemmonitor window properly on multi-screen setup

broulik added inline comments. INLINE COMMENTS > valeriymalov wrote in 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 conse

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov added inline comments. INLINE COMMENTS > broulik wrote in ksystemactivitydialog.cpp:79 > Is it intentional you removed the `KeepAbove`? Yes, per my interpretation of the comment that application shouldn't force it's window properties like that because it interferes with WM settings

D3484: Center systemmonitor window properly on multi-screen setup

broulik added inline comments. INLINE COMMENTS > ksystemactivitydialog.cpp:79 > -if (keepAbove) { > -KWindowSystem::setState(winId(), NET::KeepAbove); > -} Is it intentional you removed the `KeepAbove`? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kd

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov added a comment. 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;

D3484: Center systemmonitor window properly on multi-screen setup

cfeck added a comment. > 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. R

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov added a comment. 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 applicat

D3484: Center systemmonitor window properly on multi-screen setup

cfeck added a comment. > 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

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov added a comment. 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

D3484: Center systemmonitor window properly on multi-screen setup

cfeck added a comment. 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). REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3484 To: valeriymalov, #plasma_workspaces,

D3484: Center systemmonitor window properly on multi-screen setup

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R120:ad5b9e98b1f2: Center systemmonitor window properly on multi-screen setup (authored by valeriymalov). REPOSITORY R120

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov updated this revision to Diff 29239. valeriymalov added a comment. another rebase REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3484?vs=28410&id=29239 BRANCH master REVISION DETAIL https://phabricator.kde.org/D3484 AFFECTED FIL

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov updated this revision to Diff 28410. valeriymalov edited the summary of this revision. valeriymalov added a comment. Rebased from master & fed through arcanist Any objections if I'll just land it soon? REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://ph

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov updated this revision to Diff 21043. valeriymalov edited the summary of this revision. valeriymalov added a comment. 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

D3484: Center systemmonitor window properly on multi-screen setup

aacid added a comment. Should we commit this then? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3484 To: valeriymalov, #plasma_workspaces, aacid, mart Cc: davidedmundson, sebas, aacid, graesslin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,

D3484: Center systemmonitor window properly on multi-screen setup

sebas added inline comments. INLINE COMMENTS > davidedmundson wrote in ksystemactivitydialog.cpp:78 > > > > This doesn't work with scaling > > Yes it does. Right, was actually wondering right after I submitted. In that case, I'm not all that opposed. REPOSITORY R120 Plasma Workspace REVI

D3484: Center systemmonitor window properly on multi-screen setup

davidedmundson added inline comments. INLINE COMMENTS > sebas wrote in ksystemactivitydialog.cpp:78 > This doesn't work with scaling, different font settings or different widget > themes. Hardcoded sizes are a no-no in general. > This doesn't work with scaling Yes it does. REPOSITORY R120 P

D3484: Center systemmonitor window properly on multi-screen setup

sebas added inline comments. INLINE COMMENTS > ksystemactivitydialog.cpp:78 > { > -saveDialogSettings(); > -event->accept(); > -} > - > - > -void KSystemActivityDialog::reject () > -{ > -saveDialogSettings(); > -QDialog::reject(); > -} > - > -void KSystemActivityDialog::saveDialo

D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov updated this revision to Diff 15251. valeriymalov added a comment. 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 minimumS

[Differential] [Commented On] D3484: Center systemmonitor window properly on multi-screen setup

aacid added a comment. So we should remove the setGeometry call altogether then? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3484 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: valeriymalov, #plasma_workspaces, mart

[Differential] [Commented On] D3484: Center systemmonitor window properly on multi-screen setup

graesslin added a comment. In https://phabricator.kde.org/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

[Differential] [Requested Changes] D3484: Center systemmonitor window properly on multi-screen setup

aacid requested changes to this revision. aacid added a comment. This revision now requires changes to proceed. @graesslin so how do we move forward with this, i.e. how can this work with Wayland? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3484 EMAIL P

[Differential] [Commented On] D3484: Center systemmonitor window properly on multi-screen setup

graesslin added a comment. 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 "s

[Differential] [Commented On] D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov added a comment. 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

[Differential] [Commented On] D3484: Center systemmonitor window properly on multi-screen setup

graesslin added a comment. 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. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL htt

[Differential] [Accepted] D3484: Center systemmonitor window properly on multi-screen setup

mart accepted this revision. mart added a reviewer: mart. This revision is now accepted and ready to land. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3484 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: vale

[Differential] [Request, 2 lines] D3484: Center systemmonitor window properly on multi-screen setup

valeriymalov created this revision. valeriymalov added a reviewer: Plasma: Workspaces. valeriymalov set the repository for this revision to rPLASMAWORKSPACE Plasma Workspace. valeriymalov added a project: Plasma: Workspaces. Restricted Application edited projects, added Plasma; removed Plasma: Wo