[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request 108438: Use translucent/dialogs/background elements where possible
From: Martin_Gräßlin <mgraesslin () kde ! org>
Date: 2013-01-17 6:31:25
Message-ID: 20130117063125.1671.38873 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Jan. 16, 2013, 6:58 p.m., Sebastian Kügler wrote:
> > This patch brings a big improvement on the contrast, works better even than I \
> > expected. :)
> > On KDE/4.10 I'm getting the following build error:
> >
> > /home/sebas/kdesvn/src/kde-workspace/kwin/tabbox/../workspace.h: In function \
> > ‘bool KWin::TabBox::compositing()':
> > /home/sebas/kdesvn/src/kde-workspace/kwin/tabbox/../workspace.h:751:10: error: \
> > ‘bool KWin::Workspace::compositing() const' is private
> >
> > With kde-workspace master, it works fine.
> >
> > Martin: What regressions do you get exactly?
>
> Martin Gräßlin wrote:
> no idea about the build error - it doesn't make sense IMHO and I haven't seen it \
> (worked on 4.10 all the time)
> the regression I had seen was that the TabBox became too small - thumbnails were \
> placed outside the box.
> I will look into it tomorrow again - maybe Aaron found a solution for KWin tells \
> libplasma that we use compositing ;-)
> Thomas Lübking wrote:
> Does https://git.reviewboard.kde.org/r/107983/ not work for you?
>
> plasma could use the selectionwatcher, but must instantiate it after the eventloop \
> is up
> Sebastian Kügler wrote:
> Thomas, do you mean for me, or for Martin?
>
> Martin, the bool compositing() declaration ended up in the private section of \
> workspace.h, I've moved it to public and then it builds and works. Not sure what \
> happened, but it's sorted now.
> The use of margins looks fine to me, although it would probably be nice to add a \
> few pixels of extra padding between the tabbox's main frame top and the contents.
> I don't see the thumbnails outside of tabbox problem.
>
> Thomas Lübking wrote:
> Mostly Martin (because of his last comment) but hey, more intel data is always \
> better =)
> compositing() was moved to public to hand it over to the windows (we had to get rid \
> of an assert) - i actually thought that commit should be in 4.10 as well (otherwise \
> we've a Q_ASSERT(false) there now) check for \
> 048fe5397dbdabb791c4082f3a56ef23942d3bac (it's here)
> I'd say margins is font size and theme dependent
the tabbox used the border as a margin and that's currently gone and I think that \
causes the problem (seems also to be dependent on screen size)
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108438/#review25679
-----------------------------------------------------------
On Jan. 16, 2013, 2:57 p.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108438/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2013, 2:57 p.m.)
>
>
> Review request for kwin, Plasma, Sebastian Kügler, and Xuetian Weng.
>
>
> Description
> -------
>
> Use translucent/dialogs/background elements where possible
>
> In effects it's obvious that compositing is enabled, so specifying the
> translucent element is no problem.
>
> In tabbox a context property "compositing" is injected which decides
> whether "translucent" or "opaque" elements should be used.
>
> Also the masking is adjusted to ensure that only the shadow is not
> blurred.
>
> Reason for this change is that Plasma theme seems not always to pick up
> whether compositing is used when used from inside KWin. It does not cover
> the Desktop Change OSD which uses PlasmaCore.Dialog and there we cannot
> (yet) inject that we use compositing.
>
> CCBUG: 311995
>
>
> Diffs
> -----
>
> kwin/effects/desktopgrid/desktopgrid.cpp 53532ccff72d2a1f303265a898fd7b1fc7ea35a6
> kwin/effects/presentwindows/presentwindows.cpp \
> 100d5ec0ab80b50d8b3d0b68d11bc08f55da15ca kwin/tabbox/declarative.cpp \
> b4d615178a0e0b8b633626a1813dfa7065515b97 kwin/tabbox/qml/CMakeLists.txt \
> bda7d8b95ed3b4fb4093c9960f940a0d678710f8 kwin/tabbox/qml/ShadowedSvgItem.qml \
> 87de733bf0fa1ad19d4d9203de6c6fc3548ff57f \
> kwin/tabbox/qml/clients/big_icons/contents/ui/main.qml \
> 4a3666d2befe0010f7d9b7d4414853e8c92a92a1 \
> kwin/tabbox/qml/clients/compact/contents/ui/main.qml \
> 14fca297c40a8338de1fff413f7e4b5b0a4aef45 \
> kwin/tabbox/qml/clients/informative/contents/ui/main.qml \
> 5e3f9a6f2ad76e7eeb7ee1e51d42e0c891b762ec \
> kwin/tabbox/qml/clients/present_windows/contents/ui/main.qml \
> e539c8970acff701b92bdaaf816a40ccba94e4b8 \
> kwin/tabbox/qml/clients/small_icons/contents/ui/main.qml \
> 0c9b4f7f7df52f8a629d0c847185db38aa9f2c26 \
> kwin/tabbox/qml/clients/text/contents/ui/main.qml \
> ccae17d96b231269a7d8ec0ecc20b0ee69453d09 \
> kwin/tabbox/qml/clients/thumbnails/contents/ui/main.qml \
> 4c33703d54c84fd54da94f821234e4cbd9c1c001 kwin/tabbox/qml/desktop.qml \
> e8203b6004a321fcc5ae6d10347fdbb7144662a7
> Diff: http://git.reviewboard.kde.org/r/108438/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="http://git.reviewboard.kde.org/r/108438/">http://git.reviewboard.kde.org/r/108438/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On January 16th, 2013, 6:58 p.m. CET, <b>Sebastian \
Kügler</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;">This patch brings a big improvement on the contrast, works better even \
than I expected. :)
On KDE/4.10 I'm getting the following build error:
/home/sebas/kdesvn/src/kde-workspace/kwin/tabbox/../workspace.h: In function ‘bool \
KWin::TabBox::compositing()':
/home/sebas/kdesvn/src/kde-workspace/kwin/tabbox/../workspace.h:751:10: error: \
‘bool KWin::Workspace::compositing() const' is private
With kde-workspace master, it works fine.
Martin: What regressions do you get exactly?</pre>
</blockquote>
<p>On January 16th, 2013, 9:22 p.m. CET, <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;">no idea about the build \
error - it doesn't make sense IMHO and I haven't seen it (worked on 4.10 all \
the time)
the regression I had seen was that the TabBox became too small - thumbnails were \
placed outside the box.
I will look into it tomorrow again - maybe Aaron found a solution for KWin tells \
libplasma that we use compositing ;-)</pre> </blockquote>
<p>On January 16th, 2013, 9:28 p.m. CET, <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;">Does \
https://git.reviewboard.kde.org/r/107983/ not work for you?
plasma could use the selectionwatcher, but must instantiate it after the eventloop is \
up</pre> </blockquote>
<p>On January 17th, 2013, 12:41 a.m. CET, <b>Sebastian Kügler</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;">Thomas, do you mean for \
me, or for Martin?
Martin, the bool compositing() declaration ended up in the private section of \
workspace.h, I've moved it to public and then it builds and works. Not sure what \
happened, but it's sorted now.
The use of margins looks fine to me, although it would probably be nice to add a few \
pixels of extra padding between the tabbox's main frame top and the contents.
I don't see the thumbnails outside of tabbox problem.</pre>
</blockquote>
<p>On January 17th, 2013, 1:17 a.m. CET, <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;">Mostly Martin (because \
of his last comment) but hey, more intel data is always better =)
compositing() was moved to public to hand it over to the windows (we had to get rid \
of an assert) - i actually thought that commit should be in 4.10 as well (otherwise \
we've a Q_ASSERT(false) there now) check for \
048fe5397dbdabb791c4082f3a56ef23942d3bac (it's here)
I'd say margins is font size and theme dependent</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;">the tabbox used the \
border as a margin and that's currently gone and I think that causes the problem \
(seems also to be dependent on screen size)</pre> <br />
<p>- Martin</p>
<br />
<p>On January 16th, 2013, 2:57 p.m. CET, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://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, Plasma, Sebastian Kügler, and Xuetian Weng.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Jan. 16, 2013, 2:57 p.m.</i></p>
<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;">Use translucent/dialogs/background elements where possible
In effects it's obvious that compositing is enabled, so specifying the
translucent element is no problem.
In tabbox a context property "compositing" is injected which decides
whether "translucent" or "opaque" elements should be used.
Also the masking is adjusted to ensure that only the shadow is not
blurred.
Reason for this change is that Plasma theme seems not always to pick up
whether compositing is used when used from inside KWin. It does not cover
the Desktop Change OSD which uses PlasmaCore.Dialog and there we cannot
(yet) inject that we use compositing.
CCBUG: 311995</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/effects/desktopgrid/desktopgrid.cpp <span style="color: \
grey">(53532ccff72d2a1f303265a898fd7b1fc7ea35a6)</span></li>
<li>kwin/effects/presentwindows/presentwindows.cpp <span style="color: \
grey">(100d5ec0ab80b50d8b3d0b68d11bc08f55da15ca)</span></li>
<li>kwin/tabbox/declarative.cpp <span style="color: \
grey">(b4d615178a0e0b8b633626a1813dfa7065515b97)</span></li>
<li>kwin/tabbox/qml/CMakeLists.txt <span style="color: \
grey">(bda7d8b95ed3b4fb4093c9960f940a0d678710f8)</span></li>
<li>kwin/tabbox/qml/ShadowedSvgItem.qml <span style="color: \
grey">(87de733bf0fa1ad19d4d9203de6c6fc3548ff57f)</span></li>
<li>kwin/tabbox/qml/clients/big_icons/contents/ui/main.qml <span style="color: \
grey">(4a3666d2befe0010f7d9b7d4414853e8c92a92a1)</span></li>
<li>kwin/tabbox/qml/clients/compact/contents/ui/main.qml <span style="color: \
grey">(14fca297c40a8338de1fff413f7e4b5b0a4aef45)</span></li>
<li>kwin/tabbox/qml/clients/informative/contents/ui/main.qml <span style="color: \
grey">(5e3f9a6f2ad76e7eeb7ee1e51d42e0c891b762ec)</span></li>
<li>kwin/tabbox/qml/clients/present_windows/contents/ui/main.qml <span style="color: \
grey">(e539c8970acff701b92bdaaf816a40ccba94e4b8)</span></li>
<li>kwin/tabbox/qml/clients/small_icons/contents/ui/main.qml <span style="color: \
grey">(0c9b4f7f7df52f8a629d0c847185db38aa9f2c26)</span></li>
<li>kwin/tabbox/qml/clients/text/contents/ui/main.qml <span style="color: \
grey">(ccae17d96b231269a7d8ec0ecc20b0ee69453d09)</span></li>
<li>kwin/tabbox/qml/clients/thumbnails/contents/ui/main.qml <span style="color: \
grey">(4c33703d54c84fd54da94f821234e4cbd9c1c001)</span></li>
<li>kwin/tabbox/qml/desktop.qml <span style="color: \
grey">(e8203b6004a321fcc5ae6d10347fdbb7144662a7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108438/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