D6544: Don't draw focus line on selected list item

[Kai Uwe Broulik]

broulik added a comment.


  Updated, thanks. 
https://cgit.kde.org/breeze.git/commit/?id=6906919371ff92ff0dc7f6ae4e4e14310c5e1e53

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: cfeck, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> cfeck wrote in breezestyle.cpp:3082
> Of course inherits(QString) is slower than qobject_cast<>.
> 
> The only reason to use inherits() is when you do not link to the library that 
> the class defines.

Also order to make the fastest test come first.

if ((state & State_Selected) && qobject_cast(widget))

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: cfeck, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> broulik wrote in breezestyle.cpp:3082
> From what I can tell the difference is negligible. `qobject_cast` goes 
> through `QMetaObject::cast` which then calls `inherits` internally, so I'd 
> say `inherits` is the better choice.
> 
> I didn't benchmark, though. I just followed what the rest of the code around 
> it was doing (`inherits`)

Of course inherits(QString) is slower than qobject_cast<>.

The only reason to use inherits() is when you do not link to the library that 
the class defines.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: cfeck, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:89b916039387: Don't draw focus line on selected list item 
(authored by broulik).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6544?vs=16287=16297

REVISION DETAIL
  https://phabricator.kde.org/D6544

AFFECTED FILES
  kstyle/breezestyle.cpp

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> broulik wrote in breezestyle.cpp:3082
> From what I can tell the difference is negligible. `qobject_cast` goes 
> through `QMetaObject::cast` which then calls `inherits` internally, so I'd 
> say `inherits` is the better choice.
> 
> I didn't benchmark, though. I just followed what the rest of the code around 
> it was doing (`inherits`)

Code uses both actually (usually qobject_cast for "public" classes, from Qt, 
and inherits for "private" classes, or kde).
In any case, I trust your investigation :)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> hpereiradacosta wrote in breezestyle.cpp:3082
> Not sure which one is the most efficient:
> Widget->inherits or qobject_cast ?
> (My guess  would need the second, but ...)
> Can you double check ?

>From what I can tell the difference is negligible. `qobject_cast` goes through 
>`QMetaObject::cast` which then calls `inherits` internally, so I'd say 
>`inherits` is the better choice.

I didn't benchmark, though. I just followed what the rest of the code around it 
was doing (`inherits`)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezestyle.cpp:3082
> +// no focus indicator on selected list items
> +if (widget && widget->inherits("QAbstractItemView") && (state & 
> State_Selected))
> +{ return true; }

Not sure which one is the most efficient:
Widget->inherits or qobject_cast ?
(My guess  would need the second, but ...)
Can you double check ?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Kai Uwe Broulik]

broulik edited the test plan for this revision.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, lukas

D6544: Don't draw focus line on selected list item

[Kai Uwe Broulik]

broulik created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  A fix I did for the focus line being drawn outside item boundaries resulted 
in the item selection rectangle being "cut off" by the focus line. The 
selection rectangle is enough of an indication anyway.

TEST PLAN
  Before
  F3805982: Screenshot_20170707_100856.png 

  After
  F3805984: Screenshot_20170707_110608.png 

  
  Especially noticeable in KInfoCenter where there's a tree list and only the 
label is underlined but not the icon, resulting in an uneven "cut".

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D6544

AFFECTED FILES
  kstyle/breezestyle.cpp

To: broulik, #plasma, hpereiradacosta, fvogt
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, lukas