D13232: introduce Custom color set

[Marco Martin]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R169:e3b72a04411f: introduce Custom color set (authored by 
mart).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D13232?vs=35612&id=35676#toc

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13232?vs=35612&id=35676

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

AFFECTED FILES
  examples/gallerydata/contents/ui/gallery/ColorSetGallery.qml
  src/libkirigami/basictheme.cpp
  src/libkirigami/basictheme_p.h
  src/libkirigami/platformtheme.cpp
  src/libkirigami/platformtheme.h
  src/styles/Material/Theme.qml

To: mart, #kirigami, broulik
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Marco Martin]

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


  - confised prototype for custom colors handling
  - coloroverrides has approach
  - add missing file
  - another route: Custom color set
  - propagate all colors
  - export a palette in the basictheme
  - propagate colors to material
  - all colors writeable
  - remove dead code
  - use customColors copy properties
  - possible to have spinboxes on new line
  - make color propagation work for material

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13232?vs=35517&id=35612

BRANCH
  mart/customColors

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

AFFECTED FILES
  examples/gallerydata/contents/ui/gallery/ColorSetGallery.qml
  src/libkirigami/basictheme.cpp
  src/libkirigami/basictheme_p.h
  src/libkirigami/platformtheme.cpp
  src/libkirigami/platformtheme.h
  src/styles/Material/Theme.qml

To: mart, #kirigami, broulik
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Marco Martin]

mart added a comment.


  D13233  not necassary anymore

REPOSITORY
  R169 Kirigami

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

To: mart, #kirigami, broulik
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Marco Martin]

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


  - remove dead code

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13232?vs=35243&id=35516

BRANCH
  mart/customColors

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

AFFECTED FILES
  examples/gallerydata/contents/ui/gallery/ColorSetGallery.qml
  src/libkirigami/basictheme.cpp
  src/libkirigami/platformtheme.cpp
  src/libkirigami/platformtheme.h
  src/styles/Material/Theme.qml

To: mart, #kirigami, broulik
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Marco Martin]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R169:bf90602e90ea: remove dead code (authored by mart).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D13232?vs=35516&id=35517#toc

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13232?vs=35516&id=35517

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

AFFECTED FILES
  examples/gallerydata/contents/ui/gallery/ColorSetGallery.qml
  src/kirigamiplugin.cpp
  src/libkirigami/basictheme.cpp
  src/libkirigami/platformtheme.h

To: mart, #kirigami, broulik
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> broulik wrote in kirigamiplugin.cpp:172
> What's this?

gah, sorry, remains of old attempts

> broulik wrote in platformtheme.cpp:314
> Can this lead to issues with non-deterministic setting of properties? Ie. say 
> `Kirigami.Theme.textColor` is evaluated before `Kirigami.Theme.colorSet: 
> Kirigami.Theme.Custom`?

it would work fine, as writing to color properties always works...
which is a thing i don't like much, but either properties are writable or not.
if one writes the proeprties with another colorset, it would "partly" work, 
then all the customizations go away as soon as one of the implementations does 
a syncColors(), which is not pretty.

the only thing i can think about to make it work in a more predictable way is:

duplicate every setter, with setCustomTextColor etc (so setTextColor to be used 
only by implementations), and store all the custom colors in different members, 
then textColor() would be return colorSet == custom ? d->customTextColor : 
d->textColor

(then a reset method could be implemented

> broulik wrote in platformtheme.h:79
> Do they need a `RESET`?

perhaps... i wouldn't know how to implement one with the current architecture
(see the other comment for an idea)

> broulik wrote in platformtheme.h:220
> Where is this used?

crap, another remain of old approach :)

REPOSITORY
  R169 Kirigami

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

To: mart, #kirigami, broulik
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kirigamiplugin.cpp:172
>  
> +//qmlRegisterUncreatableType(uri, 2, 5, 
> "ThemeOverride", "Cannot create objects of type ThemeOverride, use it as an 
> attached poperty");
> +

What's this?

> basictheme.cpp:138
> +//TODO: primary, accent and background
> +
> QMetaObject::invokeMethod(basicThemeDeclarative()->instance(this), 
> "__propagateTextColor", Q_ARG(QVariant, QVariant::fromValue(this->parent())), 
> Q_ARG(QVariant, textColor()));
> +
> QMetaObject::invokeMethod(basicThemeDeclarative()->instance(this), 
> "__propagateBackgroundColor", Q_ARG(QVariant, 
> QVariant::fromValue(this->parent())), Q_ARG(QVariant, backgroundColor()));

Probably prints warnings for themes that don't have this?

> platformtheme.cpp:314
> +#define PROPAGATECUSTOMCOLOR(colorName, color)\
> +  if (colorSet() == Custom) {\
> +  for (PlatformTheme *t : d->m_childThemes) {\

Can this lead to issues with non-deterministic setting of properties? Ie. say 
`Kirigami.Theme.textColor` is evaluated before `Kirigami.Theme.colorSet: 
Kirigami.Theme.Custom`?

> platformtheme.h:79
>   */
> -Q_PROPERTY(QColor textColor READ textColor NOTIFY colorsChanged)
> +Q_PROPERTY(QColor textColor READ textColor WRITE setTextColor NOTIFY 
> colorsChanged)
>  

Do they need a `RESET`?

> platformtheme.h:220
>  void inheritChanged(bool inherit);
> +void colorOverridesChanged(const QJsonObject &overrides);
>  

Where is this used?

REPOSITORY
  R169 Kirigami

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

To: mart, #kirigami, broulik
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Marco Martin]

mart added a comment.


  F5880591: image.png 

REPOSITORY
  R169 Kirigami

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

To: mart, #kirigami
Cc: plasma-devel, apol, davidedmundson, mart, hein

D13232: introduce Custom color set

[Marco Martin]

mart created this revision.
mart added a reviewer: Kirigami.
Restricted Application added a project: Kirigami.
Restricted Application added a subscriber: plasma-devel.
mart requested review of this revision.

REVISION SUMMARY
  In some scenarios, some applications need certain areas with completely custom
  palettes, for instance if a piece of ui is on top of an image it may make
  sense that hierarchy uses whatever the dominant colors of such image are.
  in the scenario of a Custom color set, the qml code would look like this:
  Item {
  
Kirigami.Theme.inherit: false
Kirigami.Theme.colorSet: Kirigami.Theme.Custom
Kirigami.Theme.textColor: "#af2352"
Kirigami.Theme.backgroundColor: "#124523"
...
  
  }

TEST PLAN
  piece with a custom set in the gallery

REPOSITORY
  R169 Kirigami

BRANCH
  mart/customColors

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

AFFECTED FILES
  examples/gallerydata/contents/ui/gallery/ColorSetGallery.qml
  src/kirigamiplugin.cpp
  src/libkirigami/basictheme.cpp
  src/libkirigami/platformtheme.cpp
  src/libkirigami/platformtheme.h
  src/styles/Material/Theme.qml

To: mart, #kirigami
Cc: plasma-devel, apol, davidedmundson, mart, hein