Re: Review Request: Implements Plasma::TabBar::setTabHighlighted

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101264/#review3103
---


This review has been submitted with commit 
0d9efa0f939ece18849e82cd2b58cf4ce441b0c2 by Matthias Fuchs.

- Commit


On May 2, 2011, 10:25 a.m., Matthias Fuchs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101264/
> ---
> 
> (Updated May 2, 2011, 10:25 a.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Marco Martin.
> 
> 
> Summary
> ---
> 
> Adds a setTabHighlighted and a isTabHighligted method to Plasma::TabBar.
> When a tab is highlighted its icon will be drawn with 50% opacity and the 
> text will use Theme::HighlightColor.
> 
> So far I am not overly happy with the result.
> Marco suggested to blur the area behind the icon, though I am not sure how to 
> do that. Another option I thought off is to either draw the button highlight 
> effect at the area of highlighted tabs or around its icon.
> What do you think of that?
> 
> 
> Diffs
> -
> 
>   plasma/private/nativetabbar.cpp 916deed 
>   plasma/private/nativetabbar_p.h 665d99f 
>   plasma/widgets/tabbar.h eb7a2aa 
>   plasma/widgets/tabbar.cpp 16da3b9 
> 
> Diff: http://git.reviewboard.kde.org/r/101264/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> could be nicer :/
>   http://git.reviewboard.kde.org/r/101264/s/152/
> 
>   http://git.reviewboard.kde.org/r/101264/s/153/
> 
> 
> Thanks,
> 
> Matthias
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Implements Plasma::TabBar::setTabHighlighted

[Aaron J. Seigo]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101264/#review3102
---

Ship it!


looks fine; a few minor tweaks but nothing that stands in the way of committing 
this. cheers :)


plasma/private/nativetabbar.cpp


it's a private class; private membesr are unecessary. :)



plasma/private/nativetabbar.cpp


