Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Hexcolor plugin #2549

Merged

Conversation

Projects
None yet
5 participants
@PhilippGackstatter
Copy link
Contributor

commented Mar 27, 2019

Basics

Do not describe the purpose here but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Thank you, you already have a lot of progress!

What your plugin currently does is basically equivalent to:

kdb setmeta /some/key check/validation "#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$"
kdb setmeta /some/key check/validation/message "Validation of key %s with value %s failed."

(at least once #2490 is implemented)

So to make it useful enough to be added to Elektra it should do more than checking for a regular expression. This extra could be:

  • that it normalizes to some color format, even if given in some other format (e.g. #123 -> 123)
  • that it has support for specifying some colors by name
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

I would like to add specifying colors like 'red', 'blue', 'white' and have them transformed to their hex equivalent by the plugin. This could be the Extended Color keywords list of CSS3 found in section 4.3.

I could also add a transformation between two or all of the following formats.
255,0,0
100%,0%,0%
#ff0000

I guess at this point I could rename it to just the color plugin.

that it normalizes to some color format, even if given in some other format (e.g. #123 -> 123)

I'm not sure what you mean by normalization in this case.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I'm not sure what you mean by normalization in this case.

Normalization is basically just what you described as transformations. To make it proper normalization, we would decide on a standard format for colors used by all of Elektra. Your plugin would than normalize (i.e. transform) all the different representations of one color into this standard format during the kdbGet call and restore the original format during kdbSet. The type pluign for example does this for booleans.

Whether the #RRGGBBAA (we should support alpha channel as well) format is the best choice for such a standard format I am not sure. It still requires some parsing and deconstruction to actually use the color (unless your library uses strings for color, which in C seems unlikely). It might be better to convert the color into a 32-bit integer. Converting red into 4294901760 (= 0xFFFF0000, ARGB) is weird, but the color can be read easily with high-level API by just using elektraGetLong (if the type = long is set as well). After that you can just use bit manipulation to extract the individual channels.

For starters, you can implement a conversion for the colors. (The Basic color keywords would be enough I think). Whether you convert to the #RRGGBBAA format or to 32 bit integers doesn't really matter for now. That shouldn't be too hard to change later on. The actual hard part of all of this is correct normalization and restoring (without creating weird edge cases where the plugin doesn't quite behave like you expect). You can take a look at the type plugin to see how this can be achieved. The plugin implemented in a very generic way, you don't need to do that, just look at what happens to keys with type boolean or enum.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Thank you for your answer! I agree that converting to an integer makes more sense. Should be more efficient and easier to handle in C. I'll implement the conversion from color names to integers next.

@markus2330
Copy link
Contributor

left a comment

Looks very nice!

Show resolved Hide resolved src/plugins/hexcolor/README.md Outdated
kdb set /tests/color/hex "#fff"
#> Suceeds, since the value is a valid hexcolor. Quotes are important!

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Also show what kdb get returns.

#> Check the /tests/color/hex key for validity
kdb set /tests/color/hex "#fff"
#> Suceeds, since the value is a valid hexcolor. Quotes are important!

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

This is not valid for shell recorder, see tests/shell/shell_recorder/tutorial_wrapper/README.md

#include <regex.h>
#include <kdbhelper.h>

int is_valid_key(Key * key, Key * parentKey)

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

can be static


static void test_hexcolor(void)
{
test_color ("#0fb", 1);

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Good that there are already tests!

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

I could also add a transformation between two or all of the following formats.

Sounds good.

I guess at this point I could rename it to just the color plugin.

Is ok from my point. Although with the current planned feature set it would be rgbcolor. But it makes sense to have all color related functionality inside of one plugin.

The actual hard part of all of this is correct normalization and restoring

It would be great to have a lib that does that or a tutorial describing how to do it.

This could be the Extended Color keywords list of CSS3 found in section 4.3.

Yes, that would be nice. It should be only copy&paste anyway.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

It would be great to have a lib that does that or a tutorial describing how to do it.

If I find time, I might write a tutorial. A library is not really the right solution, but a special plugin template might work. Also we might need to extend the origvalue metadata, to support the case where multiple plugins one after another change a keys value.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

If I find time, I might write a tutorial.

That would be nice, as it seems to be a common problem.

but a special plugin template might work

We should avoid templates (to be copied around) as much as possible. If they are not trivial, they cause huge problems in maintenance.

Also we might need to extend the origvalue metadata, to support the case where multiple plugins one after another change a keys value.

We could introduce a plugin original/<pluginname>/value. But isn't the strategy that simply the first/last wins the easier one? (To only change original/value if it was not set before.)

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

But isn't the strategy that simply the first/last wins the easier one? (To only change original/value if it was not set before.)

Yes currently it is required that origvalue is only set, if it doesn't exist yet. However, this limits how well plugins can be isolated. For example, you can have the color plugin normalize into hexadecimal (because it is arguably easier) and let another plugin (e.g. hexnumber) deal with converting it to decimal, so that it can be read via the high-level API. It basically means that once a plugin sets origvalue, the key's value must be in a standard Elektra format.

@PhilippGackstatter PhilippGackstatter force-pushed the PhilippGackstatter:hexcolor-plugin branch from d21663a to 3837382 Apr 25, 2019

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Your plugin would than normalize (i.e. transform) all the different representations of one color into this standard format during the kdbGet call and restore the original format during kdbSet

If I store the original representation in the database, and only normalize during kdbGet, then why do I have to restore anything durgin kdbSet?

This is how I understand the plugin should work.

kdb mount config.ecf user/tests/rgbcolor dump hexcolor
kdb set user/tests/rgbcolor "#aabbccdd"
#> Create a new key user/tests/rgbcolor with string "#aabbccdd"
# I'll change that syntax
kdb setmeta user/tests/rgbcolor check/hexcolor any
# Normalizes to hex
kdb get user/tests/rgbcolor
#> 0xaabbccdd
kdb set user/tests/rgbcolor green
#> Set string to "green"
# Normalizes green to hex
kdb get user/tests/rgbcolor
#> 0x00ff00ff

So this is basically what I have implemented (color names are not in yet), only that I'm normalizing during kdbSet and store it as binary internally. So kdbGet currently returns \xaa\xbb\xcc\xdd for the first example.

So where and why would I need to restore anything?

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

The KeySet that is returned from kdbGet() has to be passed to kdbSet() and you have to call kdbGet() before kdbSet(). So any changes a plugin makes during kdbGet() will be persisted during the next kdbSet() call, unless the plugin undoes them during that call.

This doesn't become apparent in your example, because you only use a single key. If you use kdb set user/tests/rgbcolor2 green instead of kdb set user/tests/rgbcolor green, both keys will be changed in the underlying file. This is unexpected behaviour.

If my explanation is to convoluted, just mount an ini file (makes it easier to see changes), play around with getting/setting multiple keys and look at how the file changes.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Ah yes, I understand that now, thanks.

# Colors are normalized to bytes
kdb get user/tests/color/hex
#> \xaa\xbb\xcc\xdd

This comment has been minimized.

Copy link
@PhilippGackstatter

PhilippGackstatter May 5, 2019

Author Contributor

@markus2330
Is it okay to return the bytes like that? I suppose this is good for any program that wants to read the values, but it's not so nice for the enduser.

This comment has been minimized.

Copy link
@kodebach

kodebach May 5, 2019

Contributor

I think it is a bit weird to parse this string inside a C program.

IMO we should either convert to decimal (i.e. 2864434397 in your case), to ensure optimal compatibility with the highlevel API. Or just convert to the standard #aabbccdd representation, as extending the highlevel API is not really hard.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 5, 2019

Contributor

I agree with @kodebach. I would prefer the decimal variant.

@PhilippGackstatter PhilippGackstatter force-pushed the PhilippGackstatter:hexcolor-plugin branch 2 times, most recently from 370ed82 to 79661e3 May 11, 2019

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

I have now added named colors and normalization. See the Readme for working examples. If there's anything left that I can add, let me know. Other than that, I think this is ready for a review.

The only two checks failing are broken links and testmod_dbus, which I don't really know why that's happening as it seems unrelated to my plugin. Maybe someone can help me understand that, thanks.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

testmod_dbus

You can ignore that one. It is very time sensitive and fails, if there is high load on the build server. I triggered a re-run.

broken links

Those links seem fine to me... @sanssecours any idea?

@mpranj

This comment has been minimized.

Copy link
Member

commented May 17, 2019

The links were fixed on master. You'll need to rebase in order to fix the build.

@sanssecours

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Those links seem fine to me... @sanssecours any idea?

The test just shows temporary connection issues with those links. I restarted the build job. Now it only shows two broken links to:

, which I unfortunately introduced somewhere in the last days. Anyway, I fixed the problem in commit bda33c3.

@kodebach
Copy link
Contributor

left a comment

Good, simple and clean solution. The only really problematic thing is the type metadata issue. The documentation is also a bit lacking. Please finish the README and also add doxygen comments to your functions, if it isn't immediately clear what the function does.

Other than that I only added suggestions for various optimizations


## Introduction

tba

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

describe the basic idea of the plugin here


## Usage

- add your plugin in `src/plugins/README.md`

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

Add a usage guide here


static const NamedColor * findNamedColor (const char * colorName)
{
const NamedColor * cur = &NamedColors[0];

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

since your array is sorted, you could use bsearch(3)

regmatch_t offsets;
int compile_failure = regcomp (&regex, regexString, REG_NOSUB | REG_EXTENDED | REG_NEWLINE);
if (compile_failure) return -1;
// regexec returns 0 on a successful match

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

if you use the bool type from stdbool.h and == 0 instead of !, you don't need the comment

return NAMED_COLOR;
}

if (!match && namedColor == NULL)

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

namedColor == NULL is always true at this point

kdb_unsigned_long_t color;
if (colVar == NAMED_COLOR)
{
const NamedColor * namedColor = findNamedColor (str);

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

If you can somehow find a way to avoid calling findNamedColor a second time that would be nice.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 20, 2019

Contributor

Is this done now?

This comment has been minimized.

Copy link
@PhilippGackstatter

PhilippGackstatter May 20, 2019

Author Contributor

I haven't found a reasonably simple way to avoid this. I think this would require a rewrite of how the validity of a key is checked and wherever it is called, which is a fairly big part of the plugin. I would leave it as is, if that's fine.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 20, 2019

Contributor

Yes, of course it is fine.

@@ -357,6 +357,11 @@ removed due to:
- no time in future to maintain
_(Kurt Micheli)_

### RGBColor

- New plugin to validate hex formatted colors (e.g. #fff or #abcd) and normalize them to rgba (#ffffffff and #aabbccdd respectively). It also has support for named colors according to the [extended color keywords](https://www.w3.org/TR/css-color-3/#svg-color) from CSS3.

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

This is not up-to-date anymore.

This comment has been minimized.

Copy link
@PhilippGackstatter

PhilippGackstatter May 20, 2019

Author Contributor

I updated the text to indicate that the plugin normalizes to decimal rrggbbaa instead of hex. Is that what you meant?


ELEKTRA_LOG_DEBUG ("Set %s to integer %s", keyName (key), colorStr);
keySetString (key, colorStr);
keySetMeta (key, "type", "unsigned_long");

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

Don't set this. It only causes problems:

  • in kdbSet you have to remove metadata you added in kdbGet
  • you must only set the metadata, if it isn't set to some other value
  • etc.

It's best, if you ignore type altogether. Instead add infos/ordering = type to the README, the the type plugin will always be executed after you normalized the plugin. That way the user can safely set type = unsigned_long themselves, if the need it.

This comment has been minimized.

Copy link
@PhilippGackstatter

PhilippGackstatter May 20, 2019

Author Contributor

I removed it.

{
const Key * meta = keyGetMeta (cur, "check/rgbcolor");
if (!meta) continue;
ColorVariant colVar = is_valid_key (cur, parentKey);

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

I think you don't need this first call, if you call elektraColorRestore unconditionally.

This comment has been minimized.

Copy link
@PhilippGackstatter

PhilippGackstatter May 20, 2019

Author Contributor

Great catch, I removed the first call.

@@ -1060,6 +1060,14 @@ description= who should see this configuration setting?
- developer: settings only relevant for developers of the application
- internal: settings which should not be seen by anyone. (e.g. intermediate steps for calculations)

[check/rgbcolor]
type= enum
any

This comment has been minimized.

Copy link
@kodebach

kodebach May 17, 2019

Contributor

AFAIK this means that the only valid value for check/rgbcolor is any. Use type= any instead.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 18, 2019

Contributor

Or type=string. Or do we really want to support binary data?

This comment has been minimized.

Copy link
@kodebach

kodebach May 18, 2019

Contributor

The code below only checks that the metakey exists, so the value actually doesn't matter.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 19, 2019

Contributor

Then it should be "empty". (like [binary], see #2705)

This comment has been minimized.

Copy link
@PhilippGackstatter

PhilippGackstatter May 20, 2019

Author Contributor

The enum was still leftover from an earlier version. As @kodebach pointed out, the keys value doesn't matter, only its existence. I set the type to empty.

@PhilippGackstatter PhilippGackstatter force-pushed the PhilippGackstatter:hexcolor-plugin branch from 49be022 to dd45a78 May 20, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Amazing job, is this ready to merge?

@kodebach I wonder if this plugin works together with hexnumber?

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Amazing job, is this ready to merge?

@markus2330
From my side this is ready 🙂

@kodebach

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

I wonder if this plugin works together with hexnumber?

The plugin now converts to decimal, so there is no need for that.

@markus2330 markus2330 merged commit 01ab194 into ElektraInitiative:master May 20, 2019

13 checks passed

LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 57.427%
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.