[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request 116021: Migrate all effects from displayWidth()/displayHeight() to clientArea(Ful
From: Fredrik_Höglund <fredrik () kde ! org>
Date: 2014-02-26 0:42:19
Message-ID: 20140226004219.11047.61093 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Feb. 24, 2014, 6:11 p.m., Fredrik Höglund wrote:
> > clientArea(FullArea) is a really bad name for a function that returns the screen \
> > size. The name practically screams "subset-of-the-screen-geometry".
> >
>
> Martin Gräßlin wrote:
> I also thought about it and whether it makes sense to add a convenient method for \
> it to kwineffects. But I didn't find a good name to express this - displaySize came \
> to my mind but is IMHO unfitting as one could think it's just one display and not \
> all outputs combined. So I thought given that ClientAreaOptions are documented it \
> would be fine. Just checked to verify and noticed it's not documented, just \
> commented, will fix that.
> If you have a good name suggestion I'm open for it :-)
>
> Thomas Lübking wrote:
> QRect ::viewport(), ::canvas(), ::sceneArea()
>
> > > clientArea() is a weird name anyway and the "Area" suffix to all enums is a bit \
> > > clumsy.
> > > area(Movement) ::area(Work[space]) etc. seems nicer, but is oc a major \
> > > refactoring (also impacting scripts)
>
> Martin Gräßlin wrote:
> Some suggestsions:
> > > combinedScreensGeometry()
> > > combinedOutputsGeometry()
> > > boundingScreensGeometry()
> > > boundingOutputsGeometry()
> > > boundingGeometry()
>
> I don't like ::viewport() as it reminds me too stronly of gl. The other two are too \
> close to rendering, which could be OK in some effects, but if it's about \
> positioning I think canvas or scene are not fitting.
> Thomas Lübking wrote:
> Reminded me of QAbstractScrollArea ;-P
>
> Actually, it is ::combined[Screens|Outputs]BoundingGeometry() - but that's a bit \
> clumsy (regardless of the Screens/Outputs issue)
> > > displaySize() might be a reasonable shortcut, but imo \
> > > ::geometry(RootWindow|Canvas) (different scope) in Workspace and EffectsHandler \
> > > should be nice (short, "geometry" says it's a geometry and the subject says \
> > > geometry of what - not misleading)
>
> This however implies a major refactoring and should probably not happen as long as \
> 4.11 is still being merged into master. So my suggestion was to keep the \
> less-than-ideal ::clientArea for now and change API and introduce a new/extended \
> set of enums later.
In Qt this is called virtualGeometry() / virtualSize().
- Fredrik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116021/#review50729
-----------------------------------------------------------
On Feb. 24, 2014, 3:27 p.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116021/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2014, 3:27 p.m.)
>
>
> Review request for kwin.
>
>
> Repository: kde-workspace
>
>
> Description
> -------
>
> Migrate all effects from displayWidth()/displayHeight() to clientArea(FullArea)
>
> Rational behind this change is that displayWidth and displayHeight are
> X specific API calls in kwinglobals. For the future it's easier to only
> rely on functionality which goes through the EffectsHandler API which
> allows easier adjustments in KWin core.
>
> displayWidth() and displayHeight() are only used to get the size or the
> complete rect of all screens. This is also provided by:
>
> effects->clientArea(FullArea)
>
> In fact the implementation in KWin core still uses displayWidth and
> displayHeight to return that value. In order to simplify the usage the
> EffectsHandler::clientArea taking desktop and screen recieved default
> argument values for screen and desktop allowing to just specify the
> AreaOption which ignores screen and desktop for FullArea.
>
>
> Diffs
> -----
>
> kwin/libkwineffects/kwineffects.cpp c4586c0ce3bac3e3b49467c1dd14a8a30f149f25
> kwin/scripting/scriptedeffect.cpp 7f3f6c54cef5f61a3f3f756bfe3dd7198aab4094
> kwin/effects/screenshot/screenshot.cpp 5b30fd5e0fe600a1ae3ff0ac6dd1ad11af0e4514
> kwin/effects/showfps/showfps.cpp cea7d540f4310b6dee4b46655adccddccab237c6
> kwin/effects/slide/slide.cpp b31c833f47f2bb91ba8d1d321387eb19fbba854a
> kwin/effects/zoom/zoom.cpp 96d6a04a57c99bfd392f4f681e9a2ea9af15712b
> kwin/libkwineffects/kwineffects.h df3bd3fdc7799eb224f8454a82b4136a0522ac42
> kwin/effects/cube/cubeslide.cpp c86d2572afb2372a0400fa12a5b2ecc2564621eb
> kwin/effects/logout/logout.cpp 998355ff3f748163979d4f2fc19f2f20f318d01c
> kwin/effects/lookingglass/lookingglass.cpp f7387547ee40c85bd2aa0ca4d61aee386e3e5c46 \
> kwin/effects/minimizeanimation/minimizeanimation.cpp \
> 4546cf88cc4051446844ea3bb791dfbc04bac94f \
> kwin/effects/backgroundcontrast/contrastshader.cpp \
> 890842d73dba09e2be779ddafe15b5921decd175 kwin/effects/blur/blur.cpp \
> 7983002eb4f3736695090c4550394e4b352b011e kwin/effects/blur/blurshader.cpp \
> 2172a05717316eb97d98850c6df4d47eca25ca29 kwin/effects/coverswitch/coverswitch.cpp \
> a174069d78f761ad5e8d04c6851510c5fa3631a0 kwin/effects/cube/cube.cpp \
> 7b80dde5a39aa7bdf5a4d592a32187fca84073c9 \
> kwin/effects/backgroundcontrast/contrast.cpp \
> 8de5c5d076e2a5d8e71207c52afedc7a8be9aff6
> Diff: https://git.reviewboard.kde.org/r/116021/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Martin Gräßlin
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/116021/">https://git.reviewboard.kde.org/r/116021/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On February 24th, 2014, 6:11 p.m. UTC, <b>Fredrik \
Höglund</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">clientArea(FullArea) is a really bad name for a function that returns \
the screen size. The name practically screams \
"subset-of-the-screen-geometry". </pre>
</blockquote>
<p>On February 24th, 2014, 6:55 p.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I also thought about it \
and whether it makes sense to add a convenient method for it to kwineffects. But I \
didn't find a good name to express this - displaySize came to my mind but is IMHO \
unfitting as one could think it's just one display and not all outputs combined. \
So I thought given that ClientAreaOptions are documented it would be fine. Just \
checked to verify and noticed it's not documented, just commented, will fix that.
If you have a good name suggestion I'm open for it :-)</pre>
</blockquote>
<p>On February 24th, 2014, 7:01 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">QRect ::viewport(), \
::canvas(), ::sceneArea()
> > clientArea() is a weird name anyway and the "Area" suffix to all enums \
> > is a bit clumsy.
> > area(Movement) ::area(Work[space]) etc. seems nicer, but is oc a major \
> > refactoring (also impacting scripts)</pre>
</blockquote>
<p>On February 25th, 2014, 7:39 a.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Some suggestsions: \
::combinedScreensGeometry() ::combinedOutputsGeometry()
> > boundingScreensGeometry()
> > boundingOutputsGeometry()
> > boundingGeometry()
I don't like ::viewport() as it reminds me too stronly of gl. The other two are \
too close to rendering, which could be OK in some effects, but if it's about \
positioning I think canvas or scene are not fitting.</pre> </blockquote>
<p>On February 25th, 2014, 2:41 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Reminded me of \
QAbstractScrollArea ;-P
Actually, it is ::combined[Screens|Outputs]BoundingGeometry() - but that's a bit \
clumsy (regardless of the Screens/Outputs issue)
> > displaySize() might be a reasonable shortcut, but imo \
> > ::geometry(RootWindow|Canvas) (different scope) in Workspace and EffectsHandler \
> > should be nice (short, "geometry" says it's a geometry and the \
> > subject says geometry of what - not misleading)
This however implies a major refactoring and should probably not happen as long as \
4.11 is still being merged into master. So my suggestion was to keep the \
less-than-ideal ::clientArea for now and change API and introduce a new/extended set \
of enums later.</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In Qt this is called \
virtualGeometry() / virtualSize().</pre> <br />
<p>- Fredrik</p>
<br />
<p>On February 24th, 2014, 3:27 p.m. UTC, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kwin.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Feb. 24, 2014, 3:27 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Migrate all effects from displayWidth()/displayHeight() to \
clientArea(FullArea)
Rational behind this change is that displayWidth and displayHeight are
X specific API calls in kwinglobals. For the future it's easier to only
rely on functionality which goes through the EffectsHandler API which
allows easier adjustments in KWin core.
displayWidth() and displayHeight() are only used to get the size or the
complete rect of all screens. This is also provided by:
effects->clientArea(FullArea)
In fact the implementation in KWin core still uses displayWidth and
displayHeight to return that value. In order to simplify the usage the
EffectsHandler::clientArea taking desktop and screen recieved default
argument values for screen and desktop allowing to just specify the
AreaOption which ignores screen and desktop for FullArea.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kwin/libkwineffects/kwineffects.cpp <span style="color: \
grey">(c4586c0ce3bac3e3b49467c1dd14a8a30f149f25)</span></li>
<li>kwin/scripting/scriptedeffect.cpp <span style="color: \
grey">(7f3f6c54cef5f61a3f3f756bfe3dd7198aab4094)</span></li>
<li>kwin/effects/screenshot/screenshot.cpp <span style="color: \
grey">(5b30fd5e0fe600a1ae3ff0ac6dd1ad11af0e4514)</span></li>
<li>kwin/effects/showfps/showfps.cpp <span style="color: \
grey">(cea7d540f4310b6dee4b46655adccddccab237c6)</span></li>
<li>kwin/effects/slide/slide.cpp <span style="color: \
grey">(b31c833f47f2bb91ba8d1d321387eb19fbba854a)</span></li>
<li>kwin/effects/zoom/zoom.cpp <span style="color: \
grey">(96d6a04a57c99bfd392f4f681e9a2ea9af15712b)</span></li>
<li>kwin/libkwineffects/kwineffects.h <span style="color: \
grey">(df3bd3fdc7799eb224f8454a82b4136a0522ac42)</span></li>
<li>kwin/effects/cube/cubeslide.cpp <span style="color: \
grey">(c86d2572afb2372a0400fa12a5b2ecc2564621eb)</span></li>
<li>kwin/effects/logout/logout.cpp <span style="color: \
grey">(998355ff3f748163979d4f2fc19f2f20f318d01c)</span></li>
<li>kwin/effects/lookingglass/lookingglass.cpp <span style="color: \
grey">(f7387547ee40c85bd2aa0ca4d61aee386e3e5c46)</span></li>
<li>kwin/effects/minimizeanimation/minimizeanimation.cpp <span style="color: \
grey">(4546cf88cc4051446844ea3bb791dfbc04bac94f)</span></li>
<li>kwin/effects/backgroundcontrast/contrastshader.cpp <span style="color: \
grey">(890842d73dba09e2be779ddafe15b5921decd175)</span></li>
<li>kwin/effects/blur/blur.cpp <span style="color: \
grey">(7983002eb4f3736695090c4550394e4b352b011e)</span></li>
<li>kwin/effects/blur/blurshader.cpp <span style="color: \
grey">(2172a05717316eb97d98850c6df4d47eca25ca29)</span></li>
<li>kwin/effects/coverswitch/coverswitch.cpp <span style="color: \
grey">(a174069d78f761ad5e8d04c6851510c5fa3631a0)</span></li>
<li>kwin/effects/cube/cube.cpp <span style="color: \
grey">(7b80dde5a39aa7bdf5a4d592a32187fca84073c9)</span></li>
<li>kwin/effects/backgroundcontrast/contrast.cpp <span style="color: \
grey">(8de5c5d076e2a5d8e71207c52afedc7a8be9aff6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116021/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic