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

[deconz] Add actions for device joining and scenes #14134

Closed
wants to merge 11 commits into from

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Jan 1, 2023

Backport of smarthomej/addons#22 and scene actions.

Originally contributed by @J-N-K

Tested with my 3.4 production installation with combination of test rules and Postman API calls (verifying results).

Related to #14124

@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Jan 1, 2023
@jlaur jlaur requested a review from a team as a code owner January 1, 2023 21:54
@jlaur
Copy link
Contributor Author

jlaur commented Jan 1, 2023

@J-N-K - please let me know if I should do anything differently with copyright notices etc. I believe I need to use the standard openHAB header for compliance, so SmartHome/J is not mentioned, but you are still mentioned with the original @author tags.

@J-N-K
Copy link
Member

J-N-K commented Jan 1, 2023

I'm sorry, replacing the license header is not possible: https://opensource.org/faq#preserve-copyright-notices.

This also part of EPL 2.0 3.3:

3.3 Contributors may not remove or alter any copyright, patent, trademark, attribution notices, disclaimers of warranty, or limitations of liability (‘notices’) contained within the Program from any copy of the Program which they Distribute, provided that Contributors may add their own appropriate notices.

You can modify it by adding openHAB's copyright line below, like SmartHome/J does:

/**
 * Copyright (c) 2010-2021 Contributors to the openHAB project
 * Copyright (c) 2021-2022 Contributors to the SmartHome/J project
 *
 * See the NOTICE file(s) distributed with this work for additional
 * information.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0
 *
 * SPDX-License-Identifier: EPL-2.0
 */

You can fix spotless like I did here or you can move the code to src/3rdparty which is excluded from spotless check.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 2, 2023

I'm sorry, replacing the license header is not possible: https://opensource.org/faq#preserve-copyright-notices.

I'm sorry for messing around here, but will need some additional advise. If it's allowed to modify the copyright header by adding multiple copyright lines:

 * Copyright (c) 2010-2022 Contributors to the openHAB project
 * Copyright (c) 2021-2022 Contributors to the SmartHome/J project

this would probably be preferable, but requires modification of tools/static-code-analysis/checkstyle/ruleset.properties as you also pointed out. The regex could then be changed to require " * Copyright (c) 2010-2022 Contributors to the openHAB project" but allow other lines starting with " * Copyright (c) " before/after this line. @kaikreuzer, here I would need your opinion/advise.

In my last commit I tried to work around this by preserving the entire header:

/**
 * Copyright (c) 2010-2022 Contributors to the openHAB project
 *
 * See the NOTICE file(s) distributed with this work for additional
 * information.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0
 *
 * SPDX-License-Identifier: EPL-2.0
 */

/**
 * Copyright (c) 2021-2022 Contributors to the SmartHome/J project
 *
 * See the NOTICE file(s) distributed with this work for additional
 * information.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0
 *
 * SPDX-License-Identifier: EPL-2.0
 */

I did a small hack though by removing the openHAB line from the header I copied from SmartHome/J to not have it duplicated. So perhaps more correct would be:

/**
 * Copyright (c) 2010-2022 Contributors to the openHAB project
 *
 * See the NOTICE file(s) distributed with this work for additional
 * information.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0
 *
 * SPDX-License-Identifier: EPL-2.0
 */

/**
 * Copyright (c) 2010-2021 Contributors to the openHAB project
 * Copyright (c) 2021-2022 Contributors to the SmartHome/J project
 *
 * See the NOTICE file(s) distributed with this work for additional
 * information.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0
 *
 * SPDX-License-Identifier: EPL-2.0
 */

although this would be even more redundant. Here I would need your advise @J-N-K, what is acceptable to you?

I would really prefer not having to move files to src/3rdparty since the whole exercise is about reducing differences between the two projects, and that would be counter-productive in this regard.

Additionally, I would need to know, assuming one of the above solutions is acceptable, if I would need the two full copyright headers in all files where I picked changes, also the files originating from the openHAB repository? For example, if a single line in an existing file was backported.

Sorry for being so verbose and detail-oriented, but since this is legal stuff, we probably need to get it right. For your information, it seems to have been handled a bit more casually here: #12568

@jlaur jlaur force-pushed the 14124-deconz-actions branch from 2536a27 to 01b76d1 Compare January 2, 2023 01:28
@kaikreuzer
Copy link
Member

