Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

added unit to configuration description parameters according to issue… #889

Merged
merged 1 commit into from Jan 26, 2016
Merged

added unit to configuration description parameters according to issue… #889

merged 1 commit into from Jan 26, 2016

Conversation

tomhoefer
Copy link
Contributor

… #741

Signed-off-by: Thomas Höfer t.hoefer@telekom.de

kaikreuzer
kaikreuzer reviewed Jan 25, 2016
View reviewed changes
docs/documentation/development/bindings/xml-reference.md
@@ -95,6 +97,7 @@ The following HTML tags are allowed -: ```<b>, <br>, <em>, <h1>, <h2>, <h3>, <h4
<tr><td>option</td><td>The element definition of a static selection list (optional).</td></tr>
<tr><td>option.value</td><td>The value of the selection list element.</td></tr>
<tr><td>multipleLimit</td><td>If multiple is true, sets the maximum number of options that can be selected (optional).</td></tr>
<tr><td>unitLabel</td><td>The unit label represents a human readable natural language unit as iterations, runs, etc. If the unit attribute is set then the unit label attribute will be ignored. The unit label must only be set if the type of the parameter is either integer or decimal (optional).</td></tr>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the unit label attribute will be ignored

-> "is ignored"

So this means we do not have an exception when parsing the XML, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont have an exception if both unit and unit label is specified. There is only an exception during parsing when the unit or unit label is specified for a textual or a boolean parameter.

@kaikreuzer
Copy link
Contributor

Thanks @tomhoefer, looks good to me.
One issue that I would like to discuss though:
If a unit is specified, the current implementation does not deliver (e.g. through the REST API) any unitLabel. Since the unit "symbol" and its label are not always the same (see "Cel" vs. "°C" or also localized "week" vs. "Woche(n)", this is imho a problem. We should not put the effort for this on the UI side, but should address it on the server, don't you think?
I am not sure, where to best place this logic, since it needs to consider localization. Maybe the implementations of ConfigDescriptionProviders have to do it? Maybe it can be done centrally in the ConfigDescriptionRegistry?

@tomhoefer
Copy link
Contributor Author

Hmm, not sure if I understand you right. I have modified the ConfigDescriptionDTOMapper and also tested the REST interface with Accept-Language in order to retrieve an internationalized unit label.

@tomhoefer
Copy link
Contributor Author

Now I got your point ;) I will think about a solution and update this PR as soon as possible

@tomhoefer tomhoefer mentioned this pull request Jan 26, 2016
Closed
kaikreuzer
kaikreuzer reviewed Jan 26, 2016
View reviewed changes
bundles/config/org.eclipse.smarthome.config.core/ESH-INF/i18n/units.properties
@@ -0,0 +1,42 @@
unit.A=Ampere
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit label is what UIs are expected to display besides the value, right?
So wouldn't you in most cases want to display the symbol as a label and not the "prosa" name?
Otherwise you would also have to write "Meter per square second", etc., which does not make much sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok... I have also thought about this issue... I will change it...

@tomhoefer
added unit and unit label to config description parameters
091a796 
Signed-off-by: Thomas Höfer <t.hoefer@telekom.de>
kaikreuzer added a commit that referenced this pull request Jan 26, 2016
@kaikreuzer
Merge pull request #889 from qivicon/config-parameter-units
a9d3244 
added unit to configuration description parameters according to issue…
@kaikreuzer kaikreuzer merged commit a9d3244 into eclipse-archived:master Jan 26, 2016
@maggu2810 maggu2810 mentioned this pull request Jan 27, 2016
Closed
@kaikreuzer kaikreuzer mentioned this pull request Jan 28, 2016
Closed
@kaikreuzer kaikreuzer modified the milestone: 0.8.0.b3 Feb 1, 2016
@kgoderis kgoderis mentioned this pull request Apr 23, 2017
Closed
@tomhoefer tomhoefer mentioned this pull request Jul 28, 2017
Merged
@kaikreuzer kaikreuzer deleted the config-parameter-units branch July 28, 2017 10:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.0.b3
Development

Successfully merging this pull request may close these issues.
2 participants
@tomhoefer @kaikreuzer