[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: Bug 82171 persist along with ShadeHover
From: Thomas_Lübking <thomas.luebking () web ! de>
Date: 2012-12-22 19:16:40
Message-ID: 20121222191640.15625.68069 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Dec. 22, 2012, 12:04 p.m., Martin Gräßlin wrote:
> > wouldn't it make more sense to stop the ShadeHover timer when the window gets \
> > minimized?
>
> Slavek Banko wrote:
> The window would then be left unshaded after minimizing / restore?
> Or would have to be addressed if shadeHover in which case do shade before \
> minimizing? Or deal with the timer after the restore window?
>
> Thomas Lübking wrote:
> Minimized windows must not be shaded (NETWM), thus the patch is generally ok by \
> fixing it at the root but it could (should) also be done before by
> void Client::setShade(ShadeMode mode)
> {
> if (mode == ShadeHover && isMove())
> return; // causes geometry breaks and is probably nasty
> - if (isSpecialWindow() || noBorder())
> + if (isSpecialWindow() || noBorder() || isMinimized())
> mode = ShadeNone;
> mode = rules()->checkShade(mode);
> ...
>
> to bypass useless action and side effects (if isMinimized() holds, the window \
> should NOT be shaded at that time, ie. that gets us a quick exit for sure and also \
> ensures we don't somehow keep the window shaded)
> Please test this and in doubt update the request (and thanks for that anyway)
>
> Martin Gräßlin wrote:
> > The window would then be left unshaded after minimizing / restore?
> I would say that this is exactly how it should be. The user minimized the window \
> when the window was not shaded, so the state should be preserved.
> Thomas Lübking wrote:
> If you stop the timer, the shadelevel remains at ShadeHover, what means "isShade()" \
> is false at unminimization and atm. will thus bypass
> if (isShade()) // NETWM restriction - KWindowInfo::isMinimized() == Hidden && \
> !Shaded info->setState(NET::Shaded, NET::Shaded);
>
> so the shade level would remain at ShadeHover (not ShadeNone)
>
> I'd however still suggest to catch Client::setShade() and simply forbid ShadeNormal \
> while the window is minimized (a bit more precise: "if (isMinimized() && mode == \
> ShadeNormal) return; // mode = shade_mode, bypassing rules as well)"
> Martin Gräßlin wrote:
> in that case libtaskmanager needs to be adjusted as well to disable the Shade \
> option when a window is minimized.
> What about the other way around? Should we also block minimize for a shaded window? \
> Or go to not-shaded state when minimizing? (Pro: would give us window previews, \
> user sees the window on restore, Contra: it breaks state)
> Slavek Banko wrote:
> With ShadeHover: When the cursor leaves the window area, this is a reason for \
> shade. When the user minimizes the window, it certainly leaves the window area => \
> this is a reason to shade. Moreover, it is very likely that after the restore \
> window, the cursor will be outside the window area => again a reason for shade. \
> Does not seem to me right that after the restore window was unshaded. Regardless of \
> that, before minimization was unshaded.
> Disable minimize for windows that are shaded does not strike me as a good reason. \
> Indeed, bug 82171 was reported just because someone minimize shaded windows.
> Note: Suggestions from Thomas I'll try later.
What probably would work would be to enforce ShadeActivated (on attempts to set \
ShadeNormal) on minimized windows.
Pros:
- window/content is visible in PresentWindows and DesktopGrid
- the shading state is kept across minimizations (ShadeActivated is NOT ShadeNone, \
the window is internally still shaded, though visible)
- the shading state is changeable across minimizations (no need to restrict \
libtaskbar, the user can alter the state anytime. Afaics the event -> setShade path \
will do that automagically with such restriction in place)
- the window is not tagged NET::Shaded in this state
- the state can implicitly fix with unminimization (if the unminimized window is \
active, the state remains, otherwise we fall back to ShadeNormal)
- not limited to particular timer driven implementation details (in hover)
Cons:
- none at hand ;-)
So yes, setting the state to ShadeActivated on minimization and fixing that on \
unminimization alongside the mode enforcement (ShadeNormal -> ShadeActivated) in \
::setShade() should pretty much do.
@Slavek - please try that first.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107845/#review23837
-----------------------------------------------------------
On Dec. 22, 2012, 11:51 a.m., Slavek Banko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107845/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2012, 11:51 a.m.)
>
>
> Review request for kwin.
>
>
> Description
> -------
>
> I tried the patch from git commit 8a02a383 - for bug 81271, and I discovered that \
> it is incomplete => partly still does not solve the problem reported in bug 81271. \
> Problem persists along with ShadeHover.
> 1. Allow shade hover
> 2. Move focus to the shaded window => is unshaded
> 3. Minimize this window
> 4. After the expiry shadeHoverTimer is called shadeHover => setShade(ShadeHover) => \
> info->setstate() with NET::Shaded. 5. Bug 81271 is again apparent.
>
>
> This addresses bug 81271.
> http://bugs.kde.org/show_bug.cgi?id=81271
>
>
> Diffs
> -----
>
> kwin/client.cpp b555c11
>
> Diff: http://git.reviewboard.kde.org/r/107845/diff/
>
>
> Testing
> -------
>
> Patches I successfully tested in TDE.
>
>
> Thanks,
>
> Slavek Banko
>
>
[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/107845/">http://git.reviewboard.kde.org/r/107845/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On December 22nd, 2012, 12:04 p.m., <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;">wouldn't it make more sense to stop the ShadeHover timer when the \
window gets minimized?</pre> </blockquote>
<p>On December 22nd, 2012, 12:25 p.m., <b>Slavek Banko</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;">The window would then be \
left unshaded after minimizing / restore? Or would have to be addressed if shadeHover \
in which case do shade before minimizing? Or deal with the timer after the restore \
window?</pre> </blockquote>
<p>On December 22nd, 2012, 1:52 p.m., <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;">Minimized windows must \
not be shaded (NETWM), thus the patch is generally ok by fixing it at the root but it \
could (should) also be done before by
void Client::setShade(ShadeMode mode)
{
if (mode == ShadeHover && isMove())
return; // causes geometry breaks and is probably nasty
- if (isSpecialWindow() || noBorder())
+ if (isSpecialWindow() || noBorder() || isMinimized())
mode = ShadeNone;
mode = rules()->checkShade(mode);
...
to bypass useless action and side effects (if isMinimized() holds, the window should \
NOT be shaded at that time, ie. that gets us a quick exit for sure and also ensures \
we don't somehow keep the window shaded)
Please test this and in doubt update the request (and thanks for that anyway)</pre>
</blockquote>
<p>On December 22nd, 2012, 4:24 p.m., <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;">> The window would \
then be left unshaded after minimizing / restore? I would say that this is exactly \
how it should be. The user minimized the window when the window was not shaded, so \
the state should be preserved.</pre> </blockquote>
<p>On December 22nd, 2012, 4:47 p.m., <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;">If you stop the timer, \
the shadelevel remains at ShadeHover, what means "isShade()" is false at \
unminimization and atm. will thus bypass
if (isShade()) // NETWM restriction - KWindowInfo::isMinimized() == Hidden && \
!Shaded info->setState(NET::Shaded, NET::Shaded);
so the shade level would remain at ShadeHover (not ShadeNone)
I'd however still suggest to catch Client::setShade() and simply forbid \
ShadeNormal while the window is minimized (a bit more precise: "if \
(isMinimized() && mode == ShadeNormal) return; // mode = shade_mode, \
bypassing rules as well)"</pre> </blockquote>
<p>On December 22nd, 2012, 5:28 p.m., <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;">in that case \
libtaskmanager needs to be adjusted as well to disable the Shade option when a window \
is minimized.
What about the other way around? Should we also block minimize for a shaded window? \
Or go to not-shaded state when minimizing? (Pro: would give us window previews, user \
sees the window on restore, Contra: it breaks state)</pre> </blockquote>
<p>On December 22nd, 2012, 6:59 p.m., <b>Slavek Banko</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;">With ShadeHover: When \
the cursor leaves the window area, this is a reason for shade. When the user \
minimizes the window, it certainly leaves the window area => this is a reason to \
shade. Moreover, it is very likely that after the restore window, the cursor will be \
outside the window area => again a reason for shade. Does not seem to me right \
that after the restore window was unshaded. Regardless of that, before minimization \
was unshaded.
Disable minimize for windows that are shaded does not strike me as a good reason. \
Indeed, bug 82171 was reported just because someone minimize shaded windows.
Note: Suggestions from Thomas I'll try 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;">What probably would work \
would be to enforce ShadeActivated (on attempts to set ShadeNormal) on minimized \
windows.
Pros:
- window/content is visible in PresentWindows and DesktopGrid
- the shading state is kept across minimizations (ShadeActivated is NOT ShadeNone, \
the window is internally still shaded, though visible)
- the shading state is changeable across minimizations (no need to restrict \
libtaskbar, the user can alter the state anytime. Afaics the event -> setShade \
path will do that automagically with such restriction in place)
- the window is not tagged NET::Shaded in this state
- the state can implicitly fix with unminimization (if the unminimized window is \
active, the state remains, otherwise we fall back to ShadeNormal)
- not limited to particular timer driven implementation details (in hover)
Cons:
- none at hand ;-)
So yes, setting the state to ShadeActivated on minimization and fixing that on \
unminimization alongside the mode enforcement (ShadeNormal -> ShadeActivated) in \
::setShade() should pretty much do.
@Slavek - please try that first.</pre>
<br />
<p>- Thomas</p>
<br />
<p>On December 22nd, 2012, 11:51 a.m., Slavek Banko wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kwin.</div>
<div>By Slavek Banko.</div>
<p style="color: grey;"><i>Updated Dec. 22, 2012, 11:51 a.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;">I tried the patch from git commit 8a02a383 - for bug 81271, and I \
discovered that it is incomplete => partly still does not solve the problem \
reported in bug 81271. Problem persists along with ShadeHover.
1. Allow shade hover
2. Move focus to the shaded window => is unshaded
3. Minimize this window
4. After the expiry shadeHoverTimer is called shadeHover => setShade(ShadeHover) \
=> info->setstate() with NET::Shaded. 5. Bug 81271 is again apparent.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Patches I successfully tested in TDE.</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=81271">81271</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kwin/client.cpp <span style="color: grey">(b555c11)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107845/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