kaikreuzer commented Jan 6, 2023

@kaikreuzer, here I would need your opinion/advise.

@jlaur For all openHAB code we are using the same standard license header and nothing else to keep the management simple.
Everything else would have to go into 3rdparty.

For the PR at hand, I would say there can be a very simple solution. As you state in the initial comment this PR is "Originally contributed by @J-N-K" - so all we need is @J-N-K's ok that this is a contribution to the openHAB project and we can go with the standard license header as "Contributors to the openHAB project" would correctly describe it. The PR should then carry the "Also-By"-line to show that it is a joint contribution.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 7, 2023

Here is how the doc looks like in Github. Is it "normal" ?

image

@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

Here is how the doc looks like in Github. Is it "normal" ?

image

Yes, see for example https://raw.githubusercontent.com/openhab/openhab-addons/main/bundles/org.openhab.binding.omnilink/README.md (chapter "Rule Actions") and see here how it's rendered:

https://www.openhab.org/addons/bindings/omnilink/#rule-actions

jlaur added 9 commits January 7, 2023 12:47
Backported from SmartHome/J

Also-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the 14124-deconz-actions branch from 078d39f to 5022dd1 Compare January 7, 2023 11:48
@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

For the PR at hand, I would say there can be a very simple solution. As you state in the initial comment this PR is "Originally contributed by @J-N-K" - so all we need is @J-N-K's ok that this is a contribution to the openHAB project and we can go with the standard license header as "Contributors to the openHAB project" would correctly describe it.

@J-N-K - would this be acceptable to you?

The PR should then carry the "Also-By"-line to show that it is a joint contribution.

I have added this to the first commit, and will emphasize that the maintainer merging this PR should preserve this in the final commit message.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 7, 2023

Yes, see for example

You mean it does not render properly in github but we do not need to care about that ?

@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

Yes, see for example

You mean it does not render properly in github but we do not need to care about that ?

Yes, it should be rendered correctly when the docs are generated. Unfortunately there is no way of testing this prior to merging, AFAIK.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

LGTM, thank you

Please wait with merging until the license header question has come to a conclusion.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

@lolodomo - where can I see the provided i18n labels and descriptions? I was testing the actions from DSL rules, but can't seem to find my way into this stuff from Main UI.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 7, 2023

@lolodomo - where can I see the provided i18n labels and descriptions? I was testing the actions from DSL rules, but can't seem to find my way into this stuff from Main UI.

Good question. I don't know, I am not using MainUI for all rule stuff.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

@lolodomo - where can I see the provided i18n labels and descriptions? I was testing the actions from DSL rules, but can't seem to find my way into this stuff from Main UI.

Good question. I don't know, I am not using MainUI for all rule stuff.

Me neither, but looking for them in my text rules would make no sense. :-) They must be visible somewhere - otherwise, what's the point of the labels, descriptions and translations?

@lolodomo
Copy link
Contributor

lolodomo commented Jan 7, 2023

They must be visible somewhere - otherwise, what's the point of the labels, descriptions and translations?

@ghys should certainly be able to tell us where to find thing actions in MainUI.

@J-N-K
Copy link
Member

J-N-K commented Jan 7, 2023

I think they are currently not available due to openhab/openhab-core#1745

@ghys
Copy link
Member

ghys commented Jan 7, 2023

to be honest I've never come across them, they don't appear in the API. I only know they exist because I've written blockly libraries and published them to the marketplace. It's a rule-only affair for now, there is no API to access them.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 7, 2023

There is probably an API now with this recent merge?
openhab/openhab-core#2810

@J-N-K
Copy link
Member

J-N-K commented Jan 7, 2023

To call them via REST API, but configuring them for rules is still not possible.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

@J-N-K - now that you are here, would you care to comment on #14134 (comment)? 🙂 Many thanks in advance.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 7, 2023

@lolodomo - with the changes you requested regarding I18N, I have now tested what I could:

[
  {
    "actionUid": "deconz.permitJoin",
    "label": "@text/action.permit-join-network.label",
    "description": "@text/action.permit-join-network.description",
    "inputs": [
      {
        "name": "duration",
        "type": "java.lang.Integer",
        "label": "@text/action.permit-join-network.duration.label",
        "description": "@text/action.permit-join-network.duration.description",
        "required": false,
        "tags": [],
        "reference": "",
        "defaultValue": ""
      }
    ],
    "outputs": []
  }
]

I don't know how/if this will eventually work, but the raw strings with placeholders are returned without substitutions, e.g. "@text/action.permit-join-network.label".

@J-N-K
Copy link
Member

J-N-K commented Jan 7, 2023

Probably a similar issue to openhab/openhab-core#2546

@J-N-K
Copy link
Member

J-N-K commented Jan 8, 2023

@jlaur I think I made my opinion on that clear here. That code was deliberately not contributed to openHAB and since the underlying issues still have not been solved I don't not see any reason to do so. From my POV it would be more improvement for the user if deconz was dropped from openHAB addons distribution as one of the biggest improvements in the SmartHome/J (updating things without deleting them) can't be back ported.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 8, 2023

@lolodomo - with the changes you requested regarding I18N, I have now tested what I could:

[
  {
    "actionUid": "deconz.permitJoin",
    "label": "@text/action.permit-join-network.label",
    "description": "@text/action.permit-join-network.description",
    "inputs": [
      {
        "name": "duration",
        "type": "java.lang.Integer",
        "label": "@text/action.permit-join-network.duration.label",
        "description": "@text/action.permit-join-network.duration.description",
        "required": false,
        "tags": [],
        "reference": "",
        "defaultValue": ""
      }
    ],
    "outputs": []
  }
]

I don't know how/if this will eventually work, but the raw strings with placeholders are returned without substitutions, e.g. "@text/action.permit-join-network.label".

New recently added API was apparently not properly implemented (because ignoring translation stuff). A new issue has to be created in core and to be fixed before OH 4 is released (and before the new API starts to be used).
I just checked the channel-type REST API in API explorer and it properly returned translated label/description in French for example.
As you know, I can't help before a certain time as my dev env is still Java 11.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 8, 2023

I think I made my opinion on that clear here.

True, but you did not comment on my proposal of a third option here. Would two headers (SmartHome/J + openHAB) for new files and standard openHAB header for old files be acceptable to all, @J-N-K and @kaikreuzer? See the added files in this PR.

That code was deliberately not contributed to openHAB and since the underlying issues still have not been solved I don't not see any reason to do so. From my POV it would be more improvement for the user if deconz was dropped from openHAB addons distribution as one of the biggest improvements in the SmartHome/J (updating things without deleting them) can't be back ported.

Actually that could be backported as well, but I agree that it shouldn't and should be fixed in core instead. However, following that logic any binding could be forked to SmartHome/J and support smooth migration and then should be deleted from this repository. I'm not sure this is the way to go.

I'm not sure where this will end, but at worst I (and reviewers) would have wasted some time backporting some enhancements and fixes until some broader decision is taken. Reducing the differences between the two binding versions wouldn't be a bad thing though, as it would ease the merge that might happen one day, whether that would be in SmartHome/J or in openHAB ultimately. I'm even considering porting changes the other way as well for the same reason - to reduce differences.

@J-N-K
Copy link
Member

J-N-K commented Jan 10, 2023

Please see my comment. To prevent unnecessary work I would suggest to postpone this PR.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur
Copy link
Contributor Author

jlaur commented Mar 7, 2023

@J-N-K - how should we proceed? Accept this one as first step, or close it and await full backport?

J-N-K added a commit to J-N-K/openhab-addons that referenced this pull request Mar 16, 2023
Signed-off-by: Jan N. Klug <github@klug.nrw>
@jlaur
Copy link
Contributor Author

jlaur commented Mar 18, 2023

Superseded by #14622.

@jlaur jlaur closed this Mar 18, 2023
jlaur pushed a commit that referenced this pull request Mar 18, 2023
)

* port changes
* update instructions
* Incorporate review comments from #14134
* new improvements (mostly Java 17 changes)
* further improvements

Signed-off-by: Jan N. Klug <github@klug.nrw>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
…nhab#14622)

* port changes
* update instructions
* Incorporate review comments from openhab#14134
* new improvements (mostly Java 17 changes)
* further improvements

Signed-off-by: Jan N. Klug <github@klug.nrw>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
…nhab#14622)

* port changes
* update instructions
* Incorporate review comments from openhab#14134
* new improvements (mostly Java 17 changes)
* further improvements

Signed-off-by: Jan N. Klug <github@klug.nrw>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants