[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: [Differential] [Commented On] D3131: Show full scrollbar only on mouse over
From: "hpereiradacosta (Hugo Pereira Da Costa)" <noreply () phabricator ! kde ! org>
Date: 2016-10-31 15:54:42
Message-ID: 20161031155441.17018.73319.E7FC6267 () phabricator ! kde ! org
[Download RAW message or body]
[Attachment #2 (text/plain)]
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3131#59715, @hpereiradacosta wrote:
> Hi Marco,
> Two problems with the commit:
> 1/ it does not compile under kde4 (and we are still maintaining a kde4 \
and a kf5 version) >
> breezestyle.cpp:6652:26: error: ‘const class QStyleOptionSlider' has \
no member named ‘styleObject' >
> else if( option->styleObject ) widgetMouseOver = \
option->styleObject->property("hover").toBool(); >
>
> I can easily fix that.
>
> 2/ there are warnings:
>
> /home/hpereira/kf5/src/kde/workspace/breeze/kstyle/breezestyle.cpp:6719:24: \
warning: unused variable ‘mouseOver' [-Wunused-variable] >
> const bool mouseOver( ( option->state & State_MouseOver ) );
> ^
>
> /home/hpereira/kf5/src/kde/workspace/breeze/kstyle/breezestyle.cpp:6724:18: \
warning: ‘widgetMouseOver' may be used uninitialized in this function \
[-Wmaybe-uninitialized] >
> else if( !widgetMouseOver ) return Qt::transparent;
>
>
> Here I think this is because the first mouseOver should be \
widgetMouseOver, but I am not sure. Can you double check ?
Looking more at the code, I think I am confused about the logic ...
How is widgetMouseOver different from mouseOver later on.
Why is one calling widget->mouseOver() and the second option->state & \
State_MouseOver ? (I would think the second is correct and should be used \
everywhere)
And finally, about the block on "disabled" state scrollbars, do we want \
the groove to show up on mouse-over in that case ? I think this tends to \
indicate that something will happen if you click, which is not the case ... \
Sorry if I have not commented on this before the commit, I guess I must \
have not reviewed this part of the patch carefully enough.
REPOSITORY
rBREEZE Breeze
REVISION DETAIL
https://phabricator.kde.org/D3131
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: mart, #plasma, #breeze, #vdg, hpereiradacosta
Cc: plasma-devel, abetts, lesliezhai, ali-mohamed, jensreuterberg, sebas
[Attachment #3 (text/html)]
<table><tr><td style="">hpereiradacosta added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; \
float: right; color: #464C5C; font-weight: bold; border-radius: 3px; \
background-color: #F7F7F9; background-image: linear-gradient(to \
bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid \
rgba(71,87,120,.2);" href="https://phabricator.kde.org/D3131" \
rel="noreferrer">View Revision</a></tr></table><br /><div><div><blockquote \
style="border-left: 3px solid #8C98B8; color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a \
href="https://phabricator.kde.org/D3131#59715" style="background-color: \
#e7e7e7; border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: line-through;" \
rel="noreferrer">D3131#59715</a>, <a \
href="https://phabricator.kde.org/p/hpereiradacosta/" style=" \
border-color: #f1f7ff; color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@hpereiradacosta</a> \
wrote:</div> <div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Hi Marco, <br />
Two problems with the commit: <br />
1/ it does not compile under kde4 (and we are still maintaining a kde4 and \
a kf5 version)</p>
<p>breezestyle.cpp:6652:26: error: ‘const class QStyleOptionSlider' has \
no member named ‘styleObject'</p>
<div class="remarkup-code-block" style="margin: 12px 0;" \
data-code-lang="text" data-sigil="remarkup-code-block"><pre \
class="remarkup-code" style="font: 11px/15px "Menlo", \
"Consolas", "Monaco", monospace; padding: 12px; margin: \
0; background: rgba(71, 87, 120, 0.08);">else if( option->styleObject ) \
widgetMouseOver = option->styleObject->property("hover").toBool();</pre></div>
<p>I can easily fix that.</p>
<p>2/ there are warnings:</p>
<p>/home/hpereira/kf5/src/kde/workspace/breeze/kstyle/breezestyle.cpp:6719:24: \
warning: unused variable ‘mouseOver' [-Wunused-variable]</p>
<div class="remarkup-code-block" style="margin: 12px 0;" \
data-code-lang="text" data-sigil="remarkup-code-block"><pre \
class="remarkup-code" style="font: 11px/15px "Menlo", \
"Consolas", "Monaco", monospace; padding: 12px; margin: \
0; background: rgba(71, 87, 120, 0.08);">const bool mouseOver( ( \
option->state & State_MouseOver ) ); ^</pre></div>
<p>/home/hpereira/kf5/src/kde/workspace/breeze/kstyle/breezestyle.cpp:6724:18: \
warning: ‘widgetMouseOver' may be used uninitialized in this function \
[-Wmaybe-uninitialized]</p>
<div class="remarkup-code-block" style="margin: 12px 0;" \
data-code-lang="text" data-sigil="remarkup-code-block"><pre \
class="remarkup-code" style="font: 11px/15px "Menlo", \
"Consolas", "Monaco", monospace; padding: 12px; margin: \
0; background: rgba(71, 87, 120, 0.08);">else if( !widgetMouseOver ) return \
Qt::transparent;</pre></div>
<p>Here I think this is because the first mouseOver should be \
widgetMouseOver, but I am not sure. Can you double check ?</p></div> \
</blockquote>
<p>Looking more at the code, I think I am confused about the logic ... <br \
/> How is widgetMouseOver different from mouseOver later on.<br />
Why is one calling widget->mouseOver() and the second option->state \
& State_MouseOver ? (I would think the second is correct and should be \
used everywhere)</p>
<p>And finally, about the block on "disabled" state scrollbars, \
do we want the groove to show up on mouse-over in that case ? I think this \
tends to indicate that something will happen if you click, which is not the \
case ...</p>
<p>Sorry if I have not commented on this before the commit, I guess I must \
have not reviewed this part of the patch carefully \
enough.</p></div></div><br \
/><div><strong>REPOSITORY</strong><div><div>rBREEZE \
Breeze</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a \
href="https://phabricator.kde.org/D3131" \
rel="noreferrer">https://phabricator.kde.org/D3131</a></div></div><br \
/><div><strong>EMAIL PREFERENCES</strong><div><a \
href="https://phabricator.kde.org/settings/panel/emailpreferences/" \
rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br \
/><div><strong>To: </strong>mart, Plasma, Breeze, VDG, hpereiradacosta<br \
/><strong>Cc: </strong>plasma-devel, abetts, lesliezhai, ali-mohamed, \
jensreuterberg, sebas<br /></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic