[Differential] [Request, 54 lines] D4831: Add new component for the greyed out labels in Item Delegates.

[David Edmundson]

davidedmundson created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  Plasma code is full of Label {opacity: 0.6}. We shouldn't be hardcoding
  style into a UI, especially with magic numbers.
  
  This adds a new component that does the same thing but will allow
  plasmoids to write more semantic code.

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/DescriptiveLabel.qml
  src/declarativeimports/plasmaextracomponents/qml/qmldir

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Updated] D4377: Fix Python dependency in test scripts for KFileMetaData (bug 375472)

[Matthieu Gallien]

mgallien added a reviewer: mgallien.

REPOSITORY
  R286 KFileMetaData

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: awilcox, mgallien
Cc: #frameworks

Jenkins-kde-ci: breeze-icons master kf5-qt5 » Linux,gcc - Build # 522 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/breeze-icons%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/522/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Mon, 27 Feb 2017 17:41:52 +
Build duration: 24 min

CHANGE SET
Revision 9ab4290b74d3800200ae46ce7b4415f1a91780de by ivan.cukic: (Added missing 
Plasma Vault icon)
  change: add icons/apps/48/plasmavault.svg


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 5 test(s), Skipped: 0 test(s), Total: 5 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 2/2 (100%)FILES 6/6 (100%)CLASSES 6/6 (100%)LINE 222/274 
(81%)CONDITIONAL 125/218 (57%)

By packages
  
default>
FILES 1/1 (100%)CLASSES 1/1 (100%)LINE 42/57 (74%)CONDITIONAL 
14/26 (54%)
autotests
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 180/217 (83%)CONDITIONAL 
111/192 (58%)

Jenkins-kde-ci: breeze-icons master kf5-qt5 » Linux,gcc - Build # 522 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/breeze-icons%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/522/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Mon, 27 Feb 2017 17:41:52 +
Build duration: 24 min

CHANGE SET
Revision 9ab4290b74d3800200ae46ce7b4415f1a91780de by ivan.cukic: (Added missing 
Plasma Vault icon)
  change: add icons/apps/48/plasmavault.svg


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 5 test(s), Skipped: 0 test(s), Total: 5 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 2/2 (100%)FILES 6/6 (100%)CLASSES 6/6 (100%)LINE 222/274 
(81%)CONDITIONAL 125/218 (57%)

By packages
  
default>
FILES 1/1 (100%)CLASSES 1/1 (100%)LINE 42/57 (74%)CONDITIONAL 
14/26 (54%)
autotests
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 180/217 (83%)CONDITIONAL 
111/192 (58%)

[Differential] [Abandoned] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Roman Gilg]

subdiff abandoned this revision.
subdiff added a comment.


  Not anymore relevant thanks to https://phabricator.kde.org/D4827.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: subdiff, #plasma, mart
Cc: mart, ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Closed] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:916bcbf6c0ac: possibility to use a fallback chain as 
prefix (authored by mart).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4827?vs=11912=11913

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

AFFECTED FILES
  src/declarativeimports/core/framesvgitem.cpp
  src/declarativeimports/core/framesvgitem.h
  src/declarativeimports/plasmastyle/ToolButtonStyle.qml
  src/desktoptheme/breeze/widgets/button.svgz

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, davidedmundson, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Abandoned] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart abandoned this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, davidedmundson, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Updated, 67 lines] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart updated this revision to Diff 11912.
mart added a comment.


  - use m_prefixes.last()

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4827?vs=11910=11912

BRANCH
  phab/framesvgFallback

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

AFFECTED FILES
  src/declarativeimports/core/framesvgitem.cpp
  src/declarativeimports/core/framesvgitem.h
  src/declarativeimports/plasmastyle/ToolButtonStyle.qml
  src/desktoptheme/breeze/widgets/button.svgz

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, davidedmundson, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> davidedmundson wrote in framesvgitem.cpp:571-574
> this !found is a behavioural change on the existing code.
> 
> Why are we adding it?

before it did set the elementprefix on the framesvg regardless it was present 
or not, so this is done in order precisely to not do a behavioral change.

to be more exact to be sure no behavioral change occurred at all, i would have 
to do

if (!found) {

  m_frameSvg->setElementPrefix(m_prefixes.last());

}

which if doesn't exist, is pretty much the same as setting an empty string, but 
i can change it to do so

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

Jenkins-kde-ci: breeze-icons master stable-kf5-qt5 » Linux,gcc - Build # 522 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/breeze-icons%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/522/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Mon, 27 Feb 2017 17:41:52 +
Build duration: 2 min 39 sec

CHANGE SET
Revision 9ab4290b74d3800200ae46ce7b4415f1a91780de by ivan.cukic: (Added missing 
Plasma Vault icon)
  change: add icons/apps/48/plasmavault.svg


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 5 test(s), Skipped: 0 test(s), Total: 5 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 2/2 (100%)FILES 6/6 (100%)CLASSES 6/6 (100%)LINE 222/274 
(81%)CONDITIONAL 125/218 (57%)

By packages
  
default>
FILES 1/1 (100%)CLASSES 1/1 (100%)LINE 42/57 (74%)CONDITIONAL 
14/26 (54%)
autotests
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 180/217 (83%)CONDITIONAL 
111/192 (58%)

Jenkins-kde-ci: breeze-icons master stable-kf5-qt5 » Linux,gcc - Build # 522 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/breeze-icons%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/522/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Mon, 27 Feb 2017 17:41:52 +
Build duration: 2 min 39 sec

CHANGE SET
Revision 9ab4290b74d3800200ae46ce7b4415f1a91780de by ivan.cukic: (Added missing 
Plasma Vault icon)
  change: add icons/apps/48/plasmavault.svg


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 5 test(s), Skipped: 0 test(s), Total: 5 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 2/2 (100%)FILES 6/6 (100%)CLASSES 6/6 (100%)LINE 222/274 
(81%)CONDITIONAL 125/218 (57%)

By packages
  
default>
FILES 1/1 (100%)CLASSES 1/1 (100%)LINE 42/57 (74%)CONDITIONAL 
14/26 (54%)
autotests
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 180/217 (83%)CONDITIONAL 
111/192 (58%)

[Differential] [Requested Changes] D4827: possibility to use a fallback chain as prefix

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> framesvgitem.cpp:571-574
> +if (!found) {
> +qWarning() << "The image" << m_frameSvg->imagePath() << "doesn't 
> contain any of the prefixes" << m_prefixes;
> +m_frameSvg->setElementPrefix(QString());
> +}

this !found is a behavioural change on the existing code.

Why are we adding it?

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Accepted] D4827: possibility to use a fallback chain as prefix

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  note there is a minor API break for any JS code that does
  
  if (prefix === "someValue")
  
  but I've checked and no-one does...(and it would be a bit of a weird thing to 
do)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  phab/framesvgFallback

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Updated, 66 lines] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart updated this revision to Diff 11910.
mart added a comment.


  - logic all in applyPrefixes()

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4827?vs=11904=11910

BRANCH
  phab/framesvgFallback

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

AFFECTED FILES
  src/declarativeimports/core/framesvgitem.cpp
  src/declarativeimports/core/framesvgitem.h
  src/declarativeimports/plasmastyle/ToolButtonStyle.qml
  src/desktoptheme/breeze/widgets/button.svgz

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D4827#90608, @davidedmundson wrote:
  
  > Either:
  >
  >   a QML client could just do 
  >   prefix: frame.hasElementPrefix("toolbutton-pressed") ? 
"toolbutton-pressed" : "pressed"
  >   
  >   
  >
  > or this doesn't work
  >
  > It's the same thing
  >
  > both are just using for m_frameSvg->hasElementPrefix()
  
  
  ah, yes, of course.
  (since the irc discussion you quoted was about falling back between files, i 
tought that was the topic, while here is about falling back between prefixes)
  
  anyways yes, that qml chunk does the same thing, tough it would not work when 
you change the theme from one that has the prefix to one that hasn't, as it's 
querying a method so can't be binded. (and is uglier qml, that being the part 
that breaks more easily, i think is better to have uglier c++ in order to have 
prettier qml)

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4827: possibility to use a fallback chain as prefix

[David Edmundson]

davidedmundson added a comment.


  Either:
  
a QML client could just do 
prefix: frame.hasElementPrefix("toolbutton-pressed") ? 
"toolbutton-pressed" : "pressed"
  
  or this doesn't work
  
  It's the same thing
  
  both are just using for m_frameSvg->hasElementPrefix()

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Updated] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart marked an inline comment as done.
mart added a comment.


  In https://phabricator.kde.org/D4827#90599, @davidedmundson wrote:
  
  > > [15:20] ‎<‎notmart‎>‎ if the svg doesn't exists, it falls back to the 
breeze one
  > > ‎[15:20] ‎<‎notmart‎>‎ then all hasElementPrefix would check if said 
prefix exists in the breeze theme
  >
  > That's exactly what this code is doing, just in a much more complicated 
way...
  
  
  no, because buttons.svg would exist in the old theme that does not have the 
prefix

INLINE COMMENTS

> broulik wrote in framesvgitem.cpp:287
> Maybe print a warning if none found? Or how did the old code behave where we 
> would always call this no matter if we had it?

yeah, it would always do it (and for framesvg setting an inexistent prefix has 
the effect of setting an empty one)
so perhaps if none found, print a warning and set one (the first, the last, 
whatever)

> broulik wrote in framesvgitem.cpp:321
> Sure that QML gives you a QStringList?

yep, just works

> broulik wrote in framesvgitem.cpp:438
> I think this loop which is in three places should be turned into a private 
> method void applyPrefixes(const QStringList )

ok

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart added a comment.


  F2657134: Spectacle.X22789.png 

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4827: possibility to use a fallback chain as prefix

[David Edmundson]

davidedmundson added a comment.


  > [15:20] ‎<‎notmart‎>‎ if the svg doesn't exists, it falls back to the 
breeze one
  > ‎[15:20] ‎<‎notmart‎>‎ then all hasElementPrefix would check if said prefix 
exists in the breeze theme
  
  That's exactly what this code is doing, just in a much more complicated way...

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Changed Subscribers] D4827: possibility to use a fallback chain as prefix

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> framesvgitem.cpp:287
> +m_frameSvg->setElementPrefix(prefix);
> +break;
> +}

Maybe print a warning if none found? Or how did the old code behave where we 
would always call this no matter if we had it?

> framesvgitem.cpp:321
> +prefixList << prefixes.toString();
> +} else if (prefixes.canConvert()) {
> +prefixList = prefixes.toStringList();

Sure that QML gives you a QStringList?

> framesvgitem.cpp:438
> +//if the theme changed, the available prefix may have changed as well
> +for (const QString  : m_prefixes) {
> +if (m_frameSvg->hasElementPrefix(prefix)) {

I think this loop which is in three places should be turned into a private 
method void applyPrefixes(const QStringList )

> framesvgitem.h:122
>   * for a list of paths and prefixes
> + * It cal also be an array of strings, specifying a fallback chain in 
> case
> + * the first element isn't found in the theme, eg ["toolbutton-normal", 
> "normal"]

*can

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: broulik, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Request, 58 lines] D4827: possibility to use a fallback chain as prefix

[Marco Martin]

mart created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  if a framesvgitem has an array as prefix, like
  
  FrameSvg {
   prefix: ["toolbutton-hover", "hover"]
  }
  
  it will use the first available prefix, so on themes that
  have toolbutton-hover, that one will be used, old themes will
  continue to use "hover" as prefix

TEST PLAN
  tested the calendar with breeze theme has breeze-widget style
  toolbuttons, with olt themes as air, the old behavior is still
  there, switching on the fly works

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  phab/framesvgFallback

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

AFFECTED FILES
  src/declarativeimports/core/framesvgitem.cpp
  src/declarativeimports/core/framesvgitem.h
  src/declarativeimports/plasmastyle/ToolButtonStyle.qml
  src/desktoptheme/breeze/widgets/button.svgz

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Updated] D4801: KNotifications cleanup

[Valerio Pilo]

vpilo edited the summary of this revision.

REPOSITORY
  R289 KNotifications

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: #frameworks

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D4797#90144, @broulik wrote:
  
  > > So changing only the theme would mean to change the svg of `ButtonStyle`, 
which is not desirable.
  >
  > Then we need to introduce new elements in `widgets/button.svg` or a new 
`widget/toolbutton.svg` for `ToolButton` which is used and falls back to the 
old behavior if not present (to not break compat with other themes). 
`QToolButton` is QtWidgets which has nothing to do with QML or Plasma styles.
  
  
  having a new svg would make it easier for fallback (would fallback to breeze 
toolbutton for old themes) even tough i like the idea of keeping a single svg 
instead and trying to give some fallback api to framesvgitem

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: subdiff, #plasma, mart
Cc: mart, ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Requested Changes] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Marco Martin]

mart requested changes to this revision.
mart added a comment.
This revision now requires changes to proceed.


  in general, this gives me the idea that it may be a good idea to try to 
expand framesvgitem api to have things like
  
FrameSvgItem {
prefix: ["toolbutton-normal", "normal"]
}

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: subdiff, #plasma, mart
Cc: mart, ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Accepted] D4762: [SliderStyle] Allow providing a "hint-handle-size" element

[Marco Martin]

mart accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, #vdg, mart
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D4797#90142, @subdiff wrote:
  
  > In https://phabricator.kde.org/D4797#90126, @broulik wrote:
  >
  > > Design looks ok but still you can't change `ToolButtonStyle`, instead, 
the Breeze Plasma theme needs to be changed.
  >
  >
  > Sorry, I didn't quite get it the last time you mentioned it. I think I 
understand it now. You mean it will make other themes look like Breeze, when we 
insert the code in `ToolButtonStyle`, correct? But in this case take a look at 
the current `ToolButtonStyle` code: It takes `ButtonStyle`'s svg frame directly 
and uses it for its hovered/pressed state. So changing only the theme would 
mean to change the svg of `ButtonStyle`, which is not desirable.
  >
  > Also I tested it now with the Oxygen and United themes, and `QToolButton` 
has still the same flat frame line in highlight color around it when hovered 
like in Breeze, so it seems to be independent of the theme in `QToolButton` as 
well. In the Oxygen case it is though different looking in the pressed and 
checked state. Here it has a "depth" effect to it (or it's only the frame line 
getting bigger). Not sure how we can replicate that.
  
  
  It needs to have new svg theme elements (and fallback to the currently used 
prefixes if not found) would be like toolbutton-hover and toolbutton-ressed 
prefixes in the button svg (i think for the pressed one is fine to keep it 
simple and keep the normal pressed button tough)

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: subdiff, #plasma
Cc: mart, ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

> sebas wrote in iconitem.cpp:313
> Changing roundToIconSize may change the size of the icon, so should we 
> trigger size changes (paintedWidth / paintedHeight / boundingRect likely? 
> Perhaps others.) and re-rendering here as well? Right now, judging from the 
> code, changing this property mid-flight is broken. We may even have to go as 
> far as loading a new pixmap (in loadPixmap() from a quick glance).
> 
> Please also add unit tests for the effects on the iconitem's size properties.

The only property that should change is paintedWidth/paintedHeight.

> sebas wrote in iconitem.h:147
> bool roundToIconSize() const;
> 
> we generally don't use "is" in new code where we can avoid it, and the ing 
> makes this even more inconsistent. I'd much prefer the above suggestion.
> 
> Please also add api docs, at least for new code.

The property is documented, I think there's no point in documenting the 
getters/setters as you can't use them from QML anyway.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma, sebas
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol

[Differential] [Updated, 68 lines] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca updated this revision to Diff 11897.
drosca added a comment.


  Rename getter
  Emit paintedSizeChanged + schedulePixmapUpdate

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4689?vs=11845=11897

BRANCH
  arcpatch-D4689

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

AFFECTED FILES
  autotests/iconitemtest.cpp
  autotests/iconitemtest.h
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, sebas, #plasma
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol