[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