Re: Review Request 127875: Selected state concept for icons

2016-05-16 Thread Marco Martin 


> On May 15, 2016, 7:26 p.m., David Faure wrote:
> > src/kiconloader.cpp, line 820
> > 
> >
> > Could q->theme() be NULL here in some cases?
> > See 
> > https://build.kde.org/job/khtml%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/32/testReport/(root)/TestSuite/khtml_parttest/

added a check on the pointer


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95492
---


On May 13, 2016, 9:46 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 13, 2016, 9:46 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-15 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95492
---




src/kiconloader.cpp (line 820)


Could q->theme() be NULL here in some cases?
See 
https://build.kde.org/job/khtml%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/32/testReport/(root)/TestSuite/khtml_parttest/


- David Faure


On May 13, 2016, 9:46 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 13, 2016, 9:46 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-13 Thread Marco Martin 

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

(Updated May 13, 2016, 9:46 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Changes
---

Submitted with commit fde854c5bed063424e1870e45c2348e09eaca4e6 by Marco Martin 
to branch master.


Repository: kiconthemes


Description
---

QIcon has a Selected state that wasn't mapped to KIcon, use it and in case for 
svg based icons that take colors from the palette take the highlightedText 
color from the palette to color the icon instead of the text color, making it 
possible for styles to have white icons and white text in selected menu items 
(need explicit support from the style, patches in breeze and the like coming)


Diffs
-

  src/kiconengine.cpp 7c72ade 
  src/kiconloader.h cf2f58a 
  src/kiconloader.cpp 01d0a8d 

Diff: https://git.reviewboard.kde.org/r/127875/diff/


Testing
---


File Attachments


menu.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png


Thanks,

Marco Martin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-11 Thread Marco Martin 

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

(Updated May 11, 2016, 12:12 p.m.)


Review request for KDE Frameworks and Plasma.


Changes
---

use highlight as background color when selected


Repository: kiconthemes


Description
---

QIcon has a Selected state that wasn't mapped to KIcon, use it and in case for 
svg based icons that take colors from the palette take the highlightedText 
color from the palette to color the icon instead of the text color, making it 
possible for styles to have white icons and white text in selected menu items 
(need explicit support from the style, patches in breeze and the like coming)


Diffs (updated)
-

  src/kiconengine.cpp 7c72ade 
  src/kiconloader.h cf2f58a 
  src/kiconloader.cpp 01d0a8d 

Diff: https://git.reviewboard.kde.org/r/127875/diff/


Testing
---


File Attachments


menu.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png


Thanks,

Marco Martin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-11 Thread Marco Martin 


> On May 11, 2016, 10:52 a.m., David Edmundson wrote:
> > src/kiconloader.cpp, line 840
> > 
> >
> > 1) For the unselected case, do you think we want Text and Base rather 
> > than Window and WindowText?
> > for breeze that's(black and white rather than black and greyish)
> > 
> > 2) If so for the selected case background should change to Highlight 
> > when selected
> > 
> > I don't think HighlightedText is guaranteed to be visible over Window.

should be greyish window background as in plasma svg uses textcolor and 
backgroundcolor that are mapped to the "window" group rather than the "view" 
group, so corresponding to windowtext and window.

changing background to highlight makes sense, yes will do it (note that once 
this is settled i'll need an almost identical patch in plasma::svg to keep same 
behavior)

the icons that are changed, must be made sure (by the breeze qstyle in this 
case) that are painted over an area painted with highlight color
as in breeze highlightedtext is exactly the color of Window, so they would all 
be invisible just on top of window


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95350
---


On May 10, 2016, 2:15 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 10, 2016, 2:15 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-11 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95350
---




src/kiconloader.cpp (line 840)


1) For the unselected case, do you think we want Text and Base rather than 
Window and WindowText?
for breeze that's(black and white rather than black and greyish)

2) If so for the selected case background should change to Highlight when 
selected

I don't think HighlightedText is guaranteed to be visible over Window.


- David Edmundson


On May 10, 2016, 2:15 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 10, 2016, 2:15 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread Marco Martin 


> On May 10, 2016, 12:50 p.m., David Edmundson wrote:
> > src/kiconloader.cpp, line 839
> > 
> >
> > Aren't we missing an arg?
> > 
> > return QStringLiteral(".ColorScheme-Text {\
> > color:%1;\
> > }\
> > .ColorScheme-Background{\
> > color:%2;\
> > }\
> > .ColorScheme-Highlight{\
> > color:%3;\
> > }\
> > .ColorScheme-PositiveText{\
> > color:%4;\
> > }\
> > .ColorScheme-NeutralText{\
> > color:%5;\
> > }\
> > .ColorScheme-NegativeText{\
> > color:%6;\
> > }");
> > 
> > 
> > == 6
> > 
> > this (before and after) has 5.
> > 
> > background is missing.

proper colors are .arg ed now


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95345
---


On May 10, 2016, 2:15 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 10, 2016, 2:15 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread Marco Martin 

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

(Updated May 10, 2016, 2:15 p.m.)


Review request for KDE Frameworks and Plasma.


Repository: kiconthemes


Description
---

QIcon has a Selected state that wasn't mapped to KIcon, use it and in case for 
svg based icons that take colors from the palette take the highlightedText 
color from the palette to color the icon instead of the text color, making it 
possible for styles to have white icons and white text in selected menu items 
(need explicit support from the style, patches in breeze and the like coming)


Diffs (updated)
-

  src/kiconengine.cpp 7c72ade 
  src/kiconloader.h cf2f58a 
  src/kiconloader.cpp 01d0a8d 

Diff: https://git.reviewboard.kde.org/r/127875/diff/


Testing
---


File Attachments


menu.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png


Thanks,

Marco Martin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95347
---




src/kiconloader.cpp (line 827)


endsWith has a QLatin1String overload, QL1S is preferred then.



src/kiconloader.cpp (line 878)


QL1S here too


- Kai Uwe Broulik


On Mai 10, 2016, 10:56 vorm., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated Mai 10, 2016, 10:56 vorm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95345
---




src/kiconloader.cpp (line 839)


Aren't we missing an arg?

return QStringLiteral(".ColorScheme-Text {\
color:%1;\
}\
.ColorScheme-Background{\
color:%2;\
}\
.ColorScheme-Highlight{\
color:%3;\
}\
.ColorScheme-PositiveText{\
color:%4;\
}\
.ColorScheme-NeutralText{\
color:%5;\
}\
.ColorScheme-NegativeText{\
color:%6;\
}");

== 6

this (before and after) has 5.

background is missing.


- David Edmundson


On May 10, 2016, 10:56 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 10, 2016, 10:56 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread Marco Martin 


> On May 9, 2016, 10:32 p.m., David Edmundson wrote:
> > src/kiconloader.cpp, line 841
> > 
> >
> > we're already passing a highlightedText colour for some reason.
> > 
> > Either:
> > - Kiconloader is responsible for choosing whether to use the normal 
> > colour or the highlight colour
> > OR
> >  - The SVG is
> > 
> > Right now it looks like it's a mixture?
> 
> Marco Martin wrote:
> the concept is that the svg has parts that use "textcolor" as their text.
> the monochrome icons would be mostly of text color, mostly black (they 
> can use the other named colors as well if they want).
> what this patch does is replacing the text color with highlighted text, 
> so the icon goes from black to white.
> 
> the svg can't have this conditional case "if selected" so all we can do 
> is replacing colors via c++
> 
> David Edmundson wrote:
> >the monochrome icons would be mostly of text color, mostly black (they 
> can use the other named colors as well if they want).
> 
> So if an icon currently uses textColor and highlightColor it will get 
> screwed with this patch as two previously distinct colours will be the same.

highlightedtextcolor, not highlightcoolr, but yeah, that's the idea that is 
actually wanted, it doesn't have necessarly to maintain all its semantic 
informations, just a basic contrast with the background.
(with vdg we wanted to experiment to make used areas of highlightcolr with the 
same color as well)


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95297
---


On May 10, 2016, 10:56 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 10, 2016, 10:56 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread David Edmundson 


> On May 9, 2016, 10:32 p.m., David Edmundson wrote:
> > src/kiconloader.cpp, line 841
> > 
> >
> > we're already passing a highlightedText colour for some reason.
> > 
> > Either:
> > - Kiconloader is responsible for choosing whether to use the normal 
> > colour or the highlight colour
> > OR
> >  - The SVG is
> > 
> > Right now it looks like it's a mixture?
> 
> Marco Martin wrote:
> the concept is that the svg has parts that use "textcolor" as their text.
> the monochrome icons would be mostly of text color, mostly black (they 
> can use the other named colors as well if they want).
> what this patch does is replacing the text color with highlighted text, 
> so the icon goes from black to white.
> 
> the svg can't have this conditional case "if selected" so all we can do 
> is replacing colors via c++

>the monochrome icons would be mostly of text color, mostly black (they can use 
>the other named colors as well if they want).

So if an icon currently uses textColor and highlightColor it will get screwed 
with this patch as two previously distinct colours will be the same.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95297
---


On May 10, 2016, 10:56 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 10, 2016, 10:56 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp 01d0a8d 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread Marco Martin 

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

(Updated May 10, 2016, 10:56 a.m.)


Review request for KDE Frameworks and Plasma.


Repository: kiconthemes


Description
---

QIcon has a Selected state that wasn't mapped to KIcon, use it and in case for 
svg based icons that take colors from the palette take the highlightedText 
color from the palette to color the icon instead of the text color, making it 
possible for styles to have white icons and white text in selected menu items 
(need explicit support from the style, patches in breeze and the like coming)


Diffs (updated)
-

  src/kiconengine.cpp 7c72ade 
  src/kiconloader.h cf2f58a 
  src/kiconloader.cpp 01d0a8d 

Diff: https://git.reviewboard.kde.org/r/127875/diff/


Testing
---


File Attachments


menu.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png


Thanks,

Marco Martin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-10 Thread Marco Martin 


> On May 9, 2016, 10:32 p.m., David Edmundson wrote:
> > src/kiconloader.cpp, line 311
> > 
> >
> > was createIconImage in the last frameworks?
> > 
> > if so this is an ABI break.
> > 
> > instead of using a default argument you'll have to have two methods.
> 
> Kai Uwe Broulik wrote:
> Are you sure? This is KIconLoaderPrivate

kiconloaderprivate is not exported, so should be fine


> On May 9, 2016, 10:32 p.m., David Edmundson wrote:
> > src/kiconloader.cpp, line 841
> > 
> >
> > we're already passing a highlightedText colour for some reason.
> > 
> > Either:
> > - Kiconloader is responsible for choosing whether to use the normal 
> > colour or the highlight colour
> > OR
> >  - The SVG is
> > 
> > Right now it looks like it's a mixture?

the concept is that the svg has parts that use "textcolor" as their text.
the monochrome icons would be mostly of text color, mostly black (they can use 
the other named colors as well if they want).
what this patch does is replacing the text color with highlighted text, so the 
icon goes from black to white.

the svg can't have this conditional case "if selected" so all we can do is 
replacing colors via c++


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95297
---


On May 9, 2016, 3:26 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 9, 2016, 3:26 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp b3c7166 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-09 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95297
---




src/kiconloader.cpp (line 311)


was createIconImage in the last frameworks?

if so this is an ABI break.

instead of using a default argument you'll have to have two methods.



src/kiconloader.cpp (line 820)


these should be QStringLiteral ?



src/kiconloader.cpp (line 841)


we're already passing a highlightedText colour for some reason.

Either:
- Kiconloader is responsible for choosing whether to use the normal colour 
or the highlight colour
OR
 - The SVG is

Right now it looks like it's a mixture?


- David Edmundson


On May 9, 2016, 3:26 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated May 9, 2016, 3:26 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp b3c7166 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-09 Thread Kai Uwe Broulik 


> On Mai 9, 2016, 10:32 nachm., David Edmundson wrote:
> > src/kiconloader.cpp, line 311
> > 
> >
> > was createIconImage in the last frameworks?
> > 
> > if so this is an ABI break.
> > 
> > instead of using a default argument you'll have to have two methods.

Are you sure? This is KIconLoaderPrivate


- Kai Uwe


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95297
---


On Mai 9, 2016, 3:26 nachm., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated Mai 9, 2016, 3:26 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp b3c7166 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-09 Thread Andreas Kainz 


> On Mai 9, 2016, 10:21 nachm., Andreas Kainz wrote:
> > Ship It!

I can't review the code but this is one of the best features in the past 2 years


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95309
---


On Mai 9, 2016, 3:26 nachm., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated Mai 9, 2016, 3:26 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp b3c7166 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127875: Selected state concept for icons

2016-05-09 Thread Andreas Kainz 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127875/#review95309
---


Ship it!




Ship It!

- Andreas Kainz


On Mai 9, 2016, 3:26 nachm., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127875/
> ---
> 
> (Updated Mai 9, 2016, 3:26 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon has a Selected state that wasn't mapped to KIcon, use it and in case 
> for svg based icons that take colors from the palette take the 
> highlightedText color from the palette to color the icon instead of the text 
> color, making it possible for styles to have white icons and white text in 
> selected menu items (need explicit support from the style, patches in breeze 
> and the like coming)
> 
> 
> Diffs
> -
> 
>   src/kiconengine.cpp 7c72ade 
>   src/kiconloader.h cf2f58a 
>   src/kiconloader.cpp b3c7166 
> 
> Diff: https://git.reviewboard.kde.org/r/127875/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> menu.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127875: Selected state concept for icons

2016-05-09 Thread Marco Martin 

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

Review request for KDE Frameworks and Plasma.


Repository: kiconthemes


Description
---

QIcon has a Selected state that wasn't mapped to KIcon, use it and in case for 
svg based icons that take colors from the palette take the highlightedText 
color from the palette to color the icon instead of the text color, making it 
possible for styles to have white icons and white text in selected menu items 
(need explicit support from the style, patches in breeze and the like coming)


Diffs
-

  src/kiconengine.cpp 7c72ade 
  src/kiconloader.h cf2f58a 
  src/kiconloader.cpp b3c7166 

Diff: https://git.reviewboard.kde.org/r/127875/diff/


Testing
---


File Attachments


menu.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/05/09/0fb9a44c-8db4-4a10-91e7-1a6a36e41f62__menu.png


Thanks,

Marco Martin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel