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

size plugin #1574

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@tom-wa
Contributor

tom-wa commented Aug 16, 2017

Purpose

size plugin #1395

Checklist

  • commit messages are fine (with references to issues)
  • I ran all tests and everything went fine
  • I added unit tests
  • affected documentation is fixed
  • I added code comments, logging, and assertions
  • meta data is updated (e.g. README.md of plugins)

@markus2330 please review my pull request

@markus2330

Simple and elegant functionality.

Transformation back is unclear. Please rename width and height to be lower case and that it works together with rename.

## Introduction
The `size` plugin transforms values with the format `WIDTH<separator>HEIGHT` into a separate `Width` and `Height` key. The metakey `transform/size` triggers the conversion.

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

width and height should be lower case (by Elektra's convention)

Does it work togehter with the rename plugin? (Do we need a infos/ordering = rename?)

@markus2330

markus2330 Aug 16, 2017

Contributor

width and height should be lower case (by Elektra's convention)

Does it work togehter with the rename plugin? (Do we need a infos/ordering = rename?)

This comment has been minimized.

@tom-wa

tom-wa Aug 16, 2017

Contributor

Does it work togehter with the rename plugin?

i don't see any reason why shouldn't, i'll add some shelltests for it

@tom-wa

tom-wa Aug 16, 2017

Contributor

Does it work togehter with the rename plugin?

i don't see any reason why shouldn't, i'll add some shelltests for it

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

Thanks! If rename is done earlier than size, different capitalizations of width and height would not work. Its only relevant if rename and this plugin use the same placements. If they do, a simple infos/ordering = rename in the contract of this plugin here should fix the potential problem.

@markus2330

markus2330 Aug 16, 2017

Contributor

Thanks! If rename is done earlier than size, different capitalizations of width and height would not work. Its only relevant if rename and this plugin use the same placements. If they do, a simple infos/ordering = rename in the contract of this plugin here should fix the potential problem.

## Configuration
By default `x` is assumed as separator. If the plugin configuration `/separator` is set, its value is used instead. If set, `transform/size`'s value takes precedence.

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

So transform/size also contains a separator as value? Is this needed? Please document in doc/METADATA.ini.

From my point of view it should be okay to hard code the x separator (unless LCDproc itself uses the separator inconsistently)

@markus2330

markus2330 Aug 16, 2017

Contributor

So transform/size also contains a separator as value? Is this needed? Please document in doc/METADATA.ini.

From my point of view it should be okay to hard code the x separator (unless LCDproc itself uses the separator inconsistently)

This comment has been minimized.

@tom-wa

tom-wa Aug 16, 2017

Contributor

the transform/size value (per key) overrides /separator (plugin configuration), neither is needed and it defaults to x (hardcoded).
LCDproc only uses x, but it could be useful for similar things like aspect ratios which are usually separated by a :
but we can drop it if there's no need for it

@tom-wa

tom-wa Aug 16, 2017

Contributor

the transform/size value (per key) overrides /separator (plugin configuration), neither is needed and it defaults to x (hardcoded).
LCDproc only uses x, but it could be useful for similar things like aspect ratios which are usually separated by a :
but we can drop it if there's no need for it

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

Okay, if you already found a use case its okay. Maybe give aspect ratios as example for how to change the separator.

@markus2330

markus2330 Aug 16, 2017

Contributor

Okay, if you already found a use case its okay. Maybe give aspect ratios as example for how to change the separator.

kdb get /examples/size/SizeKey/Height
#> 4
kdb rm -r /examples/size

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

What happens if width or height is removed or modified?

@markus2330

markus2330 Aug 16, 2017

Contributor

What happens if width or height is removed or modified?

#include "size.h"
#include <kdbhelper.h>
#include <stdio.h>

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

Is this needed?

@markus2330

markus2330 Aug 16, 2017

Contributor

Is this needed?

char * value = elektraStrDup (keyString (key));
if (!value) return 0;
char * dptr = strstr (value, sep);
if (!dptr) return 0;

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

Isn't this a validation error? I would expect an error if a transformation is requested but no separator is available.

@markus2330

markus2330 Aug 16, 2017

Contributor

Isn't this a validation error? I would expect an error if a transformation is requested but no separator is available.

This comment has been minimized.

@tom-wa

tom-wa Aug 16, 2017

Contributor

i'm not sure if it should fail. using your example from #1395

[CFontz/Size]
transform/size

[CFontz/Size/Width]
default = 20
range = ...
type = long

[CFontz/Size/Height]
default = 4
range = ...
type = long

the value of Size would be assumed as invalid/broken/whatever and the default values used instead. imho a warning would be a better

where should we draw the line between a validation and a transformation plugin though ?

@tom-wa

tom-wa Aug 16, 2017

Contributor

i'm not sure if it should fail. using your example from #1395

[CFontz/Size]
transform/size

[CFontz/Size/Width]
default = 20
range = ...
type = long

[CFontz/Size/Height]
default = 4
range = ...
type = long

the value of Size would be assumed as invalid/broken/whatever and the default values used instead. imho a warning would be a better

where should we draw the line between a validation and a transformation plugin though ?

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

Yes, it would only be a validation error if on writing out size or height is missing (and no default exists). When parsing, emitting a warning would be needed though, we should not silently ignore metadata.

@markus2330

markus2330 Aug 16, 2017

Contributor

Yes, it would only be a validation error if on writing out size or height is missing (and no default exists). When parsing, emitting a warning would be needed though, we should not silently ignore metadata.

ksRewind (iterKS);
while ((cur = ksNext (iterKS)) != NULL)
{
keyDel (ksLookup (returned, cur, KDB_O_POP));

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2017

Contributor

I do not understand this, please add source-code comment here.

How does this code transform the width/height keys back to the original value widthxheight?
Or document the limitation if it is one.

@markus2330

markus2330 Aug 16, 2017

Contributor

I do not understand this, please add source-code comment here.

How does this code transform the width/height keys back to the original value widthxheight?
Or document the limitation if it is one.

@tom-wa

This comment has been minimized.

Show comment
Hide comment
@tom-wa

tom-wa Aug 16, 2017

Contributor

ok, it doesn't really transform the key, but just adds width and height and drops them presetstorage. i don't think theres any reason to remove the original key, but i forgot to change the value of the original key if height or width gets changed, will add it now

Contributor

tom-wa commented Aug 16, 2017

ok, it doesn't really transform the key, but just adds width and height and drops them presetstorage. i don't think theres any reason to remove the original key, but i forgot to change the value of the original key if height or width gets changed, will add it now

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Aug 16, 2017

Contributor

Ok, if it does not transform we should not name it like that. So it basically only duplicates/copies so that the API finds what it needs, but the keys are actually read-only.

What about calling it copy/size then? And we should tag the resulting keys with readonly and yield an error if the user tried to change them.

Maybe is doing a proper transformation less work and more user friendly? (Where the original key is hidden and the transformed keys can be changed.)

Contributor

markus2330 commented Aug 16, 2017

Ok, if it does not transform we should not name it like that. So it basically only duplicates/copies so that the API finds what it needs, but the keys are actually read-only.

What about calling it copy/size then? And we should tag the resulting keys with readonly and yield an error if the user tried to change them.

Maybe is doing a proper transformation less work and more user friendly? (Where the original key is hidden and the transformed keys can be changed.)

@tom-wa

This comment has been minimized.

Show comment
Hide comment
@tom-wa

tom-wa Aug 16, 2017

Contributor

already changed it so that the values will be written back if either height or width are changed. i'll hide the original key too and update the PR

Contributor

tom-wa commented Aug 16, 2017

already changed it so that the values will be written back if either height or width are changed. i'll hide the original key too and update the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment