Bug 278760 - SlidingPopups protocol requires screen geometry mapping on client or server side
Summary: SlidingPopups protocol requires screen geometry mapping on client or server side
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: effects-various (show other bugs)
Version: git master
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-29 08:53 UTC by Matthias Fuchs
Modified: 2018-08-18 15:03 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Fixes issue at least with Xinerama (421 bytes, patch)
2011-07-29 08:53 UTC, Matthias Fuchs
Details
Patch to fix the issues for PopupApplet _not_ KRunner (538 bytes, patch)
2011-07-29 09:17 UTC, Matthias Fuchs
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Fuchs 2011-07-29 08:53:45 UTC
Created attachment 62294 [details]
Fixes issue at least with Xinerama

Version:           unspecified (using Devel) 
OS:                Linux

Animations of displaying PopupApplets which are in the top panel start at the wrong y (too low) coordinate.

Reproducible: Always

Steps to Reproduce:
1. Have two monitors and create either layout a) or b):
 a) Same layout as described in BUG #256835 here monitor 0 will mean the right one, while monitor 1 is the left one
 b) One monitor setuped to be on top (called monitor 0 here), the other to be bellow (called monitor 1 here)
2. Place a panel at the top of monitor 1
3. Add the comic plasmoid, the analog clock or any other PopupApplet to the panel
4. Click the PopupApplet to display its contents

Actual Results:  
The animation starts at the wrong y-coordinate (y is too high), this is especially recogniseable if you choose layout 1. b)

Expected Results:  
The animation should always start at the top not bellow.

The attached patch fixes the issue for PopupApplets but not for KRunner, where it would in fact work the same way.

I have no clue why the current code does not seem to work with the real y coordinates while it does with the real x ones.

E.g. Take scenario 1. b) with a 1280x1024 display at top and a 1680x1050 display at bottom. Previously the y was 1024, that is the place the display 1 starts. Yet this causes the animation to way below: I guess at 1024 + 1024 - widget->height() in absolute coordiantes.

So maybe the reason for that might be that either XChangeProperty only wants absolute y-coordinates. Or it could mean that there is a bug somewhere in XChangeProperty so that screen.top() is added to the y coordinate.

NOTE:
In any case I have no clue if this bug only exists with Xinerama or also with other setups.

NOTE2:
If you apply the patch and use layout 1. b) you can see the animation start on screen 0, though that has in general nothing to do with this patch but is rather a problem with the way the animations are handled, i.e. moving the widget from start = widget.top() - widget.height() to widget.top(), while start is then naturally on screen 0.
Comment 1 Matthias Fuchs 2011-07-29 08:55:26 UTC
Adding KWin as CC as they might know more about this problem.
Comment 2 Matthias Fuchs 2011-07-29 09:17:52 UTC
Created attachment 62296 [details]
Patch to fix the issues for PopupApplet _not_ KRunner

I tested some more and interestingly the same issue exists with animations starting from Left, but both Bottom and Right are not affected.

All the other notes taken above remain valid.
Comment 3 Matthias Fuchs 2011-07-29 09:35:18 UTC
I think this is rather a bug in KWin SlidingPopupsEffect::paintWindow
Comment 4 Martin Flöser 2011-07-30 06:49:18 UTC
I think the idea was to start the animation from the top of the panel and not from top of the screen - at least for the popups.
Comment 5 Matthias Fuchs 2011-07-30 09:29:49 UTC
In either case in the scenario described above (1. b)) they start not at the correct position rather at the middle of the screen and moving back from there.
Comment 6 Thomas Lübking 2011-07-30 19:17:26 UTC
The start & stop geometries are piped into the effect from outside (the panel, popup, whatever controls it in the client) - the effect doesn't care about screen geometries &  struts at all -> no idea whether this is still notmart's issue, but it's on the client's (in this case plasma) side.
Comment 7 Matthias Fuchs 2011-07-31 11:17:43 UTC
That is true, though the start and stop geometries that are pushed into it are the correct absolute geometries.

This is also why I mentioned that I am not sure if absolute geomtries are allowed at all or if relative (to the screen) geomtries should be used in all cases.
Comment 8 Thomas Lübking 2011-07-31 13:12:20 UTC
Ahhh, ok - i missed the 1024+1024-h part in your ... longer ... OP ;-)

From the discussion i thought there was an offset against an absent panel.

You're however right (except that there's certainly no such bug in XChangeProperty..) - compositing happens "per screen" while under Xrandr or TwinView multiscreen QWidget::geometry() is relative a combined screen area.

This needs to be mapped, and actually preferable on the server (kwin effect) side so we fix it w/o changing the "protocol".
Comment 9 Matthias Fuchs 2011-07-31 14:23:23 UTC
So in that case it should be fixed in slidingpopups.cpp?
I wonder if the QRegion in SlidingPopupsEffect::paintWindow is correct in this regard.
Comment 10 Thomas Lübking 2011-07-31 16:20:16 UTC
yes ... slidingpopups.cpp should be fixed.
"mAppearingWindows[ w ]" *sigh*

anyways, back on topic: what QRegion do you mean? the paintWindow parameter? That's window related and should be fine (place this window across to screens, scroll. works? works! -> that region is ok ;-)

The fix should happen in slotPropertyNotify() and that points need to be offset by i guess "effects->clientArea(ScreenArea, EffectWindow *w).topLeft()"
Comment 11 Martin Flöser 2011-12-10 21:41:56 UTC
Git commit cbdd7295d100b19ec55d4b88f2a8113095439e26 by Martin Gräßlin.
Committed on 10/12/2011 at 12:31.
Pushed by graesslin into branch 'master'.

Fixing incorrect clipping of sliding popups

Make use of new extension of protocol for magic number -1.
If offset is -1 KWin has to decide the offset. This fixes all the
incorrect animations and allows us to perform clipping again by
filtering out the window quads which should not be visible.

Additionally the effect now sanitizes the offset. That is for e.g.
Yakuake setting an offset of 0, but there is a strut on the top
corner causing Yakuake not to appear on 0, but with an offset of
the strut. Such cases are now considered as well and the animation
is fixed.

REVIEW: 103367
BUG: 287602
CCBUG: 261159
CCBUG: 278760
FIXED-IN: 4.8.0

M  +115  -37   kwin/effects/slidingpopups/slidingpopups.cpp

http://commits.kde.org/kde-workspace/cbdd7295d100b19ec55d4b88f2a8113095439e26
Comment 12 Vlad Zahorodnii 2018-08-18 07:31:49 UTC
@Martin

Is this bug still actual? Can it be marked as RESOLVED?
Comment 13 Martin Flöser 2018-08-18 14:54:00 UTC
Good question... No new comment for seven years: I assume we can consider it fixed.
Comment 14 Vlad Zahorodnii 2018-08-18 15:03:39 UTC
Okay then, marking as fixed.