watch the whitespace => count()) {


- Aaron J.


On May 2, 2011, 10:25 a.m., Matthias Fuchs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101264/
> ---
> 
> (Updated May 2, 2011, 10:25 a.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Marco Martin.
> 
> 
> Summary
> ---
> 
> Adds a setTabHighlighted and a isTabHighligted method to Plasma::TabBar.
> When a tab is highlighted its icon will be drawn with 50% opacity and the 
> text will use Theme::HighlightColor.
> 
> So far I am not overly happy with the result.
> Marco suggested to blur the area behind the icon, though I am not sure how to 
> do that. Another option I thought off is to either draw the button highlight 
> effect at the area of highlighted tabs or around its icon.
> What do you think of that?
> 
> 
> Diffs
> -
> 
>   plasma/private/nativetabbar.cpp 916deed 
>   plasma/private/nativetabbar_p.h 665d99f 
>   plasma/widgets/tabbar.h eb7a2aa 
>   plasma/widgets/tabbar.cpp 16da3b9 
> 
> Diff: http://git.reviewboard.kde.org/r/101264/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> could be nicer :/
>   http://git.reviewboard.kde.org/r/101264/s/152/
> 
>   http://git.reviewboard.kde.org/r/101264/s/153/
> 
> 
> Thanks,
> 
> Matthias
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Implements Plasma::TabBar::setTabHighlighted

[Matthias Fuchs]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101264/
---

(Updated May 2, 2011, 10:25 a.m.)


Review request for Plasma, Aaron J. Seigo and Marco Martin.


Changes
---

Draws the PushButton activated effect behind icons, to highlight them.


Summary
---

Adds a setTabHighlighted and a isTabHighligted method to Plasma::TabBar.
When a tab is highlighted its icon will be drawn with 50% opacity and the text 
will use Theme::HighlightColor.

So far I am not overly happy with the result.
Marco suggested to blur the area behind the icon, though I am not sure how to 
do that. Another option I thought off is to either draw the button highlight 
effect at the area of highlighted tabs or around its icon.
What do you think of that?


Diffs (updated)
-

  plasma/private/nativetabbar.cpp 916deed 
  plasma/private/nativetabbar_p.h 665d99f 
  plasma/widgets/tabbar.h eb7a2aa 
  plasma/widgets/tabbar.cpp 16da3b9 

Diff: http://git.reviewboard.kde.org/r/101264/diff


Testing
---


Screenshots
---

could be nicer :/
  http://git.reviewboard.kde.org/r/101264/s/152/

  http://git.reviewboard.kde.org/r/101264/s/153/


Thanks,

Matthias

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Implements Plasma::TabBar::setTabHighlighted

[Matthias Fuchs]

> On May 1, 2011, 2:11 p.m., Marco Martin wrote:
> > plasma/private/nativetabbar.cpp, line 87
> > 
> >
> > this will have an o(n) access time, isTabHighlighted will be quite 
> > inefficient.
> > should be a qvector with repacking when at item gets removed in the 
> > middle

As I mentioned on the irc channel QList is also O(1) according to the 
documentation.
So I think I should keep using QList there, as inserting/removing is a lot 
faster than for QVector.


- Matthias


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101264/#review3012
---


On May 1, 2011, 1:31 p.m., Matthias Fuchs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101264/
> ---
> 
> (Updated May 1, 2011, 1:31 p.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Marco Martin.
> 
> 
> Summary
> ---
> 
> Adds a setTabHighlighted and a isTabHighligted method to Plasma::TabBar.
> When a tab is highlighted its icon will be drawn with 50% opacity and the 
> text will use Theme::HighlightColor.
> 
> So far I am not overly happy with the result.
> Marco suggested to blur the area behind the icon, though I am not sure how to 
> do that. Another option I thought off is to either draw the button highlight 
> effect at the area of highlighted tabs or around its icon.
> What do you think of that?
> 
> 
> Diffs
> -
> 
>   plasma/private/nativetabbar.cpp 916deed 
>   plasma/private/nativetabbar_p.h 665d99f 
>   plasma/widgets/tabbar.h eb7a2aa 
>   plasma/widgets/tabbar.cpp 16da3b9 
> 
> Diff: http://git.reviewboard.kde.org/r/101264/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> could be nicer :/
>   http://git.reviewboard.kde.org/r/101264/s/152/
> 
> 
> Thanks,
> 
> Matthias
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Implements Plasma::TabBar::setTabHighlighted

[Marco Martin]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101264/#review3012
---


the look is ok enough for now, only the concern below in the implementation


plasma/private/nativetabbar.cpp


this will have an o(n) access time, isTabHighlighted will be quite 
inefficient.
should be a qvector with repacking when at item gets removed in the middle


- Marco


On May 1, 2011, 1:31 p.m., Matthias Fuchs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101264/
> ---
> 
> (Updated May 1, 2011, 1:31 p.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Marco Martin.
> 
> 
> Summary
> ---
> 
> Adds a setTabHighlighted and a isTabHighligted method to Plasma::TabBar.
> When a tab is highlighted its icon will be drawn with 50% opacity and the 
> text will use Theme::HighlightColor.
> 
> So far I am not overly happy with the result.
> Marco suggested to blur the area behind the icon, though I am not sure how to 
> do that. Another option I thought off is to either draw the button highlight 
> effect at the area of highlighted tabs or around its icon.
> What do you think of that?
> 
> 
> Diffs
> -
> 
>   plasma/private/nativetabbar.cpp 916deed 
>   plasma/private/nativetabbar_p.h 665d99f 
>   plasma/widgets/tabbar.h eb7a2aa 
>   plasma/widgets/tabbar.cpp 16da3b9 
> 
> Diff: http://git.reviewboard.kde.org/r/101264/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> could be nicer :/
>   http://git.reviewboard.kde.org/r/101264/s/152/
> 
> 
> Thanks,
> 
> Matthias
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel