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.

Sign up for GitHub

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

Jump to bottom

[hue] Semantics added to channel types #10977

Merged
merged 3 commits into from
Feb 8, 2022
Merged

[hue] Semantics added to channel types #10977

merged 3 commits into from
Feb 8, 2022

Conversation

lolodomo
Copy link
Contributor

Signed-off-by: Laurent Garnier [email protected]

@lolodomo lolodomo requested a review from cweitkamp as a code owner July 11, 2021 18:52
@Skinah Skinah added the enhancement An enhancement or new feature for an existing add-on label Jul 12, 2021
Skinah
Skinah approved these changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@Skinah Skinah left a comment •
edited
Loading

Choose a reason for hiding this comment

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

EDIT: @lolodomo I had a second look after playing with tags the last few days....

'Control' and 'Light' tags give the locations tabs a number of how many lights are turned ON in that location.. etc.
See this for example and note the bulb icon with a number of how many lights are on in that location...
http://www.pcmus.com/openhab/locations.png

Some of the channels are tagged with these and since I don't own any HUE I am not certain but some I would question like the effect channel. Can the effect be turned ON when the light is actually OFF? I also don't believe it will make any difference tagging the String channels with this. Perhaps err on the side of caution and only tag the ones that control the light actually being ON and OFF, otherwise the light icon may read the wrong number of lights turned on in the locations tab.

@Skinah Skinah added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Aug 12, 2021
@ecdye ecdye mentioned this pull request Aug 12, 2021
Merged
@Skinah Skinah added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 14, 2021
@cweitkamp cweitkamp added the awaiting feedback Awaiting feedback from the pull request author label Oct 16, 2021
@jlaur
Copy link
Contributor

jlaur commented Jan 19, 2022

@lolodomo - is this ready for being merged?

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 20, 2022 •
edited
Loading

@lolodomo - is this ready for being merged?

No because we have a kind of veto from @kubawolanin regarding this approach.

kaikreuzer
kaikreuzer requested changes Jan 22, 2022
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

No because we have a kind of veto from @kubawolanin regarding this approach.

You probably wanted to mention me instead here.
As commented on the other PRs, I would prefer to reduce this to the "obvious" fits.
As @Skinah mentioned above, the tags in this PR might have some unexpected effects to users.
I left some comments below on channels, which I consider problematic.

bundles/org.openhab.binding.hue/src/main/resources/OH-INF/thing/channels.xml Outdated
<state readOnly="true" pattern="%.1f %unit%"/>
</channel-type>

<channel-type id="light_level" advanced="true">
<item-type>Number</item-type>
<label>Light Level</label>
<description>Current light level.</description>
<tags>
<tag>Measurement</tag>
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer "Measurement" tags to be only used for Numbers with dimension, so that it is clear, what exactly is measured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what else to use in this case ?

Copy link
Member

Choose a reason for hiding this comment

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

Following our agreement to only tag what is obvious, I'd vote to not tag it at all.

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, this is now removed.

bundles/org.openhab.binding.hue/src/main/resources/OH-INF/thing/channels.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.hue/src/main/resources/OH-INF/thing/channels.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.hue/src/main/resources/OH-INF/thing/channels.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.hue/src/main/resources/OH-INF/thing/channels.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.hue/src/main/resources/OH-INF/thing/channels.xml Outdated Show resolved Hide resolved
@lolodomo lolodomo removed the awaiting feedback Awaiting feedback from the pull request author label Feb 6, 2022
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 6, 2022

I am going to consider review comments.

@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 8, 2022

@kaikreuzer : I updated the PR, remains only one open point to discuss

@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 8, 2022

@kaikreuzer : last change done

@lolodomo
Updated tags for the channel alert
75537e4 
Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 8, 2022 •
edited
Loading

@kaikreuzer : I am counting 27 bindings defining tags on some of their channels. Maybe we/you could check that they all define tags properly.

  1. airquality
  2. danfossairunit
  3. deconz
  4. dmx
  5. feican
  6. homeconnect
  7. ipobserver
  8. lifx
  9. max
  10. mihome
  11. milight
  12. millheat
  13. mqtt.espmilighthub
  14. nikohomecontrol
  15. openweathermap
  16. openwebnet
  17. qbus
  18. rotel
  19. sagercaster
  20. sensibo
  21. shelly
  22. sonyprojector
  23. souliss
  24. touchwand
  25. vigicrues
  26. wled
  27. yeelight

@jlaur
Copy link
Contributor

jlaur commented Feb 8, 2022

@lolodomo - would you like code owner (@cweitkamp), or @kaikreuzer to review before merging? It looks good to me now after removing the tags which were not straight-forward.

jlaur
jlaur approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM.

@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 8, 2022

@lolodomo - would you like code owner (@cweitkamp), or @kaikreuzer to review before merging? It looks good to me now after removing the tags which were not straight-forward.

No, I don't think this is necessary, there is now almost nothing changed and Kai already made his comments.

@jlaur jlaur merged commit b54e102 into openhab:main Feb 8, 2022
@jlaur jlaur added this to the 3.3 milestone Feb 8, 2022
@lolodomo lolodomo deleted the hue_semantics branch February 8, 2022 22:08
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Apr 27, 2022
@lolodomo @NickWaterton
[hue] Semantics added to channel types (openhab#10977)
ee7f3d2 
* [hue] Semantics added to channel types

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jun 29, 2022
@lolodomo @nemerdaud
[hue] Semantics added to channel types (openhab#10977)
c338b99 
* [hue] Semantics added to channel types

Signed-off-by: Laurent Garnier <[email protected]>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
@lolodomo @andan67
[hue] Semantics added to channel types (openhab#10977)
b72ecb4 
* [hue] Semantics added to channel types

Signed-off-by: Laurent Garnier <[email protected]>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
@lolodomo @andrasU
[hue] Semantics added to channel types (openhab#10977)
0f9538e 
* [hue] Semantics added to channel types

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Andras Uhrin <[email protected]>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
@lolodomo @psmedley
[hue] Semantics added to channel types (openhab#10977)
30af9d9 
* [hue] Semantics added to channel types

Signed-off-by: Laurent Garnier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikreuzer kaikreuzer kaikreuzer requested changes

@Skinah Skinah Skinah approved these changes

@jlaur jlaur jlaur approved these changes

@cweitkamp cweitkamp Awaiting requested review from cweitkamp cweitkamp is a code owner

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
5 participants
@lolodomo @jlaur @kaikreuzer @Skinah @cweitkamp