D9627: Use combobox to choose shadow size and use more appropriate values for menu & tooltip shadow sizes

[Henrik Fehlauer]

rkflx accepted this revision.

REPOSITORY
  R31 Breeze

BRANCH
  master

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

To: ngraham, #vdg, #breeze, hpereiradacosta, abetts, rkflx
Cc: rkflx, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9627: Use combobox to choose shadow size and use more appropriate values for menu & tooltip shadow sizes

[Nathaniel Graham]

ngraham marked 3 inline comments as done.
ngraham added a comment.


  I'm agnostic about the `default` style, but I do see the utility of putting 
it on top. I'll leave it that way unless anybody has strong objections.
  
  Is this shippable now?

REPOSITORY
  R31 Breeze

BRANCH
  master

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

To: ngraham, #vdg, #breeze, hpereiradacosta, abetts
Cc: rkflx, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9627: Use combobox to choose shadow size and use more appropriate values for menu & tooltip shadow sizes

[Nathaniel Graham]

ngraham updated this revision to Diff 24854.
ngraham added a comment.


  Handle inappropriately high numeric values set in pre-existing breezerc files

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9627?vs=24801&id=24854

BRANCH
  master

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

AFFECTED FILES
  kdecoration/breezedecoration.cpp
  kdecoration/breezesettingsdata.kcfg
  kdecoration/config/breezeconfigwidget.cpp
  kdecoration/config/ui/breezeconfigurationui.ui
  kstyle/breeze.kcfg
  kstyle/breezeshadowhelper.cpp

To: ngraham, #vdg, #breeze, hpereiradacosta, abetts
Cc: rkflx, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9627: Use combobox to choose shadow size and use more appropriate values for menu & tooltip shadow sizes

[Henrik Fehlauer]

rkflx added a comment.


  Thanks for the review and your help ;)

INLINE COMMENTS

> hpereiradacosta wrote in breezedecoration.cpp:643
> Matter of taste. 
> Some people (including me), like to have the default (fallback) choice first. 
> Some others like to have it last.
> Some, inline.
> There is no c++ rule about this as far as I know.

Fair enough. However, the other `defaults` in the same file are ordered already…

> hpereiradacosta wrote in breezeconfigwidget.cpp:91
> No
> Only the first should be ShadowVeryLarge
> The second (l92) should remain "ShadowLarge", or rather whatever the default 
> shadow size we want. 
> The logic behind this is that if the shadowSize is "invalid" (meaning: larger 
> than the largest possible value), you fallback to the default value. 
> This way we gracefully reset all previously set sizes (from spinbox) to the 
> default size.

Oops, I was sloppy on the second one. (I guess the root problem the int/enum 
conversion kcfg requires? But that's OT)

REPOSITORY
  R31 Breeze

BRANCH
  master

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

To: ngraham, #vdg, #breeze, hpereiradacosta, abetts
Cc: rkflx, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9627: Use combobox to choose shadow size and use more appropriate values for menu & tooltip shadow sizes

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Other than that ... Ship it ! I'm fine with the change and happy to maintain 
it in the future.

INLINE COMMENTS

> rkflx wrote in breezedecoration.cpp:643
> Ordering by size would be more readable, `default` doesn't have to sit at the 
> top (it should just tag the currently favoured default option).

Matter of taste. 
Some people (including me), like to have the default (fallback) choice first. 
Some others like to have it last.
Some, inline.
There is no c++ rule about this as far as I know.

> rkflx wrote in breezeconfigwidget.cpp:91
> Both should be `ShadowVeryLarge`, otherwise when reopening the config (as 
> well as restarting KWin) `Large` would be shown despite setting a bigger 
> shadow beforehand.

No
Only the first should be ShadowVeryLarge
The second (l92) should remain "ShadowLarge", or rather whatever the default 
shadow size we want. 
The logic behind this is that if the shadowSize is "invalid" (meaning: larger 
than the largest possible value), you fallback to the default value. 
This way we gracefully reset all previously set sizes (from spinbox) to the 
default size.

> rkflx wrote in breezeshadowhelper.cpp:57
> Please order by size here, too.

idem

REPOSITORY
  R31 Breeze

BRANCH
  master

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

To: ngraham, #vdg, #breeze, hpereiradacosta, abetts
Cc: rkflx, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9627: Use combobox to choose shadow size and use more appropriate values for menu & tooltip shadow sizes

[Henrik Fehlauer]

rkflx added a comment.


  In https://phabricator.kde.org/D9627#186696, @rkflx wrote:
  
  > I won't need to test again, as the screenshots clearly show it working fine 
;)
  
  
  Never trust a screenshot…

INLINE COMMENTS

> breezedecoration.cpp:643
> +{
> +default:
> +case InternalSettings::ShadowLarge: shadowSize = 64; break;

Ordering by size would be more readable, `default` doesn't have to sit at the 
top (it should just tag the currently favoured default option).

> breezeconfigwidget.cpp:91
>  // load shadows
> -m_ui.shadowSize->setValue( m_internalSettings->shadowSize() );
> +if( m_internalSettings->shadowSize() <= 
> InternalSettings::ShadowLarge ) m_ui.shadowSize->setCurrentIndex( 
> m_internalSettings->shadowSize() );
> +else m_ui.shadowSize->setCurrentIndex( InternalSettings::ShadowLarge 
> );

Both should be `ShadowVeryLarge`, otherwise when reopening the config (as well 
as restarting KWin) `Large` would be shown despite setting a bigger shadow 
beforehand.

> breezeshadowhelper.cpp:57
> +{
> +default:
> +case Breeze::StyleConfigData::ShadowLarge: return 16;

Please order by size here, too.

REPOSITORY
  R31 Breeze

BRANCH
  master

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

To: ngraham, #vdg, #breeze, hpereiradacosta, abetts
Cc: rkflx, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9699: Keep fullscreen windows in active layer based on transients not the group

[Thomas Lübking]

luebking added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  The original implementation based the fullscreen status on the stack position 
of the window (ie. whenever a window would rise above the plain stack position 
of the FS window, it would loose the FS status, ie. top layer)
  The result was iirc that random notifications would not only show up but also 
de-fullscreen the window and also virtual desktop switches would constantly 
kill the FS state.
  
  The group/transient thing was iirc a "make this simpler" thing (assuming the 
group would be sufficient again, since the switch from stack => active killed 
the major issues and annoyances that the stack selection brought)
  
  Notice that the new patch does not cover the case of layered transient 
windows (which iirc was an issue with dolphin at the time, but pls don't nail 
me on that)
  
  The code in place is certainly wrong, but none of the linked commits actually 
removed the group check that was used in 
https://phabricator.kde.org/R108:476ca65295bfb3f0d90f535d9930250a13a8b323 (the 
other two commits predate that one)

REPOSITORY
  R108 KWin

BRANCH
  fullscreen-transient-not-group

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

To: graesslin, #kwin, #plasma, romangg
Cc: luebking, romangg, plasma-devel, kwin, iodelay, bwowk, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D9391: [effects] Add 'Fullscreen' effect

[Vlad Zagorodniy]

zzag added a comment.
Restricted Application edited projects, added Plasma; removed KWin.


  X11
  ===
  
  F5625140: fullscreen-animation-x11.mp4 
  
  Xwayland
  
  
  F5625142: fullscreen-animation-xwayland.mp4 

  
  Problems
  
  
  Seems like KWin has a bug. When a window enters/leaves fullscreen mode, there 
is a moment when the window is smaller than original(or some sort of it):
  
  With 'Fullscreen' effect turned on
  
  F5625143: fullscreen-animation-behind-scene-resizes.mp4 

  
  With 'Fullscreen' effect turned off
  
  F5625158: fullscreen-animation-behind-scene-resizes-disabled-effect.mp4 


REPOSITORY
  R108 KWin

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

To: zzag, #kwin, #plasma, graesslin
Cc: abetts, graesslin, broulik, plasma-devel, kwin, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D9699: Keep fullscreen windows in active layer based on transients not the group

[Roman Gilg]

romangg accepted this revision.
romangg added a comment.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma; removed KWin.


  I've tested it with Kate and also multiple screens, and everything worked 
fine. On Wayland I couldn't really test it, because Kate can't be made 
fullscreen (with and without the patch). VLC fullscreen worked fine though, or 
let's say it this way: I couldn't put windows on top of it with and without the 
patch. :)

REPOSITORY
  R108 KWin

BRANCH
  fullscreen-transient-not-group

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

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

D9391: [effects] Add 'Fullscreen' effect

[Vlad Zagorodniy]

zzag updated this revision to Diff 24849.
zzag added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  Updating https://phabricator.kde.org/D9391: [effects] Add 'Fullscreen' effect
  
  Re-implemented this effect. The only missing part is emitters of 
`windowFullScreenChanged`
  signal. For development I've made stub in `void 
EffectsHandlerImpl::setupClientConnections(Client* c)`:
  
cpp
connect(c, &AbstractClient::fullScreenChanged, this,
[this, c]() {
bool entered = c->isFullScreen();
QRect oldGeometry = entered
? c->geometryFSRestore()
: QRect(0, 0, 1366, 768); // 1366x768 my screen resolution 
:(
emit windowFullScreenChanged(c->effectWindow(), entered, 
oldGeometry);
});

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9391?vs=24718&id=24849

BRANCH
  effects/fullscreen

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

AFFECTED FILES
  autotests/test_builtin_effectloader.cpp
  autotests/test_plugin_effectloader.cpp
  autotests/test_scripted_effectloader.cpp
  effects/CMakeLists.txt
  effects/effect_builtins.cpp
  effects/effect_builtins.h
  effects/fullscreen/CMakeLists.txt
  effects/fullscreen/fullscreen.cpp
  effects/fullscreen/fullscreen.h
  effects/fullscreen/timeline.h
  libkwineffects/kwineffects.h

To: zzag, #kwin, #plasma, graesslin
Cc: abetts, graesslin, broulik, plasma-devel, kwin, iodelay, bwowk, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, sebas, apol, mart

D8529: Plasma change icons should exit after its work finished.

[Xuetian Weng]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:60bc30083fe4: Plasma change icons should exit after its 
work finished. (authored by xuetianweng).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8529?vs=21457&id=24848

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

AFFECTED FILES
  kcms/icons/changeicons.cpp

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

D9699: Keep fullscreen windows in active layer based on transients not the group

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  In https://phabricator.kde.org/D9699#186972, @graesslin wrote:
  
  > git blame result: The group check was introduced with 
https://phabricator.kde.org/R108:476ca65295bfb3f0d90f535d9930250a13a8b323 - 
before it was a check for transient, but the group check was there as commented 
out code. The change to not use group members was commit 
https://phabricator.kde.org/R108:2d99ef918bbd797701ac63c8e3d4d7ca05f64651 based 
on BUG 293265.
  
  
  Wrong, the change to not use group members was commit 
https://phabricator.kde.org/R108:818070d3aa92df29081a07a2787e005b812ed60d (SVN 
851204)

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: romangg, plasma-devel, kwin, iodelay, bwowk, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D9699: Keep fullscreen windows in active layer based on transients not the group

[Martin Flöser]

graesslin added inline comments.
Restricted Application edited projects, added Plasma; removed KWin.

INLINE COMMENTS

> romangg wrote in abstract_client.cpp:1155
> Just a question for understanding unrelated to this patch: why should 
> `isActiveFullScreen` return true, if `ac->screen() != screen()`? We don't 
> know if it's really active just because it's on another screen than the one 
> which the most recently activated client is on.

We keep fullscreen windows in the active layer if the active window is on a 
different screen. Consider you have on each screen a panel. On your second 
screen you have vlc playing a fullscreen video. On the first screen you want to 
quickly check something in the browser. This ensures that vlc stays above of 
the panel all the time.

REPOSITORY
  R108 KWin

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

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

D9699: Keep fullscreen windows in active layer based on transients not the group

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  git blame result: The group check was introduced with 
https://phabricator.kde.org/R108:476ca65295bfb3f0d90f535d9930250a13a8b323 - 
before it was a check for transient, but the group check was there as commented 
out code. The change to not use group members was commit 
https://phabricator.kde.org/R108:2d99ef918bbd797701ac63c8e3d4d7ca05f64651 based 
on BUG 293265.
  
  I have a feeling we don't really know what we are doing.

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: romangg, plasma-devel, kwin, iodelay, bwowk, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D9689: [WIP] Add a per-process CPU usage graph shown in the process list

[Fabian Vogt]

fvogt updated this revision to Diff 24838.
fvogt added a comment.


  Forgot to save ProcessModel.cpp before git commit.

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9689?vs=24836&id=24838

BRANCH
  history

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

AFFECTED FILES
  processui/ProcessModel.cpp
  processui/ProcessModel.h
  processui/ProcessModel_p.h
  processui/ksysguardprocesslist.cpp
  tests/processtest.cpp
  tests/processtest.h

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

D9699: Keep fullscreen windows in active layer based on transients not the group

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma; removed KWin.


  In https://phabricator.kde.org/D9699#186940, @romangg wrote:
  
  > Are there maybe other cases of clients in X, which share the same group, 
are not transient, but need to have `isActiveFullScreen` return true on any 
client if one of them is the most recent activated one?
  
  
  I don't think there are any cases. In fact this was anyway special KWin 
behavior. Have a look at 
https://standards.freedesktop.org/wm-spec/wm-spec-1.4.html and scroll all way 
down to "Stacking Order".  It only talks about transients, not client leaders.
  
  > I mean you change the behavior now from testing on the group to only 
testing on the transient property, what seems to make sense. But what was the 
reason to set `isActiveFullScreen` to true for clients sharing the same group 
in the first place?
  
  Let me try to git blame...

REPOSITORY
  R108 KWin

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

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

D9689: [WIP] Add a per-process CPU usage graph shown in the process list

[Fabian Vogt]

fvogt updated this revision to Diff 24836.
fvogt added a comment.


  Instead of updating the last entry's timestamp if the value is the same, add 
a new
  entry only if the latest entry has a certain age.
  Otherwise, the interpolation results would look differently and processes had 
different
  lengths of histroy graphs depending on the values, which looks odd.

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9689?vs=24804&id=24836

BRANCH
  history

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

AFFECTED FILES
  processui/ProcessModel.cpp
  processui/ProcessModel.h
  processui/ProcessModel_p.h
  processui/ksysguardprocesslist.cpp
  tests/processtest.cpp
  tests/processtest.h

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

D9699: Keep fullscreen windows in active layer based on transients not the group

[Roman Gilg]

romangg added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  Are there maybe other cases of clients in X, which share the same group, are 
not transient, but need to have `isActiveFullScreen` return true on any client 
if one of them is the most recent activated one?
  
  I mean you change the behavior now from testing on the group to only testing 
on the transient property, what seems to make sense. But what was the reason to 
set `isActiveFullScreen` to true for clients sharing the same group in the 
first place?

INLINE COMMENTS

> abstract_client.cpp:1155
>  // we'll also take the screen into account
> -return ac && (ac == this || ac->screen() != screen());
>  }

Just a question for understanding unrelated to this patch: why should 
`isActiveFullScreen` return true, if `ac->screen() != screen()`? We don't know 
if it's really active just because it's on another screen than the one which 
the most recently activated client is on.

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: romangg, plasma-devel, kwin, iodelay, bwowk, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D9699: Keep fullscreen windows in active layer based on transients not the group

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: KWin, Plasma.
Restricted Application added a project: KWin.
Restricted Application added subscribers: kwin, plasma-devel.
graesslin requested review of this revision.
Restricted Application edited projects, added Plasma; removed KWin.

REVISION SUMMARY
  So far a not-active fullscreen X11 window was kept in the active layer if
  the newly activated window is in the same group (that is same client
  leader). For example a fullscreen X11 kwrite window is in the active layer
  if another kwrite window is active. The two kwrite windows obviously
  don't have anything to do with each other, but are in the same group.
  
  This creates problems as it's not possible to raise other windows above
  the active not-fullscreen kwrite window. E.g. the panel is stacked below.
  
  The idea behind the check makes sense: if a fullscreen window opens
  another window (e.g. a configuration dialog) it should not be put back
  to normal layer. Thus the check is adjusted whether the new active
  window is a transient to the fullscreen window. Thus the intention is
  still the same, but does not cause the problems.
  
  As the code now does not need to differentiate between X11 and Wayland
  windows (group only on X11) the Client specific implementation is
  removed and the method unvirtual'ed.
  
  BUG: 388310
  FIXED-IN: 5.12.0

TEST PLAN
  Test passes

REPOSITORY
  R108 KWin

BRANCH
  fullscreen-transient-not-group

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

AFFECTED FILES
  abstract_client.cpp
  abstract_client.h
  autotests/integration/x11_client_test.cpp
  client.h
  layers.cpp

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

D8922: Avoid jumping of items toward right/botton when dropping

[Andras Mantia]

amantia added a comment.


  Sorry about it.

REPOSITORY
  R119 Plasma Desktop

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

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