D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Thomas Lübking]

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


  I'm not actually smiling ...
  
  Anyway, instead of using three vectors it's strongly suggest to use one 
vector (or rather array) of a struct.
  a) you can access the inner values by names
  b) it's far more elegant to define = { {1, 2, 3}, {4, 5, 6}, ...}

REPOSITORY
  R108 KWin

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

To: anemeth, #plasma, #kwin
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson, 
fredrik, ngraham, plasma-devel, kwin, #kwin, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Thomas Lübking]

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


  Why was that a problem?
  (Or how can I see the previous patch in this messy review tool which draws an 
entire core when typing...)

REPOSITORY
  R108 KWin

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

To: anemeth, #plasma, #kwin
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson, 
fredrik, ngraham, plasma-devel, kwin, #kwin, iodelay, bwowk, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, 
apol

D9879: [effects/blur] Disable texture cache on Wayland

[Thomas Lübking]

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


  On a general note, UI-wise: if there's no way to ever enable an item it 
should not be shown anyway.

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: luebking, broulik, 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

[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

D9608: [KScreen Effect] Fade opacity only for transparent windows

[Thomas Lübking]

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


  Should the lock screen not be guarded by a black layer in the compositor?
  As of the effect, maybe this should rather fade in a black layer than fade 
out everything else?

REPOSITORY
  R108 KWin

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

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

D8796: Support dynamic output enabling/disabling from KScreen

[Thomas Lübking]

luebking added a comment.


  Ignore the request as "probably temporary" and just freeze rendering?

REPOSITORY
  R108 KWin

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

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

D8145: Update pointer position whenever a window gets (un)minimized

[Thomas Lübking]

luebking added a comment.


  Why should it? The stack remains the same, just one window changes its 
visibility.

REPOSITORY
  R108 KWin

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

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

D7521: [WIP] make use of foreign protocol

[Thomas Lübking]

luebking added a comment.


  There's a major pitfall to inter-process modality:
  In case of a persistent transient (used by several clients), the leader may 
loose the focus access "forever" (because the modal window remains)
  
  Thus and in general, transients should certainly not be unconditionally 
modal, but the ability to control this attribute to the WM will be useful.
  If client A crucially relies on input in client B to proceed, it will 
typically reject internal focus distribution and input handling.
  
  This is complex enough if the client spawns a nested event loop and tries to 
pass the focus on to the foreign window, but in a multi-process case, client A 
might simply block for the I/O to gain this behavior.
  In this case a "dead" window might receive the input focus.
  
  Also the modality controls the forced visibility of the transient along the 
leader.

REPOSITORY
  R108 KWin

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

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

D7398: Don't create QWhatsThis when user presses showContextHelp button

[Thomas Lübking]

luebking added a comment.


  Probably to handle internal WhatsThis online help (ie. you click the maximize 
button of a deco and get the information that this is a maximize button) but I 
don't know either.
  Apparently even Lubos was already unsure what this was about.
  
  I'm not sure whether WhatsThis is actually still used by real users as of 
today, so I'd suggest to accidentally™ break it and see whether somebody misses 
something afterwards.

REPOSITORY
  R108 KWin

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

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

D7475: Make EffectsHandlerImpl::announceSupportProperty work without X11

[Thomas Lübking]

luebking added a comment.


  I take there's no such support announcement on native wayland at all?

REPOSITORY
  R108 KWin

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

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

D7475: Make EffectsHandlerImpl::announceSupportProperty work without X11

[Thomas Lübking]

luebking added a comment.


  Pretty uninformed consideration:
  When dealing with X11/wayland hybrids, won't one end up with two property 
indexes which are hard -if not impossible- to align (because one will be 
assigned by kwin internally and the other by the casual xwayland server)?
  Might the xwayland property even change over the runtime of kwin (is the 
xwayland server conditionally removed)?
  
  Would it in this case not be required to abstract the property query with a 
mapper in general (or abort x11 properties and always ask kwin via dbus resp. 
the effects API for a purely internally kept property)?
  
  Random sidenote:
  registerPropertyType() has a hash trap (unregistering a non-existing atom 
will insert it forever)

REPOSITORY
  R108 KWin

BRANCH
  effects-announce-support-property-no-x11

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

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

D7096: Only send active window changes to X11 root window if the X11 window changed

[Thomas Lübking]

luebking added a comment.


  Unless things changed for the worse, clients (through this function) still 
send a client message (on _NET_ACTIVE_WINDOW) - the difference between the 
regular call and the force version is the source indication being "2" ("i'm a 
pager/taskbar and know what i'm doing so please do") - but the actual property 
is still set by the WM after receiving (and honoring) such message.
  
  IOW, whenever the property on the root window is set by anything but kwin, 
anything™ has a bug. Period.
  
  This member variable will systematically get out of in a race between kwin 
and the X11 server (ie. you can be sure there're some ns - ms where 
m_activeWindow will differ from ::activeWindow()) but any other case is clearly 
misbehavior in a 3rd process (or the X11 server failing to handle the property 
update request)

REPOSITORY
  R108 KWin

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

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

D7096: Only send active window changes to X11 root window if the X11 window changed

[Thomas Lübking]

luebking added a comment.


  There's no technical advantage in a local static, only a design one. You 
prevent it from future mis-use beyond its limited purpose, because 
m_activeWindow isn't actually a "property" of RootInfo.
  If you however at some point need several instances, that's no longer an 
option (resp. you'd need a local hash which is but even uglier than the "false" 
member)
  
  Likewise this is not about protecting* the active window, but if a client 
does shit, RootInfo::activeWindow() still reflects a common idea, while 
m_activeWindow does not (which sets up the case for the above arguement to 
protect m_activeWindow against unqualified reads to query the active window - 
which i think is at least part of Davids concern)
  
  *questionale itfp, it's rather race-prone and there's just a bug in the other 
client that needs to be fixed.

REPOSITORY
  R108 KWin

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

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

D7096: Only send active window changes to X11 root window if the X11 window changed

[Thomas Lübking]

luebking added a comment.


  David has some point though - m_activeWindow *can* get out of sync (server 
error, mal... stupid client - and will be temporarily due to the async setup) 
and must not be used directly to query the active window.
  
  Since there should be only one root per process, maybe rather use a local 
static than a member (to constrain this cache to the particular function)?

REPOSITORY
  R108 KWin

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

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

D7323: Expose Cursor position to DeclarativeScripting

[Thomas Lübking]

luebking added a comment.


  2¢ - you don't have to expose the cursor position for those effects, just a 
signal that the cursor position changed and the ability to "do some" at the 
"current" cursor position (which is then resolved by the core)

REPOSITORY
  R108 KWin

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

To: davidedmundson, #plasma, graesslin
Cc: luebking, broulik, graesslin, anthonyfieroni, plasma-devel, kwin, #kwin, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, 
sebas, apol, mart, lukas

D6186: Implement software cursor in OpenGL backend

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> Kanedias wrote in scene_opengl.cpp:713
> How come! There's a condition on !m_cursorTexture for that, no? Or is 
> Cursor::cursorChanged itself called on every paint?

Sorry, I missed that the "if (!m_cursorTexture) {" condition still covers these 
calls - to my excuse: inline comments in phabricator are rather messy :-P

So the above worries are void - the only remainders are
a) possible glBlend restorage
b) null'ing m_cursorTexture on empty images to skip rendering (no idea how 
relevant that is) ie. not using the pointer vaildity as indicator for a 
required reset.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson
Cc: luebking, plasma-devel, kwin, #kwin, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein, lukas

D6186: Implement software cursor in OpenGL backend

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> scene_opengl.cpp:713
> +// handle shape update on case cursor image changed
> +connect(Cursor::self(), ::cursorChanged, this, 
> updateCursorTexture);
> +}

You still update the cursor texture with every paint() - there should be no 
need to hook this signal.
Actually, the patch got much worse:

a) you're fetching the image and reset the texture with each! paint()
b) you're creating a nerw connection to cursorChanged() with each! paint()

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson
Cc: luebking, plasma-devel, kwin, #kwin, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein, lukas

D6186: Implement software cursor in OpenGL backend

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> Kanedias wrote in scene_opengl.cpp:734
> You mean, to check whether BLEND was enabled prior to calling this func?

You might have to - and in addition possibly reset the actual blend function 
from before. Please wait for Martins comment on this, idk which assumptions can 
be safely made here.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson
Cc: luebking, plasma-devel, kwin, #kwin, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein, lukas

D6186: Implement software cursor in OpenGL backend

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> scene_opengl.cpp:699
> +// don't paint if no image for cursor is set
> +const QImage img = kwinApp()->platform()->softwareCursor();
> +if (img.isNull()) {

The entire head should probably be in some init function, not in every paint 
call.
If you go for a lazy init, you should seekt to prevent double connects (but I 
don't know whether Qt::UniqueConnection works with functors), but I'd 
discourage that approach, because it prevents shortcutting the function if 
there's no cursor image (ie. "!m_cursorTexture")

> scene_opengl.cpp:709
> +// handle shape update in case cursor image changed
> +connect(Cursor::self(), ::cursorChanged, this, [this] {
> +const QImage img = kwinApp()->platform()->softwareCursor();

At this time, this seems superfluous, because you fetch the current image with 
every paint call anyway.

> scene_opengl.cpp:711
> +const QImage img = kwinApp()->platform()->softwareCursor();
> +m_cursorTexture.reset(new GLTexture(img));
> +});

There's no .isNull() test here - in contrast to the assignment some lines above

> scene_opengl.cpp:734
> +
> +glDisable(GL_BLEND);
> +}

This needs a comment from Martin, but you might have to glGet with GL_BLEND_SRC 
and GL_BLEND_DST as well as glIsEnabled(GL_BLEND)

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson
Cc: luebking, plasma-devel, kwin, #kwin, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein, lukas

D6141: make shadows work for windows 100%width or height

[Thomas Lübking]

luebking added a comment.


  I think what David has in mind is a shadow constellation like
  
oo
||
 
  
  with the lower edges deliberately being omitted - this will cause glitchy 
rendering (because the bottom edge will be stretched over the edges) - thus 
he's looking for an entirely casual quad handling (instead of the present one 
that expects a regular shape with at best some sides being entirely omitted)
  
  Marco "just" wants to make cases legal, where a window has only edges on one 
or two opposing sides - there should maybe an additional sanity check to ensure 
this: only width OR height should be zero at some point and the other value 
should match the sum of the other axis of the invoked left/right resp. 
top/bottom edges.

INLINE COMMENTS

> scene_opengl.cpp:2267
>  
>  tx2 = topLeft.width()/width;
>  ty2 = topLeft.height()/height;

This is uncaught here (and in several other occasions but is caught in the 
other function) and apparently (at least has been) a somewhat legal  condition 
...

REPOSITORY
  R108 KWin

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

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

D5114: support for high dpi in aurorae

[Thomas Lübking]

luebking added a comment.


  Just saw this because of a bug report.
  Why was this patch approved at all?
  
  This line:
  
scaleFactor = (qreal)dpi / (qreal)96;
  
  is totally nuts. dpi is already qreal, so 96 is implicitly casted and 96.0f 
or so would have done.
  
  Overmore and far worse, the result is implicitly casted to int scaleFactor, 
ie. truncated - in doubt to 0.
  
  ---
  
  --> scaleFactor should be float/qreal, if you want and then the various 
paddings etc. should be like
  
m_borderLeft = qRound(scaleFactor * border.readEntry("BorderLeft", 
defaultBorderLeft()));
  
  In addition there should be a sanity check on the dpi return and bonus points 
if the dpi is calculated as mean of dpiX and dpiY.
  
  And best invoke Kai Uwe Broulik.

REPOSITORY
  R108 KWin

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

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

D5731: Fix regression for timestamp handling for Xwayland windows

[Thomas Lübking]

luebking accepted this revision.
luebking added a comment.
This revision is now accepted and ready to land.


  Matter of semantics only (if you would want to use the XCB_CURRENT_TIME 
symbol wrt backend abstraction matters and readability - the invalidity isn't 
implicitly explained, maybe read as bool "if (timestamp && ...") but I'm out of 
position for an informed comment on this.

REPOSITORY
  R108 KWin

BRANCH
  fix-timestamp-regression-try2-5.8

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

To: graesslin, #kwin, #plasma, luebking
Cc: sebas, luebking, plasma-devel, kwin, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, apol, lukas

D5726: Fix regression for timestamp handling for Xwayland windows

[Thomas Lübking]

luebking added a comment.


  0L is XCB_CURRENT_TIME iow "kinda invalid", so it's sane to block that in 
setX11Time()
  
  Also: a conditional early return? In a four line function? Really? ;-P

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas

D5704: Improve the x11 timestamp handling

[Thomas Lübking]

luebking added a comment.


  In https://phabricator.kde.org/D5704#106951, @cfeck wrote:
  
  > I may be totally unaware about what the changes do, but from reading the 
comments on the bug report, I had expected to see something like
  >
  > if (timestamp > m_X11Time || int(timestamp + INT_MAX / 2) < m_X11Time) ...
  
  
  There is no reasonable heuristic here.
  
  There is a reliable way to update the X11 time: ask the server (expensinve on 
the Qt5 design)
  And there's an unreliable way: track the timestamps on incoming events (for 
free)
  Times from the latter will typically be "a bit dated" and arrive in random 
order.
  
  If you get time far after the known one, the system might have hibernated or 
there might have been a network problem (if those are a remote clients) or the 
new value is just bullshit ("time()")
  If you get a time before the known one, the client might just have taken a 
longer time to send the event, you accepted a bullshit value before, this value 
is bullshit or the server time has wrapped around. Testing for a huge™ absolute 
difference *suggests* a wrap, but could also just mean some bullshit value is 
involved (and you don't know which one is true) and on the other hand, a small™ 
difference could stem from a stalled client/server connection across a wrap.
  
  One could probably keep timestamp stats and exclude outliers to protect 
agains broken clients and take "ok, everyone says it's ~120, so it's probably 
not 2147483648" (ie. m_x11Time being the new outlier) as factual wrap, but 
we're asking the server on a regular base anyway. And that time is always right.

REPOSITORY
  R108 KWin

BRANCH
  x11-timestamp-handling-5.8

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

To: graesslin, #kwin, #plasma, davidedmundson
Cc: cfeck, luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas

D5704: Improve the x11 timestamp handling

[Thomas Lübking]

luebking added a comment.


  > Another problem related to timestamp handling is KWin getting broken by 
wrong timestamps sent by applications. A prominent example is clusterssh
  
  Should have scrolled down the mail ;-)

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas

D5704: Improve the x11 timestamp handling

[Thomas Lübking]

luebking added a comment.


  This should also mitigate the "idiotic client confused X11 time with epoch" 
condition (iirc some ssh per script?), yesno?

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas

D5589: [helper] Terminate xclipboardsyncer if kwin_wayland goes down

[Thomas Lübking]

luebking added a comment.


  According to the bug kwin_wayland receives a SIGINT, not a SIGSEGV?

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas

D5589: [helper] Terminate xclipboardsyncer if kwin_wayland goes down

[Thomas Lübking]

luebking added a comment.


  This boils down to the question why the process is still lingering around. If 
the only parent/child link is actually the socket, then it's more likely to 
zombie around on a bad socket.
  In this case you can fire as many signals as you want - they'll never be 
handled (the process isn't interruptable)
  
  Otherwise QProcess would setup the child process in a way to die with the 
parent anyway. If that doesn't happen, the child may have been forked off at 
some point and in that case lost the deathsig/sigterm connection (according to 
the prctl manpage, I've actually never tried that myself)
  
  tl;dr - somebody needs to test this and ideally check *how* the process 
refuses to die (but the gdb hassle seems to indicate a zombie) and the process 
table at this point.

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas

D5589: [helper] Terminate xclipboardsyncer if kwin_wayland goes down

[Thomas Lübking]

luebking added a comment.


  If the child survives the parent that normally means it's forked at some 
point and the fork will clear the flag - anyway:
  Kai says the process knocks out gdb - is it a zombie process (indicated by 
"D", cannot be stopped or killed, let alone being terminated) and/or is the 
socket actually closed or still around?

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas

D5249: [RFC] New effect plugin - projector (keystone) correction

[Thomas Lübking]

luebking added a comment.


  ftr, not sure about --transform correctnes, but metamodes 
ViewportIn/ViewportOut work flawless on at least the nvidia blob.

REPOSITORY
  R108 KWin

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

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

D5245: Desaturate non-responsive windows

[Thomas Lübking]

luebking added a comment.


  In https://phabricator.kde.org/D5245#98917, @graesslin wrote:
  
  > This is something we do not know. We do not know what color scheme the 
window uses. Given that a desaturation is probably the best we can do.
  
  
  Lower brightness?

REPOSITORY
  R108 KWin

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

To: broulik, #plasma, #kwin, #vdg, graesslin
Cc: luebking, kvermette, graesslin, plasma-devel, kwin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D5232: Add override to methods that override methods on their parent class

[Thomas Lübking]

luebking added a comment.


  In case you'd rather not want to introduce a git history wall:
  -Wno-inconsistent-missing-override 
-Wno-inconsistent-missing-destructor-override -Wno-initializer-overrides

REPOSITORY
  R108 KWin

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

To: apol, #plasma, graesslin, davidedmundson
Cc: luebking, plasma-devel, kwin, #kwin, progwolff, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol

[Differential] [Changed Subscribers] D4718: support for auto-hidden windows to resize

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> client.cpp:2086
> +m_edgeGeometryTrackingConnection = connect(this, 
> ::geometryChanged, this, [this, border](){
> +hideClient(true);
> +ScreenEdges::self()->reserve(this, border);

and if the geometry changes while it's (intentionally) not hidden?

> geometry.cpp:2116
>  updateTabGroupStates(TabGroup::Geometry);
> +qWarning()<<""<  emit geometryChanged();

*ough*

> screenedge.cpp:989
>  hadBorder = true;
> -if ((*it)->border() == border) {
> -if (!(*it)->isReserved()) {

sure the sanity check is now really superfluous?

REPOSITORY
  R108 KWin

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

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

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

[Differential] [Commented On] D4220: Add a basic SNI for keyboard layout

[Thomas Lübking]

luebking added a comment.


  In https://phabricator.kde.org/D4220#78878, @graesslin wrote:
  
  >
  
  
  
  
  > This change is about the opposite: supporting *changing* the layout. Not 
about notifying the layout change, that is something we already support for at 
least a year.
  
  Yes.
  Afaiu presently kwinD (;-P) is the only authority to alter the layout and the 
only one to provide a GUI for that as well?
  Clients can't do that - be it a virtual keyboard or a multilingual editor, be 
it im- or explicitly (text language recognition or a drop down menu)?
  
  What I meant to concern (and dared to be slightly OT in it) is that too much 
(interactive) features (and code) are dragged into a highly crucial process 
which becomes an increasingly monolithic-by-design GUI shell.

REPOSITORY
  R108 KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: davidedmundson, luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas

[Differential] [Commented On] D4220: Add a basic SNI for keyboard layout

[Thomas Lübking]

luebking added a comment.


  "On Wayland that kded has no real access to the layouts and cannot
  properly implement switching. Given that it's better to integrate the
  SNI directly in KWin."
  
  I'll raise a fundamental question: is wayland prone to end up being
  PID 0: systemd
  PID 1: kwin
  ?
  
  Since kwin is the new display server, it may need an interface in such 
occasions, signalling the change, exposing the layout and allow clients to 
alter the layout.
  Suppose someone would want to have a virtual keyboard with a layout 
indicator/switcher, that should not be dragged into kwin? Latter is now an 
absolutely crucial process (notably as long as handing over wayland clients 
isn't supported) and dragging in functionality drags in complexity drags in 
crashes …
  
  /2¢

REPOSITORY
  R108 KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, eliasp, sebas

[Differential] [Accepted] D4113: Correct inital loading of BorderActivate

[Thomas Lübking]

luebking accepted this revision.
luebking added a reviewer: luebking.

REPOSITORY
  R108 KWin

BRANCH
  master

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

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

To: davidedmundson, #plasma, mart, luebking
Cc: #kwin, luebking, graesslin, mart, plasma-devel, kwin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Commented On] D4113: Correct inital loading of BorderActivate

[Thomas Lübking]

luebking added a comment.


  The entire parsing is totally not safe against JoeReddiot murking around in 
the config file, I wonder what happens if we pass "-1" and what is " " cast 
as...
  
  One should probably use "parseInt(border, 10);"?

REPOSITORY
  R108 KWin

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

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

To: davidedmundson, #plasma, mart
Cc: luebking, graesslin, mart, plasma-devel, kwin, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D3617: [Touchpad KCM] New KWin Wayland version

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> subdiff wrote in kwinwaylandbackend.cpp:84
> Are you sure? The foreach keyword is still listed in the official Qt docu. 
> What's the best alternative? A normal for-loop? With upcounting integer or 
> iterator?

"might deprecate it" - magic eightball wisdom ;-)

The reason is that there's now
http://en.cppreference.com/w/cpp/language/range-for

REPOSITORY
  R119 Plasma Desktop

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

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

To: subdiff, #kwin, #plasma_on_wayland, #plasma, #vdg
Cc: luebking, graesslin, knambiar, kwin, plasma-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D3472: API for window tabbing

[Thomas Lübking]

luebking added a comment.


  On a general note: this terribly looks like doubling all (or a lot of) the 
core logics reg. tabs - which we had in initial KDE 4 tabbing and which made 
tabbing *incredibly* buggy.
  I'd suggest to forward the cores tab logics and not keep local states, 
counters etc. around, you'll easily get out of sync.

REPOSITORY
  rKDECORATION Window Decoration Library

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

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

To: chinmoyr, graesslin, #kwin, #plasma
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> graesslin wrote in composite.cpp:748
> this misses the
> 
>   if (m_scene->compositingType() & OpenGLCompositing)

also indention.

> x11_platform.cpp:206
> +// Deliberately continue with PreFrame
> +case OpenGLSafePoint::PreFrame:
> +if (m_openGLFreezeProtectionThread == nullptr) {

PreFrame is (now) effectively "PreFirstGuardedFrame", is it?
And if invoked at some later point would create the timer and hit the config 
rewrite every single frame (for the counter is stuck at 0)?

> rename to avoid bad invocation?
=

REPOSITORY
  rKWIN KWin

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

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

To: antlarr, #kwin, #plasma, davidedmundson
Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> antlarr wrote in x11_platform.cpp:215
> yeah, but then it wouldn't be a synchronous call and it would lose all its 
> meaning, isn't it? :)

No, why? The purpose is to push the freeze timer forward as long as the GUI 
thread isn't blocked.
If the GUI thread is blocked, the forward pushing won't happen, the timer times 
out and set GL unsafe etc.

Do I miss something?

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

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

To: antlarr, #kwin, #plasma, davidedmundson
Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> antlarr wrote in x11_platform.cpp:215
> The timer is in another thread, so it can't be stopped/restarted from this 
> thread.

You should be able to call it as slot (whether QMetaObject::invokeMethod() 
works, I've never tried)

> antlarr wrote in x11_platform.cpp:224
> The config is not written in every frame. It's only saved when the timer is 
> triggered. That is, when a freeze is detected. That was the point of the 
> "remove overhead" commit.

Sorry, misread. (Actually didn't really read and just wanted to point out the 
other case ;-)

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

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

To: antlarr, #kwin, #plasma, davidedmundson
Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D3132: [platformx/x11] Add a freeze protection against OpenGL

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> composite.h:242
>  bool m_composeAtSwapCompletion;
> -bool m_firstFrame = true;
> +int m_frameCount = 0;
> +int m_maxFramesTestedForSafety = 30;

m_testedFrames or similar, "Frame Count" is too generic.
Alternatively, just use some "int m_freezeDectionFrames = 30;"  and count them 
down (so when < 1, you're done)

> platform.h:148
> +PostFrame,
> +PostLastFrame
>  };

Have a PreFirstFrame still and make PostLastFrame PostLastGuardedFrame or 
similar. ("Last frame" is right before kwin shuts down ;-)

> x11_platform.cpp:215
> +Q_ASSERT(m_openGLFreezeProtection == nullptr);
>  m_openGLFreezeProtection = new QTimer;
>  m_openGLFreezeProtection->setInterval(15000);

Why do you recreate it with every frame?
Only create it PreFirstFrame and delete it PostLastFrame (and inbeteween simply 
restart the pointer PreFrame)

> x11_platform.cpp:224
> +auto group = KConfigGroup(kwinApp()->config(), 
> "Compositing");
> +group.writeEntry(unsafeKey, true);
> +group.sync();

same goes for config writing

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

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

To: antlarr, #kwin, #plasma, davidedmundson
Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Commented On] D2931: Destroy DebugConsole on hide of QWindow

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> graesslin wrote in debug_console.cpp:547
> It might be a bug in KWin's QPA, so I wouldn't blame Qt here (yet).

Wouldn't that prevent the QWindow signal just as well?

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland, sebas
Cc: luebking, sebas, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts

[Differential] [Commented On] D3037: Support docks which take input

[Thomas Lübking]

luebking added a comment.


  In https://phabricator.kde.org/D3037#56298, @graesslin wrote:
  
  >
  
  
  
  
  > I don't see any real world cases where that would be needed.
  
  Embedded runners/search widgets (pretty famous item on KDE3 times, no idea 
whether it's relevant)
  
  "You have been warned" :-P

REPOSITORY
  rKWIN KWin

BRANCH
  dock-wants-input

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

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

To: graesslin, #kwin, #plasma_on_wayland, sebas
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D3037: Support docks which take input

[Thomas Lübking]

luebking added a comment.


  E... this does not allow the dock to control input, ie. the dock can or 
can not take focus but you want focus if eg. clicking into a lineedit while do 
certainly not want it when clicking a button that will activate a window (FSP 
trouble)
  
  With the stateful approach, the dock would have to juggle around the setting 
instead of just telling the WM to pass it the focus *now* (for some local user 
interaction)

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D2945: Workaround xkbcommon behavior concerning consumed modifiers

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> keyboard_input.cpp:420
> +// handle this is through new API in xkbcommon which doesn't exist yet
> +if (m_consumedModifiers & ~m_modifiers) {
> +return mods;

Possible pitfalls:

What if eg. Ctrl+Alt+Foo creates a keysym, but Ctrl+Alt+Shift+Foo does not?

If however Ctrl+Alt+Foo and Ctrl+Alt+Shift+Foo create keysyms, Ctrl+Alt+Shift 
would be consumed but Ctrl+Alt should be considered consumed as well (so this 
test fails), right?

-> should one test for equality to rule out at least case one?

REPOSITORY
  rKWIN KWin

BRANCH
  consumed-mods-fix-5.8

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

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

To: graesslin, #kwin, #plasma_on_wayland, sebas
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D2953: [tabbox] Intercept QWheelEvents on QQuickWindow for scrolling

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> graesslin wrote in tabboxhandler.cpp:621
> dangerous. If there is a scrolling in both horizontal and vertical (could 
> happen on touchpad) it could result in no scrolling if one is negative and 
> the other is positive.

One might call that unsoecific user behavior :-P
But on that touchpad the present implementation will ignore large vertical 
scrolls for the slightest horizontal movement.

-> pick the only with the bigger absolute?

REPOSITORY
  rKWIN KWin

BRANCH
  tabbox-wheelevent-5.8

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

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

To: graesslin, #kwin, #plasma, broulik, lukedashjr
Cc: luebking, lukedashjr, plasma-devel, kwin, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D2953: [tabbox] Intercept QWheelEvents on QQuickWindow for scrolling

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> tabboxhandler.cpp:618
> +if (watched == d->window()) {
> +if (e->type() == QEvent::Wheel) {
> +QWheelEvent *event = static_cast(e);

test type first - you'll get many paint events etc. but the filter is installed 
to only one object, ie. first check will currently always hit and second very 
few times only

> tabboxhandler.cpp:621
> +// On x11 the delta for vertical scrolling might also be on X 
> for whatever reason
> +const int delta = event->angleDelta().y() != 0 ? 
> event->angleDelta().y() : event->angleDelta().x();
> +d->wheelAngleDelta += delta;

x + y?

> tabboxhandler.cpp:626
> +const QModelIndex index = nextPrev(true);
> +if (index.isValid()) {
> +setCurrentIndex(index);

draw the indev var out, find the last one and if it's in the end valid, set it 
only once?

REPOSITORY
  rKWIN KWin

BRANCH
  tabbox-wheelevent-5.8

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

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

To: graesslin, #kwin, #plasma, broulik, lukedashjr
Cc: luebking, lukedashjr, plasma-devel, kwin, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas

[Differential] [Commented On] D2931: Destroy DebugConsole on hide of QWindow

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> graesslin wrote in debug_console.cpp:547
> closeEvent is also not invoked.

iow QWidget is bitrot. Maybe better use QML ... :-(

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, sebas, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts

[Differential] [Commented On] D2931: Destroy DebugConsole on hide of QWindow

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> debug_console.cpp:546
> +[this] (bool visible) {
> +if (visible) {
> +// ignore

if (!visible) deleteLater(); ... ;-P

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, sebas, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts

[Differential] [Commented On] D2931: Destroy DebugConsole on hide of QWindow

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> graesslin wrote in debug_console.cpp:547
> I tried implementing the hideEvent, but for whatever reason it didn't get 
> triggered.

What about closeEvent?

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, sebas, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts

[Differential] [Changed Subscribers] D2931: Destroy DebugConsole on hide of QWindow

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> sebas wrote in debug_console.cpp:547
> return?

Why not reimplment the hideEvent?

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, sebas, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts

[Differential] [Changed Subscribers] D2787: Add support for resize only borders on Wayland

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> abstract_client.cpp:1634
> +const auto margins = decoration()->resizeOnlyBorders();
> +if (!margins.isNull()) {
> +return Toplevel::inputGeometry() + margins;

spare this test and just

  return TopLevel::inputGeometry() + decoration()->resizeOnlyBorders();

resp. even (more controlled, geometry() isn't virtual, but it depends on what 
you really need here)

  return geometry() + decoration()->resizeOnlyBorders();

?

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D2786: [server] Don't send key release for not pressed keys and no double key press

[Thomas Lübking]

luebking added a comment.


  Sounds reasonable enough, but seems to point out an issue in wayland:
  
  If clients "somehow" rely on balanced input, are there artificial releases 
when a client looses focus?
  If not, do you end up with several clients in autorepeat (client side? 
really? with different behavior in every toolkit?) mode?
  If yes, are there likewise artificial presses when gaining focus?
  If yes, what happens for a shortcut where client A takes focus to client B on 
keypress X? X entered in B?

REPOSITORY
  rKWAYLAND KWayland

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

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

To: graesslin, #plasma_on_wayland, #kwin
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D2584: Introduce a config option whether applications are allowed to block compositing

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> sebas wrote in compositing.ui:9
> So your change made the window's minimum width more than 300px wider? That 
> sounds wrong...
> 
> What I mean is to resize the form  in designer to the smallest possible size 
> before you save it there, perhaps that got lost in translation.

The stored size should be rather irrelevant, for it is tied to the style used 
by the developer at creation of the widget anyway.

Ie. if there's desire to use the minimum size at some point, one should 
::adjustSize() at runtime.
Otherwise the size is (hopefully still attempted) to be restored from the last 
user provided size anyway.

REPOSITORY
  rKWIN KWin

BRANCH
  blocking-compositing

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

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

To: graesslin, #kwin, #vdg, #plasma_on_wayland, sebas
Cc: sebas, broulik, colomar, luebking, mart, bshah, plasma-devel, kwin, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts

[Differential] [Commented On] D2630: Support highlighting windows through EffectsHandlerImpl

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> kwineffects.h:496
> + **/
> +virtual bool performFeature(Feature feature, const QVariantList 
> );
> +

maybe only "perform" (and have the signature fill in the information), but in 
general a much better solution ;-)

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D2630: Support highlighting windows through EffectsHandlerImpl

[Thomas Lübking]

luebking added a comment.


  You could expose the relevant method index, but looking up the signature is 
more elegant (and this isn't a hot call)

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Accepted] D2627: Set the restore geometry after placing a ShellClient for the first time

[Thomas Lübking]

luebking accepted this revision.
luebking added a reviewer: luebking.
This revision is now accepted and ready to land.

REPOSITORY
  rKWIN KWin

BRANCH
  geometry-restore-after-placement

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

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

To: graesslin, #kwin, #plasma_on_wayland, bshah, luebking
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D2630: Support highlighting windows through EffectsHandlerImpl

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> effects.cpp:1560
> +const auto typeId = qMetaTypeId>();
> +for (int i = 0; i < e->metaObject()->methodCount(); ++i) {
> +auto method = e->metaObject()->method(i);

Errr... wut?
Any reason to not *demand* some sort of soft API (slot signature) instead of 
digging for the next best thing that remotely looks like the right thing?

Maybe provide hard API like "Effect::performFeature(Feature f, QVariant v)"?

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D2627: Set the restore geometry after placing a ShellClient for the first time

[Thomas Lübking]

luebking added a comment.


  Fixwise good, APIwise, maybe add AbstractClient::placeIn(QRect ) { 
Placement::self()->place(this, area); setGeometryRestore(geometry()); } to
  a) prevent uninformed messing around with the restore geomery
  b) guarantee sync behavior (when placing occurs)
  
  ?

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland, bshah
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D2584: Introduce a config option whether applications are allowed to block compositing

[Thomas Lübking]

luebking added a comment.


  They'll unredirect the window. The property is however not mandarory:
  
  "The compositing manager MAY bypass compositing for both fullscreen and 
non-fullscreen windows if bypassing is requested" (the supplemental "but MUST 
NOT bypass if it would cause differences from the composited appearance" btw. 
effectively means that the hint must be ignored in all cases or adjusted with 
every painted frame. It's a really shitty protocol)

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland, #vdg
Cc: broulik, colomar, luebking, mart, bshah, plasma-devel, kwin, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D2584: Introduce a config option whether applications are allowed to block compositing

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> compositing.h:63
>  int openGLPlatformInterface() const;
> +bool isWindowsBlockingCompositing() const;
>  

The "is" prefix is a QML mandate, right?

isObeyingCompositorBlockRequests
isCompositorBlockAllowed

However, afaiu the biggest complaint was that it was broken (due to shadow 
deletion) yesno? Really worth a setting (rather than a blind rule to achieve 
the same)?

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland, #vdg
Cc: luebking, mart, bshah, plasma-devel, kwin, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas

[Differential] [Commented On] D2531: Warp the xcb pointer whenever pointer leaves an X11 surface

[Thomas Lübking]

luebking added a comment.


  Errr you should get a LeaveNotify on grabs and an EnterNotify on releases 
anyway (but I do not really understand the tackled problem)

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland, bshah
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D2531: Warp the xcb pointer whenever pointer leaves an X11 surface

[Thomas Lübking]

luebking added a comment.


  Could one map/unmap an InputOnly window instead to trigger a (differen) leave 
notify event? (or would unmapping the window cause a - disturbing? - enter 
notify event?)

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland, bshah
Cc: luebking, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, 
jensreuterberg, abetts, sebas

Re: Review Request 128647: [DesktopView] Stop raising desktop over dialog windows

[Thomas Lübking]

> On Aug. 12, 2016, 11:31 vorm., Martin Gräßlin wrote:
> > Could someone please test, whether this works?
> > 
> > diff --git a/shell/desktopview.cpp b/shell/desktopview.cpp
> > index 14a25af..38c6621 100644
> > --- a/shell/desktopview.cpp
> > +++ b/shell/desktopview.cpp
> > @@ -226,6 +226,7 @@ bool DesktopView::event(QEvent *e)
> >  switch (pe->surfaceEventType()) {
> >  case QPlatformSurfaceEvent::SurfaceCreated:
> >  setupWaylandIntegration();
> > +ensureWindowType();
> >  break;
> >  case QPlatformSurfaceEvent::SurfaceAboutToBeDestroyed:
> >  delete m_shellSurface;
> 
> Anthony Fieroni wrote:
> I can't get plasmashell to work in wayland on my skylake laptop, it 
> crashes every time.
> 
> Martin Gräßlin wrote:
> I meant to test that on X11, not on Wayland. On Wayland I can test myself 
> :-)
> 
> Anthony Fieroni wrote:
> How this expect to work? Multiple calls in setFlags triggers the bug, if 
> we remove esureWindowType at focusIn then it *should* work.

The implication is probably to remove it *everywhere* else.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128647/#review98334
---


On Aug. 10, 2016, 6:28 nachm., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128647/
> ---
> 
> (Updated Aug. 10, 2016, 6:28 nachm.)
> 
> 
> Review request for kwin, Plasma, David Edmundson, David Rosca, and Marco 
> Martin.
> 
> 
> Bugs: 350826 and 365014
> https://bugs.kde.org/show_bug.cgi?id=350826
> https://bugs.kde.org/show_bug.cgi?id=365014
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> ^^
> 
> 
> Diffs
> -
> 
>   shell/desktopview.cpp 83866dc 
> 
> Diff: https://git.reviewboard.kde.org/r/128647/diff/
> 
> 
> Testing
> ---
> 
> 1. open 1 or more non maximized windows
> 2. make left clicks periodically between window and the desktop
> 3. desktop is raised over opened windows *sometimes*
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

Re: Review Request 128647: [DesktopView] Stop raising desktop over dialog windows

[Thomas Lübking]

> On Aug. 12, 2016, 11:05 vorm., Anthony Fieroni wrote:
> > I can't see bug in Qt implementaion of setFlags 
> > https://code.woboq.org/qt5/qtbase/src/plugins/platforms/xcb/qxcbwindow.cpp.html#1130
> > xcb plugin bug?

Afaik the entire thing operates on an undocumented string property to carry the 
type, ie. at least there's no legal way to initially indicate the window type 
from client code.

But I don't ultimately know why the ::ensureWindowType function exists. I would 
have ranted around when it had been presented ;-)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128647/#review98330
---


On Aug. 10, 2016, 6:28 nachm., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128647/
> ---
> 
> (Updated Aug. 10, 2016, 6:28 nachm.)
> 
> 
> Review request for kwin, Plasma, David Edmundson, David Rosca, and Marco 
> Martin.
> 
> 
> Bugs: 350826 and 365014
> https://bugs.kde.org/show_bug.cgi?id=350826
> https://bugs.kde.org/show_bug.cgi?id=365014
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> ^^
> 
> 
> Diffs
> -
> 
>   shell/desktopview.cpp 83866dc 
> 
> Diff: https://git.reviewboard.kde.org/r/128647/diff/
> 
> 
> Testing
> ---
> 
> 1. open 1 or more non maximized windows
> 2. make left clicks periodically between window and the desktop
> 3. desktop is raised over opened windows *sometimes*
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

Re: Review Request 128647: [DesktopView] Stop raising desktop over dialog windows

[Thomas Lübking]

> On Aug. 10, 2016, 1:23 nachm., David Rosca wrote:
> > https://phabricator.kde.org/D2121
> 
> Anthony Fieroni wrote:
> So why not ship it?
> 
> Sebastian Kügler wrote:
> Did you read Martin's comments on the phab request?
> 
> Anthony Fieroni wrote:
> I see it make sense, because it change props on focusIn before click who 
> triggers the bug. This is correct patch to me.
> 
> Thomas Lübking wrote:
> The bug is the sheer existence of this function which is a ridiculous 
> workaround for Qt *silently* ignoring the window type flag.
> 
> Removing it there will just lower the odds that plasmashell succeeds in 
> setting the correct window type ever (along lowering the odds to do the 
> rubbish that causes the normal type)
> 
> The ::ensureWindowType function needs to be erased and the type set 
> *once* explicitly after the native window is created for the window, but 
> before it's mapped (ie. what Qt should actually do)
> 
> Right now, this is just messing around with the symptoms.
> 
> Anthony Fieroni wrote:
> Workaround or not this patch is a *must*.

No, the bug *should* be fixed, because of course the situation is ridiculuous.
But this patch isn't even a workaround but only shifts odds around and does 
nothing to actually fix the problem.

You can remove the call here as initial step, but if you stop there, you'll 
just face other variations of the bug afterwards - maybe even initially cause 
them for other users. That's what Martin objected on David's patch.

I'd however suggest to keep the discussion there. RB is afaiu not even an 
accepted system anymore but in shutdown mode.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128647/#review98286
---


On Aug. 10, 2016, 6:28 nachm., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128647/
> ---
> 
> (Updated Aug. 10, 2016, 6:28 nachm.)
> 
> 
> Review request for kwin, Plasma, David Edmundson, David Rosca, and Marco 
> Martin.
> 
> 
> Bugs: 350826 and 365014
> https://bugs.kde.org/show_bug.cgi?id=350826
> https://bugs.kde.org/show_bug.cgi?id=365014
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> ^^
> 
> 
> Diffs
> -
> 
>   shell/desktopview.cpp 83866dc 
> 
> Diff: https://git.reviewboard.kde.org/r/128647/diff/
> 
> 
> Testing
> ---
> 
> 1. open 1 or more non maximized windows
> 2. make left clicks periodically between window and the desktop
> 3. desktop is raised over opened windows *sometimes*
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

Re: Review Request 128647: [DesktopView] Stop raising desktop over dialog windows

[Thomas Lübking]

> On Aug. 10, 2016, 1:23 nachm., David Rosca wrote:
> > https://phabricator.kde.org/D2121
> 
> Anthony Fieroni wrote:
> So why not ship it?
> 
> Sebastian Kügler wrote:
> Did you read Martin's comments on the phab request?
> 
> Anthony Fieroni wrote:
> I see it make sense, because it change props on focusIn before click who 
> triggers the bug. This is correct patch to me.

The bug is the sheer existence of this function which is a ridiculous 
workaround for Qt *silently* ignoring the window type flag.

Removing it there will just lower the odds that plasmashell succeeds in setting 
the correct window type ever (along lowering the odds to do the rubbish that 
causes the normal type)

The ::ensureWindowType function needs to be erased and the type set *once* 
explicitly after the native window is created for the window, but before it's 
mapped (ie. what Qt should actually do)

Right now, this is just messing around with the symptoms.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128647/#review98286
---


On Aug. 10, 2016, 1:22 nachm., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128647/
> ---
> 
> (Updated Aug. 10, 2016, 1:22 nachm.)
> 
> 
> Review request for kwin, Plasma, David Edmundson, David Rosca, and Marco 
> Martin.
> 
> 
> Bugs: 350826 and 365014
> https://bugs.kde.org/show_bug.cgi?id=350826
> https://bugs.kde.org/show_bug.cgi?id=365014
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> ^^
> 
> 
> Diffs
> -
> 
>   shell/desktopview.cpp 83866dc 
> 
> Diff: https://git.reviewboard.kde.org/r/128647/diff/
> 
> 
> Testing
> ---
> 
> 1. open 1 or more non maximized windows
> 2. make left clicks periodically between window and the desktop
> 3. desktop is raised over opened windows *sometimes*
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

[Differential] [Commented On] D2164: Allow struts on panels between screen edges if WM is KWin

[Thomas Lübking]

luebking added a comment.


  In https://phabricator.kde.org/D2164#40170, @graesslin wrote:
  
  > - fixed logic error with platform check
  
  
  See? ;-)
  
  Looks good otherwise.

INLINE COMMENTS

> panelview.cpp:926
>  
> -//Extended struts against a screen edge near to another screen are 
> really harmful, so windows maximized under the panel is a lesser pain
> -//TODO: force "windows can cover" in those cases?
> -const int numScreens = corona()->numScreens();
> -for (int i = 0; i < numScreens; ++i) {
> -if (i == containment()->screen()) {
> -continue;
> -}
> -
> -const QRect otherScreen = corona()->screenGeometry(i);
> -if (!otherScreen.isValid()) {
> -continue;
> -}
> -
> -switch (location())
> -{
> -case Plasma::Types::TopEdge:
> -if (otherScreen.bottom() <= thisScreen.top()) {
> -KWindowSystem::setExtendedStrut(winId(), 0, 0, 0, 0, 0, 
> 0, 0, 0, 0, 0, 0, 0);
> -return;
> -}
> -break;
> -case Plasma::Types::BottomEdge:
> -if (otherScreen.top() >= thisScreen.bottom()) {
> -KWindowSystem::setExtendedStrut(winId(), 0, 0, 0, 0, 0, 
> 0, 0, 0, 0, 0, 0, 0);
> -return;
> -}
> -break;
> -case Plasma::Types::RightEdge:
> -if (otherScreen.left() >= thisScreen.right()) {
> -KWindowSystem::setExtendedStrut(winId(), 0, 0, 0, 0, 0, 
> 0, 0, 0, 0, 0, 0, 0);
> -return;
> -}
> -break;
> -case Plasma::Types::LeftEdge:
> -if (otherScreen.right() <= thisScreen.left()) {
> -KWindowSystem::setExtendedStrut(winId(), 0, 0, 0, 0, 0, 
> 0, 0, 0, 0, 0, 0, 0);
> -return;
> -}
> -break;
> -default:
> -return;
> -}
> +if (!canSetStrut()) {
> +KWindowSystem::setExtendedStrut(winId(), 0, 0, 0, 0, 0, 0, 0, 0, 
> 0, 0, 0, 0);

this could go up (spare crashy virtualSize() calls ;-)

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

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

To: graesslin, #plasma
Cc: mart, luebking, plasma-devel, jensreuterberg, abetts, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

[Differential] [Commented On] D2164: Allow struts on panels between screen edges if WM is KWin

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> graesslin wrote in panelview.cpp:857
> > also, how does this react when the WM is replaced?
> 
> tricky. I think it's a corner case which could be ignored. We don't really 
> have a way to detect it. I would say only "experienced" users know how to do 
> that, they should be prepared for impact.

One would require another signal in KWindowSystem - ultimately, it's only 
monitoring _NET_SUPPORTING_WM_CHECK on the root window

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

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

To: graesslin, #plasma
Cc: luebking, plasma-devel, jensreuterberg, abetts, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

[Differential] [Changed Subscribers] D2164: Allow struts on panels between screen edges if WM is KWin

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> panelview.cpp:848
>  
> +bool PanelView::shouldNotSetStrut() const
> +{

API sanity:

"if (!shouldNotSetStrut())" ...

double negations make people dizzy ;-)

> panelview.cpp:857
> +NETRootInfo rootInfo(QX11Info::connection(), NET::Supported | 
> NET::SupportingWMCheck);
> +if (qstrcmp(rootInfo.wmName(), "KWin") == 0) {
> +// KWin since 5.7 can handle this fine, so only exclude for other 
> window managers

qstricmp?
also, how does this react when the WM is replaced?

> panelview.cpp:863
> +const QRect thisScreen = screen()->geometry();
> +const int numScreens = corona()->numScreens();
> +for (int i = 0; i < numScreens; ++i) {

if (numScreens < 2) return false; (ie. true for the switched API - already 
confused?)

> panelview.cpp:878
> +if (otherScreen.bottom() <= thisScreen.top()) {
> +KWindowSystem::setExtendedStrut(winId(), 0, 0, 0, 0, 0, 0, 
> 0, 0, 0, 0, 0, 0);
> +return true;

the query has a side effect - the strut update should be in the calling branch, 
yesno?

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

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

To: graesslin, #plasma
Cc: luebking, plasma-devel, jensreuterberg, abetts, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

[Differential] [Changed Subscribers] D1807: Request the resize of ShellSurface after Decoration updated the borders when maximizing

[Thomas Lübking]

luebking added inline comments.

INLINE COMMENTS

> shell_client.cpp:592
> +if (m_maximizeMode == MaximizeFull) {
> +m_geomMaximizeRestore = geometry();
> +requestGeometry(workspace()->clientArea(MaximizeArea, this));

ifff geometry() includes the deco for the ShellClient, this will get it 
shrinking away on each max/restore cycle.
(If it's only the client geo, there's no problem)

REPOSITORY
  rKWIN KWin

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

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

To: graesslin, #kwin, #plasma_on_wayland
Cc: luebking, plasma-devel, kwin, hardening, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.
> 
> Thomas Lübking wrote:
> The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
> 
> That's however not a bug (at least it's intended) and I've no idea why 
> that's relevant to the weird geometry handling of SNI items (which looks 
> *fr* too complex - the position of remapped windows is usually maintained 
> by QWidget ...)
> 
> Anthony Fieroni wrote:
> I saw the code, it looks like KWindowSystem not depend on Kwin, but Kwin 
> *must* be started *before* use of kwindowsystem. Thomas may right, because 
> setGeometry even not work on sessions restored app.
> When session was finish if start apps with kmail --session session-id 
> everything wors fine.
> 
> Anthony Fieroni wrote:
> Now, after Thomas comment, i even think only widget->show() must works 
> because widget has internal frameGeometry and position.
> 
> Martin Gräßlin wrote:
> > but Kwin must be started before use of kwindowsystem
> 
> no, really there is no reason for that. KWindowSystem doesn't depend on a 
> window manager running.
> 
> Anthony Fieroni wrote:
> Then, correct KWin to kwindowsystem to start working. If there is no bug 
> this will works on all cases -> https://git.reviewboard.kde.org/r/127216/ but 
> it's not.
> 
> Martin Gräßlin wrote:
> I don't know why you have problems, but it's impossible that 
> KWindowSystem::self() returns a nullptr. You can check the code to verify. 
> Something is really broken on your side, but I don't know what. Maybe missing 
> plugins installed?
> 
> Thomas Lübking wrote:
> it's not a hard dependecy on the instace, the sni geometry handling is 
> just plain broken.
> the workaround operates on configure events on the mapped window which 
> will go nowhere if there's no running wm.
> 
> the client needs to handle the no-wm case, ie. configure the window 
> correctly, but to repeat myself: such requirement implies that either sni or 
> qwidget/qwindow is completely fucked up.
> 
> qwindow/qwidget::setGeometry should justwork(tm)
> 
> Anthony Fieroni wrote:
> setGeometry *not* works, i can confirm since i'm test it. Nothing works 
> if Kwin is started *after* usage of kwindowsystem::self().
> Martin yeah nullprt is impossible.
> 
> Martin Gräßlin wrote:
> then this is a bug in Qt's xcb plugin and needs to be fixed there. 
> Setting a geometry without a window manager must be possible.
> 
> Anthony Fieroni wrote:
> Wait a little, the bug is not in xcb. I'm not clear or what. This 
> *happend* only on session restored apps, when kwindowsystem::self() is before 
> kwin statup, only in this case. If run a app after kwin is started *all* 
> works fine setGeometry(), move() - no problems.
> 
> Anthony Fieroni wrote:
> Thomas Lübking wrote:
>  The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
>  
> For me, this is best explanation. Adn give you the easiest, no pain, 
> working solution - wait kwin to start completely.
> 
> Martin Gräßlin wrote:
> > Adn give you the easiest, no pain, working solution - wait kwin to 
> start completely.
> 
> I disagree. Working around such problems in the session startup seems 
> wrong. I would say excluse the sni windows from session restore, which can be 
> done from Qt API.
> 
> Thomas Lübking wrote:
> Can we please get few things straight?
> 
> You say that ::setGeometry doesn't work, but 
> https://git.reviewboard.kde.org/r/127216/diff/5#index_header makes use of it 
> (uses the propsed internally saved position and restores it with 
> QWidget::move what's basically a ::setGeometry special case)
> 
> => Does this work (leaving aside session restorage), yes or no?
> 
> Assuming it *does* work (except for session restorage) the claims to 
> require a WM for operative KWindowSystem invocation are gladfully, since 
> you're not using KWindowSystem at all.
> 
> 
> 
> Now onto session restorage:
> ---
> What exactly is the problem here?
> a) the windows are *not* visible when the session restarts. Showing them 
> show them in wrong p

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.
> 
> Thomas Lübking wrote:
> The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
> 
> That's however not a bug (at least it's intended) and I've no idea why 
> that's relevant to the weird geometry handling of SNI items (which looks 
> *fr* too complex - the position of remapped windows is usually maintained 
> by QWidget ...)
> 
> Anthony Fieroni wrote:
> I saw the code, it looks like KWindowSystem not depend on Kwin, but Kwin 
> *must* be started *before* use of kwindowsystem. Thomas may right, because 
> setGeometry even not work on sessions restored app.
> When session was finish if start apps with kmail --session session-id 
> everything wors fine.
> 
> Anthony Fieroni wrote:
> Now, after Thomas comment, i even think only widget->show() must works 
> because widget has internal frameGeometry and position.
> 
> Martin Gräßlin wrote:
> > but Kwin must be started before use of kwindowsystem
> 
> no, really there is no reason for that. KWindowSystem doesn't depend on a 
> window manager running.
> 
> Anthony Fieroni wrote:
> Then, correct KWin to kwindowsystem to start working. If there is no bug 
> this will works on all cases -> https://git.reviewboard.kde.org/r/127216/ but 
> it's not.
> 
> Martin Gräßlin wrote:
> I don't know why you have problems, but it's impossible that 
> KWindowSystem::self() returns a nullptr. You can check the code to verify. 
> Something is really broken on your side, but I don't know what. Maybe missing 
> plugins installed?
> 
> Thomas Lübking wrote:
> it's not a hard dependecy on the instace, the sni geometry handling is 
> just plain broken.
> the workaround operates on configure events on the mapped window which 
> will go nowhere if there's no running wm.
> 
> the client needs to handle the no-wm case, ie. configure the window 
> correctly, but to repeat myself: such requirement implies that either sni or 
> qwidget/qwindow is completely fucked up.
> 
> qwindow/qwidget::setGeometry should justwork(tm)
> 
> Anthony Fieroni wrote:
> setGeometry *not* works, i can confirm since i'm test it. Nothing works 
> if Kwin is started *after* usage of kwindowsystem::self().
> Martin yeah nullprt is impossible.
> 
> Martin Gräßlin wrote:
> then this is a bug in Qt's xcb plugin and needs to be fixed there. 
> Setting a geometry without a window manager must be possible.
> 
> Anthony Fieroni wrote:
> Wait a little, the bug is not in xcb. I'm not clear or what. This 
> *happend* only on session restored apps, when kwindowsystem::self() is before 
> kwin statup, only in this case. If run a app after kwin is started *all* 
> works fine setGeometry(), move() - no problems.
> 
> Anthony Fieroni wrote:
> Thomas Lübking wrote:
>  The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
>  
> For me, this is best explanation. Adn give you the easiest, no pain, 
> working solution - wait kwin to start completely.
> 
> Martin Gräßlin wrote:
> > Adn give you the easiest, no pain, working solution - wait kwin to 
> start completely.
> 
> I disagree. Working around such problems in the session startup seems 
> wrong. I would say excluse the sni windows from session restore, which can be 
> done from Qt API.
> 
> Thomas Lübking wrote:
> Can we please get few things straight?
> 
> You say that ::setGeometry doesn't work, but 
> https://git.reviewboard.kde.org/r/127216/diff/5#index_header makes use of it 
> (uses the propsed internally saved position and restores it with 
> QWidget::move what's basically a ::setGeometry special case)
> 
> => Does this work (leaving aside session restorage), yes or no?
> 
> Assuming it *does* work (except for session restorage) the claims to 
> require a WM for operative KWindowSystem invocation are gladfully, since 
> you're not using KWindowSystem at all.
> 
> 
> 
> Now onto session restorage:
> ---
> What exactly is the problem here?
> a) the windows are *not* visible when the session restarts. Showing them 
> show them in wrong p

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.
> 
> Thomas Lübking wrote:
> The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
> 
> That's however not a bug (at least it's intended) and I've no idea why 
> that's relevant to the weird geometry handling of SNI items (which looks 
> *fr* too complex - the position of remapped windows is usually maintained 
> by QWidget ...)
> 
> Anthony Fieroni wrote:
> I saw the code, it looks like KWindowSystem not depend on Kwin, but Kwin 
> *must* be started *before* use of kwindowsystem. Thomas may right, because 
> setGeometry even not work on sessions restored app.
> When session was finish if start apps with kmail --session session-id 
> everything wors fine.
> 
> Anthony Fieroni wrote:
> Now, after Thomas comment, i even think only widget->show() must works 
> because widget has internal frameGeometry and position.
> 
> Martin Gräßlin wrote:
> > but Kwin must be started before use of kwindowsystem
> 
> no, really there is no reason for that. KWindowSystem doesn't depend on a 
> window manager running.
> 
> Anthony Fieroni wrote:
> Then, correct KWin to kwindowsystem to start working. If there is no bug 
> this will works on all cases -> https://git.reviewboard.kde.org/r/127216/ but 
> it's not.
> 
> Martin Gräßlin wrote:
> I don't know why you have problems, but it's impossible that 
> KWindowSystem::self() returns a nullptr. You can check the code to verify. 
> Something is really broken on your side, but I don't know what. Maybe missing 
> plugins installed?
> 
> Thomas Lübking wrote:
> it's not a hard dependecy on the instace, the sni geometry handling is 
> just plain broken.
> the workaround operates on configure events on the mapped window which 
> will go nowhere if there's no running wm.
> 
> the client needs to handle the no-wm case, ie. configure the window 
> correctly, but to repeat myself: such requirement implies that either sni or 
> qwidget/qwindow is completely fucked up.
> 
> qwindow/qwidget::setGeometry should justwork(tm)
> 
> Anthony Fieroni wrote:
> setGeometry *not* works, i can confirm since i'm test it. Nothing works 
> if Kwin is started *after* usage of kwindowsystem::self().
> Martin yeah nullprt is impossible.
> 
> Martin Gräßlin wrote:
> then this is a bug in Qt's xcb plugin and needs to be fixed there. 
> Setting a geometry without a window manager must be possible.
> 
> Anthony Fieroni wrote:
> Wait a little, the bug is not in xcb. I'm not clear or what. This 
> *happend* only on session restored apps, when kwindowsystem::self() is before 
> kwin statup, only in this case. If run a app after kwin is started *all* 
> works fine setGeometry(), move() - no problems.
> 
> Anthony Fieroni wrote:
> Thomas Lübking wrote:
>  The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
>  
> For me, this is best explanation. Adn give you the easiest, no pain, 
> working solution - wait kwin to start completely.
> 
> Martin Gräßlin wrote:
> > Adn give you the easiest, no pain, working solution - wait kwin to 
> start completely.
> 
> I disagree. Working around such problems in the session startup seems 
> wrong. I would say excluse the sni windows from session restore, which can be 
> done from Qt API.
> 
> Thomas Lübking wrote:
> Can we please get few things straight?
> 
> You say that ::setGeometry doesn't work, but 
> https://git.reviewboard.kde.org/r/127216/diff/5#index_header makes use of it 
> (uses the propsed internally saved position and restores it with 
> QWidget::move what's basically a ::setGeometry special case)
> 
> => Does this work (leaving aside session restorage), yes or no?
> 
> Assuming it *does* work (except for session restorage) the claims to 
> require a WM for operative KWindowSystem invocation are gladfully, since 
> you're not using KWindowSystem at all.
> 
> 
> 
> Now onto session restorage:
> ---
> What exactly is the problem here?
> a) the windows are *not* visible when the session restarts. Showing them 
> show them in wrong p

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.
> 
> Thomas Lübking wrote:
> The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
> 
> That's however not a bug (at least it's intended) and I've no idea why 
> that's relevant to the weird geometry handling of SNI items (which looks 
> *fr* too complex - the position of remapped windows is usually maintained 
> by QWidget ...)
> 
> Anthony Fieroni wrote:
> I saw the code, it looks like KWindowSystem not depend on Kwin, but Kwin 
> *must* be started *before* use of kwindowsystem. Thomas may right, because 
> setGeometry even not work on sessions restored app.
> When session was finish if start apps with kmail --session session-id 
> everything wors fine.
> 
> Anthony Fieroni wrote:
> Now, after Thomas comment, i even think only widget->show() must works 
> because widget has internal frameGeometry and position.
> 
> Martin Gräßlin wrote:
> > but Kwin must be started before use of kwindowsystem
> 
> no, really there is no reason for that. KWindowSystem doesn't depend on a 
> window manager running.
> 
> Anthony Fieroni wrote:
> Then, correct KWin to kwindowsystem to start working. If there is no bug 
> this will works on all cases -> https://git.reviewboard.kde.org/r/127216/ but 
> it's not.
> 
> Martin Gräßlin wrote:
> I don't know why you have problems, but it's impossible that 
> KWindowSystem::self() returns a nullptr. You can check the code to verify. 
> Something is really broken on your side, but I don't know what. Maybe missing 
> plugins installed?
> 
> Thomas Lübking wrote:
> it's not a hard dependecy on the instace, the sni geometry handling is 
> just plain broken.
> the workaround operates on configure events on the mapped window which 
> will go nowhere if there's no running wm.
> 
> the client needs to handle the no-wm case, ie. configure the window 
> correctly, but to repeat myself: such requirement implies that either sni or 
> qwidget/qwindow is completely fucked up.
> 
> qwindow/qwidget::setGeometry should justwork(tm)
> 
> Anthony Fieroni wrote:
> setGeometry *not* works, i can confirm since i'm test it. Nothing works 
> if Kwin is started *after* usage of kwindowsystem::self().
> Martin yeah nullprt is impossible.
> 
> Martin Gräßlin wrote:
> then this is a bug in Qt's xcb plugin and needs to be fixed there. 
> Setting a geometry without a window manager must be possible.
> 
> Anthony Fieroni wrote:
> Wait a little, the bug is not in xcb. I'm not clear or what. This 
> *happend* only on session restored apps, when kwindowsystem::self() is before 
> kwin statup, only in this case. If run a app after kwin is started *all* 
> works fine setGeometry(), move() - no problems.
> 
> Anthony Fieroni wrote:
> Thomas Lübking wrote:
>  The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
>  
> For me, this is best explanation. Adn give you the easiest, no pain, 
> working solution - wait kwin to start completely.
> 
> Martin Gräßlin wrote:
> > Adn give you the easiest, no pain, working solution - wait kwin to 
> start completely.
> 
> I disagree. Working around such problems in the session startup seems 
> wrong. I would say excluse the sni windows from session restore, which can be 
> done from Qt API.
> 
> Thomas Lübking wrote:
> Can we please get few things straight?
> 
> You say that ::setGeometry doesn't work, but 
> https://git.reviewboard.kde.org/r/127216/diff/5#index_header makes use of it 
> (uses the propsed internally saved position and restores it with 
> QWidget::move what's basically a ::setGeometry special case)
> 
> => Does this work (leaving aside session restorage), yes or no?
> 
> Assuming it *does* work (except for session restorage) the claims to 
> require a WM for operative KWindowSystem invocation are gladfully, since 
> you're not using KWindowSystem at all.
> 
> 
> 
> Now onto session restorage:
> ---
> What exactly is the problem here?
> a) the windows are *not* visible when the session restarts. Showing them 
> show them in wrong p

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.
> 
> Thomas Lübking wrote:
> The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
> 
> That's however not a bug (at least it's intended) and I've no idea why 
> that's relevant to the weird geometry handling of SNI items (which looks 
> *fr* too complex - the position of remapped windows is usually maintained 
> by QWidget ...)
> 
> Anthony Fieroni wrote:
> I saw the code, it looks like KWindowSystem not depend on Kwin, but Kwin 
> *must* be started *before* use of kwindowsystem. Thomas may right, because 
> setGeometry even not work on sessions restored app.
> When session was finish if start apps with kmail --session session-id 
> everything wors fine.
> 
> Anthony Fieroni wrote:
> Now, after Thomas comment, i even think only widget->show() must works 
> because widget has internal frameGeometry and position.
> 
> Martin Gräßlin wrote:
> > but Kwin must be started before use of kwindowsystem
> 
> no, really there is no reason for that. KWindowSystem doesn't depend on a 
> window manager running.
> 
> Anthony Fieroni wrote:
> Then, correct KWin to kwindowsystem to start working. If there is no bug 
> this will works on all cases -> https://git.reviewboard.kde.org/r/127216/ but 
> it's not.
> 
> Martin Gräßlin wrote:
> I don't know why you have problems, but it's impossible that 
> KWindowSystem::self() returns a nullptr. You can check the code to verify. 
> Something is really broken on your side, but I don't know what. Maybe missing 
> plugins installed?
> 
> Thomas Lübking wrote:
> it's not a hard dependecy on the instace, the sni geometry handling is 
> just plain broken.
> the workaround operates on configure events on the mapped window which 
> will go nowhere if there's no running wm.
> 
> the client needs to handle the no-wm case, ie. configure the window 
> correctly, but to repeat myself: such requirement implies that either sni or 
> qwidget/qwindow is completely fucked up.
> 
> qwindow/qwidget::setGeometry should justwork(tm)
> 
> Anthony Fieroni wrote:
> setGeometry *not* works, i can confirm since i'm test it. Nothing works 
> if Kwin is started *after* usage of kwindowsystem::self().
> Martin yeah nullprt is impossible.
> 
> Martin Gräßlin wrote:
> then this is a bug in Qt's xcb plugin and needs to be fixed there. 
> Setting a geometry without a window manager must be possible.
> 
> Anthony Fieroni wrote:
> Wait a little, the bug is not in xcb. I'm not clear or what. This 
> *happend* only on session restored apps, when kwindowsystem::self() is before 
> kwin statup, only in this case. If run a app after kwin is started *all* 
> works fine setGeometry(), move() - no problems.
> 
> Anthony Fieroni wrote:
> Thomas Lübking wrote:
>  The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
>  
> For me, this is best explanation. Adn give you the easiest, no pain, 
> working solution - wait kwin to start completely.
> 
> Martin Gräßlin wrote:
> > Adn give you the easiest, no pain, working solution - wait kwin to 
> start completely.
> 
> I disagree. Working around such problems in the session startup seems 
> wrong. I would say excluse the sni windows from session restore, which can be 
> done from Qt API.
> 
> Thomas Lübking wrote:
> Can we please get few things straight?
> 
> You say that ::setGeometry doesn't work, but 
> https://git.reviewboard.kde.org/r/127216/diff/5#index_header makes use of it 
> (uses the propsed internally saved position and restores it with 
> QWidget::move what's basically a ::setGeometry special case)
> 
> => Does this work (leaving aside session restorage), yes or no?
> 
> Assuming it *does* work (except for session restorage) the claims to 
> require a WM for operative KWindowSystem invocation are gladfully, since 
> you're not using KWindowSystem at all.
> 
> 
> 
> Now onto session restorage:
> ---
> What exactly is the problem here?
> a) the windows are *not* visible when the session restarts. Showing them 
> show them in wrong p

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.
> 
> Thomas Lübking wrote:
> The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
> 
> That's however not a bug (at least it's intended) and I've no idea why 
> that's relevant to the weird geometry handling of SNI items (which looks 
> *fr* too complex - the position of remapped windows is usually maintained 
> by QWidget ...)
> 
> Anthony Fieroni wrote:
> I saw the code, it looks like KWindowSystem not depend on Kwin, but Kwin 
> *must* be started *before* use of kwindowsystem. Thomas may right, because 
> setGeometry even not work on sessions restored app.
> When session was finish if start apps with kmail --session session-id 
> everything wors fine.
> 
> Anthony Fieroni wrote:
> Now, after Thomas comment, i even think only widget->show() must works 
> because widget has internal frameGeometry and position.
> 
> Martin Gräßlin wrote:
> > but Kwin must be started before use of kwindowsystem
> 
> no, really there is no reason for that. KWindowSystem doesn't depend on a 
> window manager running.
> 
> Anthony Fieroni wrote:
> Then, correct KWin to kwindowsystem to start working. If there is no bug 
> this will works on all cases -> https://git.reviewboard.kde.org/r/127216/ but 
> it's not.
> 
> Martin Gräßlin wrote:
> I don't know why you have problems, but it's impossible that 
> KWindowSystem::self() returns a nullptr. You can check the code to verify. 
> Something is really broken on your side, but I don't know what. Maybe missing 
> plugins installed?
> 
> Thomas Lübking wrote:
> it's not a hard dependecy on the instace, the sni geometry handling is 
> just plain broken.
> the workaround operates on configure events on the mapped window which 
> will go nowhere if there's no running wm.
> 
> the client needs to handle the no-wm case, ie. configure the window 
> correctly, but to repeat myself: such requirement implies that either sni or 
> qwidget/qwindow is completely fucked up.
> 
> qwindow/qwidget::setGeometry should justwork(tm)
> 
> Anthony Fieroni wrote:
> setGeometry *not* works, i can confirm since i'm test it. Nothing works 
> if Kwin is started *after* usage of kwindowsystem::self().
> Martin yeah nullprt is impossible.
> 
> Martin Gräßlin wrote:
> then this is a bug in Qt's xcb plugin and needs to be fixed there. 
> Setting a geometry without a window manager must be possible.
> 
> Anthony Fieroni wrote:
> Wait a little, the bug is not in xcb. I'm not clear or what. This 
> *happend* only on session restored apps, when kwindowsystem::self() is before 
> kwin statup, only in this case. If run a app after kwin is started *all* 
> works fine setGeometry(), move() - no problems.
> 
> Anthony Fieroni wrote:
> Thomas Lübking wrote:
>  The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
>  
> For me, this is best explanation. Adn give you the easiest, no pain, 
> working solution - wait kwin to start completely.
> 
> Martin Gräßlin wrote:
> > Adn give you the easiest, no pain, working solution - wait kwin to 
> start completely.
> 
> I disagree. Working around such problems in the session startup seems 
> wrong. I would say excluse the sni windows from session restore, which can be 
> done from Qt API.

Can we please get few things straight?

You say that ::setGeometry doesn't work, but 
https://git.reviewboard.kde.org/r/127216/diff/5#index_header makes use of it 
(uses the propsed internally saved position and restores it with QWidget::move 
what's basically a ::setGeometry special case)

=> Does this work (leaving aside session restorage), yes or no?

Assuming it *does* work (except for session restorage) the claims to require a 
WM for operative KWindowSystem invocation are gladfully, since you're not using 
KWindowSystem at all.


Now onto session restorage:
---
What exactly is the problem here?
a) the windows are *not* visible when the session restarts. Showing them show 
them in wrong position
b) the windows *are* visible when the session restarts, but in wrong position?

In either case, the session restored geometry and the SNI internal geometry 
should *not* differ.

If they

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.
> 
> Thomas Lübking wrote:
> The entire thing sounds as if the problem would be that the session 
> restorage in kwin overrides the placement for restored windows (with the 
> restored position)
> 
> That's however not a bug (at least it's intended) and I've no idea why 
> that's relevant to the weird geometry handling of SNI items (which looks 
> *fr* too complex - the position of remapped windows is usually maintained 
> by QWidget ...)
> 
> Anthony Fieroni wrote:
> I saw the code, it looks like KWindowSystem not depend on Kwin, but Kwin 
> *must* be started *before* use of kwindowsystem. Thomas may right, because 
> setGeometry even not work on sessions restored app.
> When session was finish if start apps with kmail --session session-id 
> everything wors fine.
> 
> Anthony Fieroni wrote:
> Now, after Thomas comment, i even think only widget->show() must works 
> because widget has internal frameGeometry and position.
> 
> Martin Gräßlin wrote:
> > but Kwin must be started before use of kwindowsystem
> 
> no, really there is no reason for that. KWindowSystem doesn't depend on a 
> window manager running.
> 
> Anthony Fieroni wrote:
> Then, correct KWin to kwindowsystem to start working. If there is no bug 
> this will works on all cases -> https://git.reviewboard.kde.org/r/127216/ but 
> it's not.
> 
> Martin Gräßlin wrote:
> I don't know why you have problems, but it's impossible that 
> KWindowSystem::self() returns a nullptr. You can check the code to verify. 
> Something is really broken on your side, but I don't know what. Maybe missing 
> plugins installed?

it's not a hard dependecy on the instace, the sni geometry handling is just 
plain broken.
the workaround operates on configure events on the mapped window which will go 
nowhere if there's no running wm.

the client needs to handle the no-wm case, ie. configure the window correctly, 
but to repeat myself: such requirement implies that either sni or 
qwidget/qwindow is completely fucked up.

qwindow/qwidget::setGeometry should justwork(tm)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127631/#review94513
---


On April 10, 2016, 7:46 nachm., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127631/
> -------
> 
> (Updated April 10, 2016, 7:46 nachm.)
> 
> 
> Review request for kwin, Plasma, David Edmundson, Martin Gräßlin, and Thomas 
> Lübking.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> We must proceed with autoStart0 when kwin process is ended otherwise 
> kwindowsystem::self() is nullptr. Every restored session app cannot use 
> kwindowsystem. This depends of cpu speed, it can be faster enough to start wm 
> before ksmserver restore apps and kwindowsystem will be usable.
> 
> 
> Diffs
> -
> 
>   ksmserver/startup.cpp f118b55 
> 
> Diff: https://git.reviewboard.kde.org/r/127631/diff/
> 
> 
> Testing
> ---
> 
> It's needed to fix this bug -> https://git.reviewboard.kde.org/r/127216/
> I don't know how to proceed if kwin fails to start in every way, can we 
> process - i think not?
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127631: [ksmserver] We must be sure that kwin process is ended

[Thomas Lübking]

> On April 11, 2016, 5:45 vorm., Martin Gräßlin wrote:
> > > otherwise kwindowsystem::self() is nullptr
> > 
> > how can KWindowSystem::self() be null? And why should that have anything to 
> > do with KWin? KWindowSystem does not depend on a window manager running.

The entire thing sounds as if the problem would be that the session restorage 
in kwin overrides the placement for restored windows (with the restored 
position)

That's however not a bug (at least it's intended) and I've no idea why that's 
relevant to the weird geometry handling of SNI items (which looks *fr* too 
complex - the position of remapped windows is usually maintained by QWidget ...)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127631/#review94513
---


On April 10, 2016, 7:46 nachm., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127631/
> ---
> 
> (Updated April 10, 2016, 7:46 nachm.)
> 
> 
> Review request for kwin, Plasma, David Edmundson, Martin Gräßlin, and Thomas 
> Lübking.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> We must proceed with autoStart0 when kwin process is ended otherwise 
> kwindowsystem::self() is nullptr. Every restored session app cannot use 
> kwindowsystem. This depends of cpu speed, it can be faster enough to start wm 
> before ksmserver restore apps and kwindowsystem will be usable.
> 
> 
> Diffs
> -
> 
>   ksmserver/startup.cpp f118b55 
> 
> Diff: https://git.reviewboard.kde.org/r/127631/diff/
> 
> 
> Testing
> ---
> 
> It's needed to fix this bug -> https://git.reviewboard.kde.org/r/127216/
> I don't know how to proceed if kwin fails to start in every way, can we 
> process - i think not?
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127102: Use fixed width for digital clock applet

[Thomas Lübking]

> On Feb. 18, 2016, 12:05 a.m., David Edmundson wrote:
> > applets/digital-clock/package/contents/ui/DigitalClock.qml, line 555
> > <https://git.reviewboard.kde.org/r/127102/diff/1/?file=444552#file444552line555>
> >
> > rather than looping, can we use FontMetric's maximumCharacterWidth
> > 
> >  * numChars.
> > 
> > Then we could kill sizeHelper competely (FontMetric's didn't exist when 
> > this was written)
> 
> Marco Martin wrote:
> hoping maximumCharacterWidth is reliable for all fonts, this loop really 
> needs to go
> 
> Daniel Faust wrote:
> As far as I understand maximumCharacterWidth returns the width of the 
> widest character of the font - which can be ridiculously wide given that the 
> font supports some wild unicode characters.
> I did a quick test and maximumCharacterWidth returned about twice the 
> width actually needed.
> 
> Martin Klapetek wrote:
> You still don't need this whole loop though, just find the biggest in 0-9 
> and use that for all the numbers (except year).
> 
> Daniel Faust wrote:
> Since I don't know the time format in advance, I have to construct 
> different times and process them with Qt.formatTime.
> That means, I have to test 0-9 for hours/minutes/seconds, 0-2 for tens of 
> hours and 0-5 for tens of minutes/seconds. Otherwise the constructed times 
> will be invalid.
> This can be done with three loops and it would bring down the amount of 
> advanceWidth and formatTime calls from 24+60=84 to 3+6+10=19.
> 
> Martin Klapetek wrote:
> You don't actually need them as a time, you just need them as a 
> placeholder. At which point it doesn't matter if it's a valid time or not. 
> You can even format 12:12 with Qt.formatTime and then replace the numbers 
> with your placeholder number.
> 
> In other words, the time format /is/ known in advance.
> 
> Daniel Faust wrote:
> I'm not sure if I understand you, I just tried it with a single loop from 
> 0-9 to construct the time strings:
> 
> for (var i=0; i<=9; i++) {
>   var str = Qt.formatTime(new Date(2000, 0, 1, i*10 + i, i*10 + i, i*10 + 
> i), main.timeFormat);
>   console.log("hour: " + (i*10 + i) + ", minute: " + (i*10 + i) + " -> " 
> + str);
> }
> 
> qml: hour: 0, minute: 0 -> 00:00
> qml: hour: 11, minute: 11 -> 11:11
> qml: hour: 22, minute: 22 -> 22:22
> qml: hour: 33, minute: 33 -> 09:33
> qml: hour: 44, minute: 44 -> 20:44
> qml: hour: 55, minute: 55 -> 07:55
> qml: hour: 66, minute: 66 -> 19:07
> qml: hour: 77, minute: 77 -> 06:18
> qml: hour: 88, minute: 88 -> 17:29
> qml: hour: 99, minute: 99 -> 04:40
> 
> Thomas Lübking wrote:
> Forget about the timeFormat - you iterate from 0-9 and figure the widest 
> glyphs in [0,1], [0,5] and [0,9] - let's reasonably say "0,3 and 7".
> Then you check the width for 00:37
> 
> (If you also need the date, you also need to check for the widest glyph 
> in [0,3])
> 
> Daniel Faust wrote:
> That would work if it wasn't for 12 hour time formats.
> For 24 hour formats you actually need [0,2], [0,3] and [0,9] for the hour 
> and [0,5] and [0,9] for the minutes/seconds.
> For 12 hour formats AM you need [0,1], [0,2] and [0,9] for the hour and 
> [0,5] and [0,9] for the minutes/seconds.
> For 12 hour formats PM you need [1,2], [3,9] and [0,3] for the hour and 
> [0,5] and [0,9] for the minutes/seconds.
> On top of that you need to account for format changes like hour=0, 
> minute=0 -> "12:00 AM".
> 
> The code for this logic would be hard to maintain.
> 
> Daniel Faust wrote:
> Ok, what about this?
> Find the widest glyph between 0 and 9.
> Use a regex to replace [hmsz] from the format string with the widest 
> number. (This will produce strings like "44:44")
> Use Qt.formatTime once with an AM time and once with a PM time and choose 
> the widest of the produced strings.
> This code is fairly short and overestimates the total width only slightly.

As long as the formatter accepts invalid times, that's certainly the most 
straight forward solution, yes.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127102/#review92514
---


On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply

Re: Review Request 127102: Use fixed width for digital clock applet

[Thomas Lübking]

> On Feb. 18, 2016, 12:05 a.m., David Edmundson wrote:
> > applets/digital-clock/package/contents/ui/DigitalClock.qml, line 555
> > 
> >
> > rather than looping, can we use FontMetric's maximumCharacterWidth
> > 
> >  * numChars.
> > 
> > Then we could kill sizeHelper competely (FontMetric's didn't exist when 
> > this was written)
> 
> Marco Martin wrote:
> hoping maximumCharacterWidth is reliable for all fonts, this loop really 
> needs to go
> 
> Daniel Faust wrote:
> As far as I understand maximumCharacterWidth returns the width of the 
> widest character of the font - which can be ridiculously wide given that the 
> font supports some wild unicode characters.
> I did a quick test and maximumCharacterWidth returned about twice the 
> width actually needed.
> 
> Martin Klapetek wrote:
> You still don't need this whole loop though, just find the biggest in 0-9 
> and use that for all the numbers (except year).
> 
> Daniel Faust wrote:
> Since I don't know the time format in advance, I have to construct 
> different times and process them with Qt.formatTime.
> That means, I have to test 0-9 for hours/minutes/seconds, 0-2 for tens of 
> hours and 0-5 for tens of minutes/seconds. Otherwise the constructed times 
> will be invalid.
> This can be done with three loops and it would bring down the amount of 
> advanceWidth and formatTime calls from 24+60=84 to 3+6+10=19.
> 
> Martin Klapetek wrote:
> You don't actually need them as a time, you just need them as a 
> placeholder. At which point it doesn't matter if it's a valid time or not. 
> You can even format 12:12 with Qt.formatTime and then replace the numbers 
> with your placeholder number.
> 
> In other words, the time format /is/ known in advance.
> 
> Daniel Faust wrote:
> I'm not sure if I understand you, I just tried it with a single loop from 
> 0-9 to construct the time strings:
> 
> for (var i=0; i<=9; i++) {
>   var str = Qt.formatTime(new Date(2000, 0, 1, i*10 + i, i*10 + i, i*10 + 
> i), main.timeFormat);
>   console.log("hour: " + (i*10 + i) + ", minute: " + (i*10 + i) + " -> " 
> + str);
> }
> 
> qml: hour: 0, minute: 0 -> 00:00
> qml: hour: 11, minute: 11 -> 11:11
> qml: hour: 22, minute: 22 -> 22:22
> qml: hour: 33, minute: 33 -> 09:33
> qml: hour: 44, minute: 44 -> 20:44
> qml: hour: 55, minute: 55 -> 07:55
> qml: hour: 66, minute: 66 -> 19:07
> qml: hour: 77, minute: 77 -> 06:18
> qml: hour: 88, minute: 88 -> 17:29
> qml: hour: 99, minute: 99 -> 04:40

Forget about the timeFormat - you iterate from 0-9 and figure the widest glyphs 
in [0,1], [0,5] and [0,9] - let's reasonably say "0,3 and 7".
Then you check the width for 00:37

(If you also need the date, you also need to check for the widest glyph in 
[0,3])


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127102/#review92514
---


On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127102/
> ---
> 
> (Updated Feb. 17, 2016, 4:23 p.m.)
> 
> 
> Review request for kde-workspace and Plasma.
> 
> 
> Bugs: 347724
> https://bugs.kde.org/show_bug.cgi?id=347724
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> Currently the width of the date label is not fixed but changes depending on 
> the text. This causes the entire applet to change its width (if the time is 
> the widest displayed item). This in turn can cause all other applets in the 
> same panel to move whenever the displayed time changes.
> 
> This patch uses FontMetrics to iterate over all possible time strings (with 
> different width) and chooses the widest of them as reference for the fixed 
> width of the time label.
> 
> This way the width of the applet stays the same (unless the date is displayed 
> and changes). The text remains centered though, which means that it can still 
> move within the applet when the time changes.
> 
> 
> Diffs
> -
> 
>   applets/digital-clock/package/contents/ui/DigitalClock.qml 95bb071 
> 
> Diff: https://git.reviewboard.kde.org/r/127102/diff/
> 
> 
> Testing
> ---
> 
> Works with horizontal and vertical panel.
> Also displaying different combinations of "seconds", "date" and "timezone" 
> works.
> 
> 
> Thanks,
> 
> Daniel Faust
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

> On Feb. 17, 2016, 2:06 p.m., Thomas Lübking wrote:
> > effects/morphingpopups/package/contents/code/morphingpopups.js, line 27
> > <https://git.reviewboard.kde.org/r/126968/diff/5/?file=98#file98line27>
> >
> > Hold it! ;-)
> > 
> > This is still error prone. Just keep the animation variables, try to 
> > retarget and if that fails add a new animation.
> > 
> > animationEnded means that *some* animation has ended, ie. you clean 
> > move also when fade ended. Once somebody decides to alter durations, you'll 
> > run into conflicts.
> 
> Marco Martin wrote:
> ok, sorry we didn't get that exactly.
> I'll do a new review request on top of current master, ok?

Yes, of course.
After my yesterdays commit mess, I'm not in any position to criticize anybody 
on early commits =)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review92489
---


On Feb. 17, 2016, 2:03 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 17, 2016, 2:03 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863 and 349669
> https://bugs.kde.org/show_bug.cgi?id=347863
> https://bugs.kde.org/show_bug.cgi?id=349669
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt e3beebf 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review92489
---




effects/morphingpopups/package/contents/code/morphingpopups.js (line 27)
<https://git.reviewboard.kde.org/r/126968/#comment63052>

Hold it! ;-)

This is still error prone. Just keep the animation variables, try to 
retarget and if that fails add a new animation.

animationEnded means that *some* animation has ended, ie. you clean move 
also when fade ended. Once somebody decides to alter durations, you'll run into 
conflicts.


- Thomas Lübking


On Feb. 17, 2016, 2:03 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 17, 2016, 2:03 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863 and 349669
> https://bugs.kde.org/show_bug.cgi?id=347863
> https://bugs.kde.org/show_bug.cgi?id=349669
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt e3beebf 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review92455
---




effects/morphingpopups/package/contents/code/morphingpopups.js (line 29)
<https://git.reviewboard.kde.org/r/126968/#comment63041>

delete window.moveAnimation;

but this is still fishy, since you don't know whether you clean up because 
the "correct" animation ended.

=> if you have an animation, try to retarget it first and start a new 
animation because and if that failed (retarget returning false)


- Thomas Lübking


On Feb. 11, 2016, 8:52 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 11, 2016, 8:52 a.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt e3beebf 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review92457
---




effects/morphingpopups/package/contents/code/morphingpopups.js (line 67)
<https://git.reviewboard.kde.org/r/126968/#comment63043>

I guess that's not required - you'll either have two entries or none.


- Thomas Lübking


On Feb. 11, 2016, 8:52 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 11, 2016, 8:52 a.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt e3beebf 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

> On Feb. 2, 2016, 4:40 p.m., Thomas Lübking wrote:
> > effects/morphingpopups/package/contents/code/morphingpopups.js, line 86
> > <https://git.reviewboard.kde.org/r/126968/diff/1/?file=442552#file442552line86>
> >
> > a) let's have AnimationEffect::retarget(id, values...)?! - progress is 
> > a private detail of AnimationEffect.
> > if that returns "bool", you also get around the cleanup issue.
> > 
> > b) something tells me the duration should probably shorten in this case?
> 
> Marco Martin wrote:
> will give it a try
> 
> Thomas Lübking wrote:
> I'll have to revisit the animationeffect class anyway because of the 
> segfault and will implement a function to retarget an animation, so you don't 
> have to spend too much time on it.
> 
> Marco Martin wrote:
> ok, thanks, I'll wait with this to adapt to the new method when done then
> 
> Marco Martin wrote:
> any update on AnimationEffect::retarget() ? do you plan to work on that 
> or i can give a try?

I did attach you to https://git.reviewboard.kde.org/r/126981/ ;-)
It's just not been tested and I waited on a comment from you before shipping it 
*lol*


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review91965
---


On Feb. 2, 2016, 8:51 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 2, 2016, 8:51 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt dec50a9 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
>   libkwineffects/kwinanimationeffect.h f59eedc 
>   libkwineffects/kwinanimationeffect.cpp 579535b 
>   scripting/scriptedeffect.h 2a77a2f 
>   scripting/scriptedeffect.cpp 7ab065b 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126980: Scale blurbehind and backgroundcontrast besides translating

[Thomas Lübking]

> On Feb. 3, 2016, 9:43 p.m., Thomas Lübking wrote:
> > effects/backgroundcontrast/contrast.cpp, line 383
> > <https://git.reviewboard.kde.org/r/126980/diff/1/?file=442652#file442652line383>
> >
> > This looks fishy - and QTransform a tiny bit clumsy.
> > 
> > Does this work:
> > 
> > QVector shapeRects = shape.rects();
> > shape = QRegion(); // clear
> > foreach (QRect r, shapeRects) {
> >    r.moveTo(r.x() * data.xScale() + data.xTranslation(),
> > r.y() * data.yScale() + data.yTranslation());
> >r.setWidth(r.width() * data.xScale());
> >r.setHeight(r.height() * data.yScale());
> >shape |= r;
> > }
> > 
> > the multiplications might need to be put into qRound or qCeil to avoid 
> > glitches.
> > 
> > This spares several qregion copies, conversions into and out of painter 
> > paths and the second shape translation.
> 
> Marco Martin wrote:
> hmm, nope, translation seems off.
> investigating
> 
> Marco Martin wrote:
> to make it work, i have to do it like before:
> QPoint pt = shape.boundingRect().topLeft();
> QVector shapeRects = shape.rects();
> shape = QRegion(); // clear
> foreach (QRect r, shapeRects) {
> r.moveTo((r.x() - pt.x()) * data.xScale() + 
> data.xTranslation(),
> (r.y() -pt.y()) * data.yScale() + 
> data.yTranslation());
> r.setWidth(r.width() * data.xScale());
> r.setHeight(r.height() * data.yScale());
> shape |= r;
> }
> shape = shape.translated(pt.x(), pt.y());
> shape = shape & region;
> 
> Marco Martin wrote:
> so, doesn't seem related to qtransform, but when the region is scaled, 
> its boundingrect just has to be at 0,0 then retranslated back afterwards

r.moveTo(pt.x() + (r.x() - pt.x()) * data.xScale() + data.xTranslation()


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126980/#review92037
---


On Feb. 3, 2016, 3:35 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126980/
> ---
> 
> (Updated Feb. 3, 2016, 3:35 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> related to https://git.reviewboard.kde.org/r/126968/
> this is only the part which adds support for scaled windows to blur and 
> contrast
> 
> 
> Diffs
> -
> 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
> 
> Diff: https://git.reviewboard.kde.org/r/126980/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

> On Feb. 3, 2016, 12:18 a.m., Xuetian Weng wrote:
> > I just wonder, does this change also fix: 
> > https://bugs.kde.org/show_bug.cgi?id=241557 ?
> > 
> > Also, I have impression that if there's a big difference between the size 
> > of two tooltip window, it will look bad.
> 
> Thomas Lübking wrote:
> No, completely unrelated issue.
> 
> Marco Martin wrote:
> tough it may help, since now blur/contrast do respect the scale factor of 
> windows as well?

My guess is that the correct regions will show up blurred, but they'll show the 
wrong content.
Could you nevertheless turn that part into a commit of its own?


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review92000
---


On Feb. 2, 2016, 8:51 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 2, 2016, 8:51 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt dec50a9 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
>   libkwineffects/kwinanimationeffect.h f59eedc 
>   libkwineffects/kwinanimationeffect.cpp 579535b 
>   scripting/scriptedeffect.h 2a77a2f 
>   scripting/scriptedeffect.cpp 7ab065b 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126980: Scale blurbehind and backgroundcontrast besides translating

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126980/#review92037
---




effects/backgroundcontrast/contrast.cpp (line 379)
<https://git.reviewboard.kde.org/r/126980/#comment62793>

QPoint pt = shape.boundingRect().topLeft();

rather than calling boundingRect() tiwce.



effects/backgroundcontrast/contrast.cpp (line 381)
<https://git.reviewboard.kde.org/r/126980/#comment62792>

you don't have to (and should not) re-assign the value, the returned 
reference if for building call chains like t.scale().translate().map()



effects/backgroundcontrast/contrast.cpp (line 382)
<https://git.reviewboard.kde.org/r/126980/#comment62794>

This looks fishy - and QTransform a tiny bit clumsy.

Does this work:

QVector shapeRects = shape.rects();
shape = QRegion(); // clear
foreach (QRect r, shapeRects) {
   r.moveTo(r.x() * data.xScale() + data.xTranslation(),
r.y() * data.yScale() + data.yTranslation());
   r.setWidth(r.width() * data.xScale());
   r.setHeight(r.height() * data.yScale());
   shape |= r;
}

the multiplications might need to be put into qRound or qCeil to avoid 
glitches.

This spares several qregion copies, conversions into and out of painter 
paths and the second shape translation.


- Thomas Lübking


On Feb. 3, 2016, 3:35 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126980/
> ---
> 
> (Updated Feb. 3, 2016, 3:35 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> related to https://git.reviewboard.kde.org/r/126968/
> this is only the part which adds support for scaled windows to blur and 
> contrast
> 
> 
> Diffs
> -
> 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
> 
> Diff: https://git.reviewboard.kde.org/r/126980/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review92023
---




effects/blur/blur.cpp (line 437)
<https://git.reviewboard.kde.org/r/126968/#comment62783>

please move data.*Translation() as well as the forth/back translation into 
the QTransform to avoid superflous region operations


- Thomas Lübking


On Feb. 2, 2016, 8:51 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 2, 2016, 8:51 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt dec50a9 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
>   libkwineffects/kwinanimationeffect.h f59eedc 
>   libkwineffects/kwinanimationeffect.cpp 579535b 
>   scripting/scriptedeffect.h 2a77a2f 
>   scripting/scriptedeffect.cpp 7ab065b 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

> On Feb. 2, 2016, 4:40 nachm., Thomas Lübking wrote:
> > effects/morphingpopups/package/contents/code/morphingpopups.js, line 86
> > <https://git.reviewboard.kde.org/r/126968/diff/1/?file=442552#file442552line86>
> >
> > a) let's have AnimationEffect::retarget(id, values...)?! - progress is 
> > a private detail of AnimationEffect.
> > if that returns "bool", you also get around the cleanup issue.
> > 
> > b) something tells me the duration should probably shorten in this case?
> 
> Marco Martin wrote:
> will give it a try

I'll have to revisit the animationeffect class anyway because of the segfault 
and will implement a function to retarget an animation, so you don't have to 
spend too much time on it.


> On Feb. 2, 2016, 4:40 nachm., Thomas Lübking wrote:
> > effects/morphingpopups/package/contents/code/morphingpopups.js, line 70
> > <https://git.reviewboard.kde.org/r/126968/diff/1/?file=442552#file442552line70>
> >
> > enums (KWin.* should work, but am not sure)
> 
> Marco Martin wrote:
> doesn't seems to work

Meh - we should wire the enum down to the effect then, using hardcoded integers 
for this purpose is ugly (and error prone)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review91965
---


On Feb. 2, 2016, 8:51 nachm., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 2, 2016, 8:51 nachm.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt dec50a9 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
>   libkwineffects/kwinanimationeffect.h f59eedc 
>   libkwineffects/kwinanimationeffect.cpp 579535b 
>   scripting/scriptedeffect.h 2a77a2f 
>   scripting/scriptedeffect.cpp 7ab065b 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

> On Feb. 3, 2016, 12:18 a.m., Xuetian Weng wrote:
> > I just wonder, does this change also fix: 
> > https://bugs.kde.org/show_bug.cgi?id=241557 ?
> > 
> > Also, I have impression that if there's a big difference between the size 
> > of two tooltip window, it will look bad.

No, completely unrelated issue.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review92000
---


On Feb. 2, 2016, 8:51 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 2, 2016, 8:51 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt dec50a9 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
>   libkwineffects/kwinanimationeffect.h f59eedc 
>   libkwineffects/kwinanimationeffect.cpp 579535b 
>   scripting/scriptedeffect.h 2a77a2f 
>   scripting/scriptedeffect.cpp 7ab065b 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review91965
---




effects/morphingpopups/package/contents/code/morphingpopups.js (line 29)
<https://git.reviewboard.kde.org/r/126968/#comment62744>

backtrace?



effects/morphingpopups/package/contents/code/morphingpopups.js (line 52)
<https://git.reviewboard.kde.org/r/126968/#comment62745>

calc distance only once. maybe operate on manhattan length?



effects/morphingpopups/package/contents/code/morphingpopups.js (line 63)
<https://git.reviewboard.kde.org/r/126968/#comment62746>

old/new > 4 - presently looks like you're avoiding small ratios on first 
glance



effects/morphingpopups/package/contents/code/morphingpopups.js (line 70)
<https://git.reviewboard.kde.org/r/126968/#comment62747>

enums (KWin.* should work, but am not sure)



effects/morphingpopups/package/contents/code/morphingpopups.js (line 86)
<https://git.reviewboard.kde.org/r/126968/#comment62748>

a) let's have AnimationEffect::retarget(id, values...)?! - progress is a 
private detail of AnimationEffect.
if that returns "bool", you also get around the cleanup issue.

b) something tells me the duration should probably shorten in this case?



effects/morphingpopups/package/contents/code/morphingpopups.js (line 143)
<https://git.reviewboard.kde.org/r/126968/#comment62749>

    you should not even be here then ;-)


- Thomas Lübking


On Feb. 2, 2016, 2:04 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 2, 2016, 2:04 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt dec50a9 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
>   libkwineffects/kwinanimationeffect.h f59eedc 
>   libkwineffects/kwinanimationeffect.cpp 579535b 
>   scripting/scriptedeffect.h 2a77a2f 
>   scripting/scriptedeffect.cpp 7ab065b 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126968: Morphingpopups effect, to animate tooltips

[Thomas Lübking]

> On Feb. 2, 2016, 4:40 p.m., Thomas Lübking wrote:
> > effects/morphingpopups/package/contents/code/morphingpopups.js, line 29
> > <https://git.reviewboard.kde.org/r/126968/diff/1/?file=442552#file442552line29>
> >
> > backtrace?
> 
> Martin Gräßlin wrote:
> I just created a test case for it: https://paste.kde.org/pywvjhtx9 and it 
> creates this backtrace: https://paste.kde.org/pcxthpmx3
> 
> It sounds valid to me. It deletes AnimData from JS side and the 
> AnimationEffect than tries to delete it as well. Why JS is allowed to delete 
> it, is not clear to me, though

The problem is the cancel call from the animationEnded emit - this causes an 
erase of what's the present iterator... which is then erased as well.

We probably need to harden the lib code itr., but the script is of course wrong:
a) there's no point in cancelling an animation which has just ended (deleting 
the property is fine)
b) to know which animation has ended, the animation needs to be tagged in the 
metadata


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126968/#review91965
---


On Feb. 2, 2016, 2:04 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126968/
> ---
> 
> (Updated Feb. 2, 2016, 2:04 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Bugs: 347863
> https://bugs.kde.org/show_bug.cgi?id=347863
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> this effect, derived from the Maximize one, will take the place of the manual 
> window position animation that Plasma tooltip are using.
> this should cause less problems as animationg positions on X is very error 
> prone, plus it's less jarring when the tooltip sizes changes too, since that 
> gets animated as well (behavior similar to Window7 taskbar tooltips)
> look:
> https://www.youtube.com/watch?v=sxE23ZgkkpU
> slow motion:
> https://www.youtube.com/watch?v=jDByfncO568
> 
> replaces https://git.reviewboard.kde.org/r/126870
> 
> 
> Diffs
> -
> 
>   effects/CMakeLists.txt dec50a9 
>   effects/backgroundcontrast/contrast.cpp 168deb0 
>   effects/blur/blur.cpp a360f03 
>   effects/morphingpopups/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/CMakeLists.txt PRE-CREATION 
>   effects/morphingpopups/package/contents/code/morphingpopups.js PRE-CREATION 
>   effects/morphingpopups/package/metadata.desktop PRE-CREATION 
>   libkwineffects/kwinanimationeffect.h f59eedc 
>   libkwineffects/kwinanimationeffect.cpp 579535b 
>   scripting/scriptedeffect.h 2a77a2f 
>   scripting/scriptedeffect.cpp 7ab065b 
> 
> Diff: https://git.reviewboard.kde.org/r/126968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126906: Simplified fullscreen blur for Application Dashboard

[Thomas Lübking]

> On Jan. 27, 2016, 5:22 p.m., Thomas Lübking wrote:
> > effects/blur/blur.cpp, line 474
> > <https://git.reviewboard.kde.org/r/126906/diff/1/?file=441789#file441789line474>
> >
> > Did you try the behavior on a varying scene?
> > 
> > Generating mipmaps isn't exactly cheap and linear interpolation suffers 
> > from artifacts when things below move (I had tried an even smarter trick 
> > for general blurring, looks stunning and is incredibly fast ... as long as 
> > you deal with static contents)
> 
> Martin Gräßlin wrote:
> just tried a youtube video + the blur on my Ivybridge system. Didn't 
> notice any problems. That's of course not a scientific measurement ;-)

Harsh contrast changes would cause "flicker" - playing a video would likely be 
more a CPU/GPU overhead (mipmapping) concern.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126906/#review91675
---


On Jan. 27, 2016, 1:24 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126906/
> ---
> 
> (Updated Jan. 27, 2016, 1:24 p.m.)
> 
> 
> Review request for kwin, Plasma and Eike Hein.
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> [kwineffects] Expose fullScreen property in EffectWindow
> Also copied to Deleted.
> 
> [effects] Add a simplified fullscreen blur
> 
> If a window is fullscreen and wants fullscreen blur behind it, we
> use the blur from logout effect. This is mostly intended for the
> Application Dashboard which requires a fullscreen blur. The generic
> blur effect is not designed for such usage and is rather costly.
> 
> This simplified blur just needs framebuffer blit and midmaps. This
> makes it rather cheap in usage and also doesn't need a cached texture.
> 
> 
> Diffs
> -
> 
>   deleted.h bb87ae9611a5b59a5b37cf5a4cd38e99ed987069 
>   deleted.cpp 239ba8fec76ad520728182faf6429be8730ebec1 
>   effects/blur/blur.h fd5a020688d0e4397ce18e03aa4f79565418e9c5 
>   effects/blur/blur.cpp a360f0301e2983d0fb0bf3e71f95ac46ff22 
>   libkwineffects/kwineffects.h 4350e2b1c86252af43186164b10ad55fa388266e 
>   libkwineffects/kwineffects.cpp b767f6671284295d2e81b023ef62b24fcca8929a 
> 
> Diff: https://git.reviewboard.kde.org/r/126906/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> Dashboard with new algorithm.
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/01/27/b6607afa-cf10-4cd8-a490-7b56de4faaec__Spectacle.o12214.png
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126906: Simplified fullscreen blur for Application Dashboard

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126906/#review91675
---




effects/blur/blur.cpp (line 446)
<https://git.reviewboard.kde.org/r/126906/#comment62628>

move that ugly ::boundingRect test to the end?



effects/blur/blur.cpp (line 474)
<https://git.reviewboard.kde.org/r/126906/#comment62629>

Did you try the behavior on a varying scene?

Generating mipmaps isn't exactly cheap and linear interpolation suffers 
from artifacts when things below move (I had tried an even smarter trick for 
general blurring, looks stunning and is incredibly fast ... as long as you deal 
with static contents)


- Thomas Lübking


On Jan. 27, 2016, 1:24 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126906/
> ---
> 
> (Updated Jan. 27, 2016, 1:24 p.m.)
> 
> 
> Review request for kwin, Plasma and Eike Hein.
> 
> 
> Repository: kwin
> 
> 
> Description
> ---
> 
> [kwineffects] Expose fullScreen property in EffectWindow
> Also copied to Deleted.
> 
> [effects] Add a simplified fullscreen blur
> 
> If a window is fullscreen and wants fullscreen blur behind it, we
> use the blur from logout effect. This is mostly intended for the
> Application Dashboard which requires a fullscreen blur. The generic
> blur effect is not designed for such usage and is rather costly.
> 
> This simplified blur just needs framebuffer blit and midmaps. This
> makes it rather cheap in usage and also doesn't need a cached texture.
> 
> 
> Diffs
> -
> 
>   deleted.h bb87ae9611a5b59a5b37cf5a4cd38e99ed987069 
>   deleted.cpp 239ba8fec76ad520728182faf6429be8730ebec1 
>   effects/blur/blur.h fd5a020688d0e4397ce18e03aa4f79565418e9c5 
>   effects/blur/blur.cpp a360f0301e2983d0fb0bf3e71f95ac46ff22 
>   libkwineffects/kwineffects.h 4350e2b1c86252af43186164b10ad55fa388266e 
>   libkwineffects/kwineffects.cpp b767f6671284295d2e81b023ef62b24fcca8929a 
> 
> Diff: https://git.reviewboard.kde.org/r/126906/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> Dashboard with new algorithm.
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/01/27/b6607afa-cf10-4cd8-a490-7b56de4faaec__Spectacle.o12214.png
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126713: fix font preview colors

[Thomas Lübking]

> On Jan. 10, 2016, 10:31 p.m., David Edmundson wrote:
> > Can I suggest you copy SNIProxy::convertFromNative from 
> > plasma-workspace/xembedsniproxy/sniproxy.cpp
> > 
> > (which in itself is copied from KWindowSystem, which I think was copied 
> > from Qt)
> 
> David Edmundson wrote:
> ...or maybe I should be the one copying this.  I have a completely lying 
> comment about Format_RGB30 not existing.
> 
> Thomas Lübking wrote:
> The 30 and some 32 bit formats are more recent additions to Qt5 (5.4, 
> iirc)
> 
> I think I even suggested to have kxutils somewhere public - seems it's 
> becoming more demanding ;-)
> 
> (We'd proabably need to port the DefaultVisual stuff there, I simply used 
> what's already used in FcEngine)
> 
> Martin?

So, what do we do
Push this into 5.5 and seek for a public kxutils to rely on in the next 
frameworks release?


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126713/#review90858
-----------


On Jan. 10, 2016, 10:48 p.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126713/
> ---
> 
> (Updated Jan. 10, 2016, 10:48 p.m.)
> 
> 
> Review request for Plasma, Martin Gräßlin and Martin Klapetek.
> 
> 
> Bugs: 336089
> https://bugs.kde.org/show_bug.cgi?id=336089
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> the image format was hardcoded to ARGB32_Premultiplied, but that's not 
> generally correct, notably not on 24bit servers
> 
> 
> Diffs
> -
> 
>   kcms/kfontinst/lib/FcEngine.cpp 19b7206 
> 
> Diff: https://git.reviewboard.kde.org/r/126713/diff/
> 
> 
> Testing
> ---
> 
> yes, bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126713: fix font preview colors

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126713/
---

(Updated Jan. 15, 2016, 1:12 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma, Martin Gräßlin and Martin Klapetek.


Changes
---

Submitted with commit 9977edac12d6bec56f8d9cf7f97529177ca842eb by Thomas 
Lübking to branch Plasma/5.5.


Bugs: 336089
https://bugs.kde.org/show_bug.cgi?id=336089


Repository: plasma-desktop


Description
---

the image format was hardcoded to ARGB32_Premultiplied, but that's not 
generally correct, notably not on 24bit servers


Diffs
-

  kcms/kfontinst/lib/FcEngine.cpp 19b7206 

Diff: https://git.reviewboard.kde.org/r/126713/diff/


Testing
---

yes, bug.


Thanks,

Thomas Lübking

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel