D28134: Add ColorUtils

2020-04-08 Thread Allen Winter 
winterz added a comment.


  In D28134#643899 , @kossebau wrote:
  
  > Seems to no be compatible with Qt 5.12, see failing on CI: 
https://build.kde.org/view/Failing/job/Frameworks/job/kirigami/job/kf5-qt5%20SUSEQt5.12/lastFailedBuild/console
  
  
  same for Qt5.13.2
  please revert or fix asap.  lots of the build stack depends on kirigami
  
[3/5] Building CXX object 
src/CMakeFiles/kirigamiplugin.dir/kirigamiplugin.cpp.o
FAILED: src/CMakeFiles/kirigamiplugin.dir/kirigamiplugin.cpp.o 
/data/kde/src/5/frameworks/kirigami/src/kirigamiplugin.cpp:248:122: error: 
invalid user-defined conversion from ‘KirigamiPlugin::registerTypes(const 
char*)::’ to ‘QObject* (*)(QQmlEngine*, 
QJSEngine*)’ [-fpermissive]
  248 | qmlRegisterSingletonType(uri, 2, 12, "ColorUtils", 
[](QQmlEngine*, QJSEngine*) { return new ColorUtils; });
  | 
 ^
/data/kde/src/5/frameworks/kirigami/src/kirigamiplugin.cpp:248:68: note: 
candidate is: ‘KirigamiPlugin::registerTypes(const char*)operator ColorUtils* (*)(QQmlEngine*, QJSEngine*)() const’ 
  248 | qmlRegisterSingletonType(uri, 2, 12, "ColorUtils", 
[](QQmlEngine*, QJSEngine*) { return new ColorUtils; });
  |^
/data/kde/src/5/frameworks/kirigami/src/kirigamiplugin.cpp:248:68: note:   
no known conversion from ‘ColorUtils* (*)(QQmlEngine*, QJSEngine*)’ to 
‘QObject* (*)(QQmlEngine*, QJSEngine*)’
In file included from /usr/include/qt5/QtQml/qqmlengine.h:47,
 from /usr/include/qt5/QtQml/QQmlEngine:1,
 from 
/data/kde/src/5/frameworks/kirigami/src/kirigamiplugin.h:14

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: winterz, dfaure, kossebau, fvogt, davidedmundson, plasma-devel, 
fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-08 Thread David Faure 
dfaure added a comment.


  Breaks compilation with Qt 5.13 too.
  
/data/kde/src/5/frameworks/kirigami/src/kirigamiplugin.cpp:248:122: error: 
invalid user-defined conversion from ‘KirigamiPlugin::registerTypes(const 
char*)::’ to ‘QObject* (*)(QQmlEngine*, 
QJSEngine*)’ [-fpermissive]
  248 | qmlRegisterSingletonType(uri, 2, 12, "ColorUtils", 
[](QQmlEngine*, QJSEngine*) { return new ColorUtils; });
  | 
 ^
  
  This overload is new in Qt 5.14.
  Please use something that works with 5.12, and/or use a QT_VERSION #if.

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: dfaure, kossebau, fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, 
domson, dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-07 Thread Friedrich W. H. Kossebau 
kossebau added a comment.


  Seems to no be compatible with Qt 5.12, see failing on CI: 
https://build.kde.org/view/Failing/job/Frameworks/job/kirigami/job/kf5-qt5%20SUSEQt5.12/lastFailedBuild/console

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: kossebau, fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-06 Thread Carson Black 
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:4c7780ea5cf7: Add ColorUtils (authored by cblack).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D28134?vs=79525=79529#toc

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=79525=79529

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-06 Thread Marco Martin 
mart added a comment.


  thanks, almost ready to go :)

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-06 Thread Carson Black 
cblack updated this revision to Diff 79525.
cblack added a comment.


  ​

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=79333=79525

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-06 Thread Marco Martin 
mart requested changes to this revision.
mart added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> colorutils.h:73
> + */
> +Q_INVOKABLE PendingValue* averageColorForItem(QVariant item, int 
> maxPixels = 65536);
> +

*sigh* should be more clear...
what i don't want in a singleton type, and i'm not going to accept until is 
there, is this whole grabbing of items.

in a singleton like that, i would want uniquely small, stateless and sync 
functions that take QColors as input and have qcolors (or any other simple 
value lile ints and what not) as output, nothing else

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-04 Thread Carson Black 
cblack updated this revision to Diff 79333.
cblack added a comment.


  Rewrite PendingValue to use QFuture

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=79327=79333

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-04 Thread Carson Black 
cblack updated this revision to Diff 79327.
cblack marked 2 inline comments as done.
cblack added a comment.


  Address issues

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=79097=79327

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-04 Thread Fabian Vogt 
fvogt added inline comments.

INLINE COMMENTS

> colorutils.cpp:90
> +});
> +}
> +

If item is neither of those three, it would never call `pending->setValue` at 
all

> colorutils.cpp:103
> +
> +if (pending->value().isValid()) {
> + pending->setValue(luma(result->value().value()) > 0.5 ? 
> ColorUtils::Brightness::Light : ColorUtils::Brightness::Dark);

Is this supposed to be `result->value().isValid()` instead?

If no, how can `pending` be valid at this point`?

If yes, how can `result` be valid at this point? It would also mean that 
`ready` wouldn't get emitted again, which results in a leak.

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-01 Thread Carson Black 
cblack updated this revision to Diff 79097.
cblack added a comment.


  Address feedback

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=78510=79097

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, 
ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-04-01 Thread Marco Martin 
mart requested changes to this revision.
mart added a comment.
This revision now requires changes to proceed.


  still, no item grabbing

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, 
ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-03-25 Thread Carson Black 
cblack updated this revision to Diff 78510.
cblack added a comment.


  Fix errors

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=78188=78510

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, 
ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-03-21 Thread Carson Black 
cblack added a comment.


  In D28134#632087 , @davidedmundson 
wrote:
  
  > What's your intended usecase?
  
  
  Generating UI colours from images, icons, etc. and then applying 
transformations on them to make them more usable.
  
  Examples:
  
  - PlaMo's splash screen uses something like this to generate a splash colour 
background
  - Plasma BigScreen cards also uses something like this to generate card 
backgrounds
  - Ikona could use this to determine what background and foreground to use for 
displaying icons from various icon themes at sufficient contrast

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, 
ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-03-21 Thread David Edmundson 
davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  What's your intended usecase?
  
  Is Kirigami the right place?  The original scope for that was "core 
application building blocks".

INLINE COMMENTS

> colorutils.cpp:78
> +
> +if (item->canConvert()) {
> +auto casted = item->value();

Depending on use case, it might be better to take a QSGTextureProvider

then images can give you a surface directly without the blit, and you can still 
take composite items through a ShaderEffectSource

> pendingvalue.h:25
> + */
> +Q_INVOKABLE QVariant await();
> +

Having a nested event loop called from QML code is really really dangerous.

If you want it for unit tests or something, fine.

Invokable, absolutely not.

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, 
ngraham, apol, ahiemstra, mart

D28134: Add ColorUtils

2020-03-21 Thread Carson Black 
cblack updated this revision to Diff 78188.
cblack added a comment.


  Avoid leaving unused connections; effectively removing leaking state related 
to item grabbing

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=78187=78188

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-21 Thread Carson Black 
cblack updated this revision to Diff 78187.
cblack added a comment.


  Make brightnessForItem and averageColorForItem take QVariants

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=78069=78187

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-20 Thread Marco Martin 
mart requested changes to this revision.
mart added a comment.
This revision now requires changes to proceed.


  please no item grabbing in the singlethon. item grabbing is *not* stateless

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-19 Thread Carson Black 
cblack updated this revision to Diff 78069.
cblack added a comment.


  Add linear interpolation, alpha blending, colour adjustment, and colour 
scaling

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=78030=78069

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-19 Thread Carson Black 
cblack updated this revision to Diff 78030.
cblack added a comment.


  Asynchronous return values

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=77976=78030

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp
  src/pendingvalue.cpp
  src/pendingvalue.h

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-19 Thread Marco Martin 
mart added a comment.


  First of all.. I love the idea! :)
  
  I wanted something like that, tough i had a slightly different approach.. i 
find the global average color usually producing a kinda washed down color (from 
what happens in the plasma mobile app startup screen)
  
  For another project i needed something along the lines, and ended up using a 
variant of the k-means clustering technique, so you have averages of the colors 
grouped in clusters by frequency which can generate interesting palettes to 
play with (then is trivial to add a global average if really needed).
  
https://towardsdatascience.com/extracting-colours-from-an-image-using-k-means-clustering-9616348712be
  
  the first implementation is there (i want a way better api for it to be 
integrated in kirigami tough)
  
https://invent.kde.org/kde/plasma-bigscreen/-/blob/master/components/imagepalette.h

INLINE COMMENTS

> colorutils.cpp:26
> +connect(response.data(), ::ready, , 
> ::quit);
> +loop.exec();
> +

sync calls should be avoided whenever possible

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-18 Thread Carson Black 
cblack updated this revision to Diff 77976.
cblack added a comment.


  Improve documentation comments

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=77975=77976

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-18 Thread Carson Black 
cblack updated this revision to Diff 77975.
cblack added a comment.


  Better class description

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=77974=77975

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-18 Thread Carson Black 
cblack updated this revision to Diff 77974.
cblack added a comment.


  Add comments to code

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=77973=77974

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-18 Thread Carson Black 
cblack updated this revision to Diff 77973.
cblack added a comment.


  Use words instead of single letters

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=77972=77973

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-18 Thread Carson Black 
cblack updated this revision to Diff 77972.
cblack added a comment.


  Add EOF newlines

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28134?vs=77971=77972

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart

D28134: Add ColorUtils

2020-03-18 Thread Carson Black 
cblack created this revision.
cblack added reviewers: Plasma, mart.
Herald added a project: Kirigami.
Herald added a subscriber: plasma-devel.
cblack requested review of this revision.

REVISION SUMMARY
  ColorUtils is a class that offers utilities for working with colour, offering 
the following items as a start:
  
  - `QColor averageColorForItem(QQuickItem*, int)` - Obtains the average colour 
of an item, ignoring transparent pixels
  - `Brightness brightnessForItem(QQuickItem*)` - Obtains the brightness of an 
item

REPOSITORY
  R169 Kirigami

BRANCH
  cblack/colour-utils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/colorutils.cpp
  src/colorutils.h
  src/kirigamiplugin.cpp

To: cblack, #plasma, mart
Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, 
ahiemstra, davidedmundson, mart