D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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.

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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3484

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: broulik, cfeck, davidedmundson, sebas, aacid, graesslin, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, 
mart

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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 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.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3484?vs=8469&id=15251

REVISION DETAIL
  https://phabricator.kde.org/D3484

AFFECTED FILES
  systemmonitor/ksystemactivitydialog.cpp
  systemmonitor/ksystemactivitydialog.h

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: aacid, graesslin, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D7558: kcms/input: Allow WheelScrollLines = 0; some code cleanup

[Valeriy Malov]

valeriymalov created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  This should allow user to disable scrollwheel in KDE/Qt apps
  This does not seem to affect non-Qt apps, e.g. Google Chrome, but they
  ignore WheelScrollLines setting anyway and require some other solution
  
  BUG: 192427
  
  Some code cleanup:
  Reorder headers
  Replace RIGHT/LEFT_HANDED defines with enum
  Replace deprecated KStandardDirs
  Remove unescessary explicit casts
  Remvoe old warning
  Replace NULL with nullptr
  Fix header guards

TEST PLAN
  builds, can disable scroll in kde apps, switch left/right hand still works

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D7558

AFFECTED FILES
  kcms/input/main.cpp
  kcms/input/mouse.cpp
  kcms/input/mouse.h

To: valeriymalov, #plasma
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, lukas

D7558: kcms/input: Allow WheelScrollLines = 0; some code cleanup

[Valeriy Malov]

valeriymalov added a comment.


  It does work with both evdev-controlled pointers, libinput-controlled 
pointers and even on Wayland. As far as I understand, it just controls how Qt 
apps react to single "full" scroll event. It's doesn't affect system scroll 
speed, although takes it in account.
  
  X11 doesn't seem to expose a good mechanism for controlling system-wide 
scroll speed anyway, judging by xinput:
  
  - evdev exposes "Scroll Distance", which can only slow down the scrolling 
whole number of times and makes GTK+ apps behave weird
  - libinput doesn't seem to expose any options related to scroll speed at all?
  
  So this can't be done, this shouldn't be done or this should be done other 
way? Should this just be a separate checkbox "disable scrolling in Qt 
applications" or it's a completely useless feature?
  
  Also, anything on the other commit (refactor changes)? MouseConfig::apply() 
eventually needs to be split into multiple parts since right now it's a lot of 
evdev code and not all of it works on libinput devices. If the commit doesn't 
belong in this diff, I can put it with eventual X11 libinput support then.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D7558

To: valeriymalov, #plasma, #vdg
Cc: graesslin, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D7890: add size hint to KScreen KCM

[Valeriy Malov]

valeriymalov created this revision.
valeriymalov added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  This should help kcmshell opening this kcm
  at a reasonable size without forcing it

REPOSITORY
  R104 KScreen

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D7890

AFFECTED FILES
  kcm/src/kcm_kscreen.cpp
  kcm/src/kcm_kscreen.h

To: valeriymalov, #plasma
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D7890: add size hint to KScreen KCM

[Valeriy Malov]

valeriymalov added a comment.


  Not really

REPOSITORY
  R104 KScreen

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D7890

To: valeriymalov, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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 that defaults to 0)
  Rolled it back but but without saving/loading window settings

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3484?vs=15251&id=21043

REVISION DETAIL
  https://phabricator.kde.org/D3484

AFFECTED FILES
  systemmonitor/ksystemactivitydialog.cpp
  systemmonitor/ksystemactivitydialog.h

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: davidedmundson, sebas, aacid, graesslin, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

D7558: kcms/input: Allow WheelScrollLines = 0; some code cleanup

[Valeriy Malov]

valeriymalov abandoned this revision.
valeriymalov added a comment.


  kcm_input seems to be getting proper refactor in 
https://phabricator.kde.org/D8168

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D7558

To: valeriymalov, #plasma, #vdg
Cc: graesslin, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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://phabricator.kde.org/D3484?vs=21043&id=28410

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D3484

AFFECTED FILES
  systemmonitor/ksystemactivitydialog.cpp
  systemmonitor/ksystemactivitydialog.h

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: davidedmundson, sebas, aacid, graesslin, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, apol, mart

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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 FILES
  systemmonitor/ksystemactivitydialog.cpp
  systemmonitor/ksystemactivitydialog.h

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: davidedmundson, sebas, aacid, graesslin, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, apol, mart

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3484?vs=29239&id=29240

REVISION DETAIL
  https://phabricator.kde.org/D3484

AFFECTED FILES
  systemmonitor/ksystemactivitydialog.cpp
  systemmonitor/ksystemactivitydialog.h

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: davidedmundson, sebas, aacid, graesslin, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, apol, mart

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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 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?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3484

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: cfeck, davidedmundson, sebas, aacid, graesslin, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3484

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: cfeck, davidedmundson, sebas, aacid, graesslin, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

D3484: Center systemmonitor window properly on multi-screen setup

[Valeriy Malov]

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; 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: F5800781: restore_size.patch 


REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3484

To: valeriymalov, #plasma_workspaces, aacid, mart
Cc: cfeck, davidedmundson, sebas, aacid, graesslin, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart

D12837: Split replaceAccount from addAccountToCache

[Valeriy Malov]

valeriymalov created this revision.
valeriymalov added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
valeriymalov requested review of this revision.

REVISION SUMMARY
  We were accidentally overwriting first account in the model with
  currently logged in user after polling AccountsService
  
  BUG: 336994

TEST PLAN
  check if kcmshell5 user_manager lists mutliple users on cold boot

REPOSITORY
  R128 User Manager

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12837

AFFECTED FILES
  src/lib/accountmodel.cpp
  src/lib/accountmodel.h

To: valeriymalov, #plasma
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D12837: Split replaceAccount from addAccountToCache

[Valeriy Malov]

valeriymalov added a comment.


  > We have addAccountToCache which has an argument pos, which determines if 
we're adding or replacing.
  
  I've actually removed this bit, addAccountToCache now only adds accounts 
(either inserts at pos or appends)
  
  Though I agree that current code is probably a bit fragile, maybe the model 
can be changed the way there's no need in manual ordering or at least storing 
"fake user". If that seems like a better approach I can look into that.

REPOSITORY
  R128 User Manager

REVISION DETAIL
  https://phabricator.kde.org/D12837

To: valeriymalov, #plasma, davidedmundson
Cc: davidedmundson, ngraham, rdieter, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12837: Split replaceAccount from addAccountToCache

[Valeriy Malov]

valeriymalov added inline comments.

INLINE COMMENTS

> davidedmundson wrote in accountmodel.cpp:361
> On second look, I might have misunderstood.
> 
> This isn't a replace, it's just making sure the current user is at the top?

Yes. Replace code was added to `addAccountToCache` at some point because of the 
"modify new user entry" logic (line 434 in the original), which broke 
`addAccountToCache` here. This patch moves the replace code out to 
`replaceAccount` so `addAccountToCache` does what the name says again.

In fact `replaceAccount` technically adds a user too but does it by replacing 
fake user entry ("+ Add new user" in the list) with it 😕 This whole bit might 
be worth refactoring to kick fake user entry out of model containers & add 
tests, but I don't think I'll be able to do it this week.

REPOSITORY
  R128 User Manager

REVISION DETAIL
  https://phabricator.kde.org/D12837

To: valeriymalov, #plasma, davidedmundson
Cc: davidedmundson, ngraham, rdieter, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12837: Split replaceAccount from addAccountToCache

[Valeriy Malov]

valeriymalov added a comment.


  In D12837#262817 , @ngraham wrote:
  
  > Any chance we could land this on the stable branch?
  
  
  Well, it does apply cleanly to Plasma/5.12 branch and it still works, but I 
don't really know if there's a special procedure for landing patches onto 
stable branches

REPOSITORY
  R128 User Manager

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12837

To: valeriymalov, #plasma, davidedmundson
Cc: davidedmundson, ngraham, rdieter, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12837: Split replaceAccount from addAccountToCache

[Valeriy Malov]

valeriymalov added a comment.


  I meant it more in terms of "not landing it right before next stable release" 
:v But if it's okay to land now I can land it right now

REPOSITORY
  R128 User Manager

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12837

To: valeriymalov, #plasma, davidedmundson
Cc: davidedmundson, ngraham, rdieter, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12837: Split replaceAccount from addAccountToCache

[Valeriy Malov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R128:ff88e24e4380: Split replaceAccount from addAccountToCache 
(authored by valeriymalov).

REPOSITORY
  R128 User Manager

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12837?vs=34027&id=34304

REVISION DETAIL
  https://phabricator.kde.org/D12837

AFFECTED FILES
  src/lib/accountmodel.cpp
  src/lib/accountmodel.h

To: valeriymalov, #plasma, davidedmundson
Cc: davidedmundson, ngraham, rdieter, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[valeriymalov (Valeriy Malov)]

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: 
Workspaces.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  QScreen may return non-zero x0/y0 for geometry in a multi-screen setup, those 
should be taken in account to center the window properly on active screen
  
  Could be related to bugs about it opening off-screen ( #368158 / #356706 )

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3484

AFFECTED FILES
  systemmonitor/ksystemactivitydialog.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: valeriymalov, #plasma_workspaces
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

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

[valeriymalov (Valeriy Malov)]

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
  
  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

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3484

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: valeriymalov, #plasma_workspaces, mart
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas