-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
AddonInfo extensions #3865
AddonInfo extensions #3865
Conversation
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@mherwege ping. |
@andrewfg Based on the discussion in the addon PR, I think you can ommit the AddonInfoProviderInstaller. But you should bring most of the code from the addon PR in here, i.e. read an |
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@J-N-K this PR is now fully aligned with the following..
Done. |
@J-N-K @mherwege @kaikreuzer just so you know: I have tested this PR by manually putting an 'addons.xml' file in the '/userdata/addons' folder, and I can confirm that its respective AddonInfoProvider does indeed load and parse the file and produce the necessary AddonInfo results. And furthermore when my AddonSuggestionFinderService component is instantiated and queried from the Rest API, it indeed finds all thirteen of my addons that I would expect it to find..
|
@@ -24,16 +24,16 @@ | |||
<attribute name="annotationpath" value="target/dependency"/> | |||
</attributes> | |||
</classpathentry> | |||
<classpathentry kind="src" output="target/test-classes" path="src/test/java"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm. Not sure. I think Eclipse did it automagically when I wrote a new JUnit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you simply revert the changes so that this PR leaves the file untouched?
So I did a full build of everything incorporating both of my PRs in openhab-core, plus my PR in openhab-addons plus both of @J-N-K 's PRs in openhab-addons and openhab-distro, plus the PR of @mherwege in openhab-webui; (it took absolutely ages on my (old) PC); however .. it works!! Following is a screenshot from the OH startup screen .. |
....core.config.core/src/main/java/org/openhab/core/config/core/xml/util/XmlDocumentReader.java
Outdated
Show resolved
Hide resolved
...penhab.core.addon/src/main/java/org/openhab/core/addon/infoproviders/AddonsInfoProvider.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andrew Fiddian-Green <[email protected]>
LGTM |
@andrewfg Does it depend on it, or is it a pre-requisite? I believe it is just a pre-requisite. Apart from being a pre-requisite, the big added value of this PR (combined with openhab/openhab-addons#15870 and openhab/openhab-distro#1604) is that it makes all distribution add-on info available before installation of the add-on. This potentially allows extending the UI to filter on more add-on attributes. @openhab/maintainers This PR and the 2 related ones are a foundation for making add-on selection easier or automatic. They don't do it by themselves, but collect all the necessary information. There is no code in here that would start discovery processes. Would it be possible to review and merge these as a start? |
It is indeed only a pre-requisite. |
@kaikreuzer I appreciate your support. This PR is only a part step, and #3806 contains the full set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
I have only a few minor comments/questions, see below.
bundles/org.openhab.core.addon/src/main/java/org/openhab/core/addon/AddonInfoListReader.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core.addon/src/main/java/org/openhab/core/addon/AddonInfoListReader.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core.addon/src/main/java/org/openhab/core/addon/AddonInfoRegistry.java
Outdated
Show resolved
Hide resolved
* | ||
* SPDX-License-Identifier: EPL-2.0 | ||
*/ | ||
package org.openhab.core.addon.infoproviders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please always use singular for package names.
But does this really need a separate package? And does it have to be a public one or could it be moved to org.openhab.core.addon.internal.xml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...penhab.core.addon/src/main/java/org/openhab/core/addon/infoproviders/AddonsInfoProvider.java
Outdated
Show resolved
Hide resolved
...penhab.core.addon/src/main/java/org/openhab/core/addon/infoproviders/AddonsInfoProvider.java
Outdated
Show resolved
Hide resolved
...penhab.core.addon/src/main/java/org/openhab/core/addon/infoproviders/AddonsInfoProvider.java
Outdated
Show resolved
Hide resolved
...penhab.core.addon/src/main/java/org/openhab/core/addon/infoproviders/AddonsInfoProvider.java
Outdated
Show resolved
Hide resolved
@@ -24,16 +24,16 @@ | |||
<attribute name="annotationpath" value="target/dependency"/> | |||
</attributes> | |||
</classpathentry> | |||
<classpathentry kind="src" output="target/test-classes" path="src/test/java"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you simply revert the changes so that this PR leaves the file untouched?
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the prompt updates!
FTR: I have deployed the updated XSDs on our website, see openhab/website@74e3185. |
@andrewfg @kaikreuzer Thank you for the work and the review. |
@mherwege and thank you too!! |
@kaikreuzer Cool. Thank you. I have openhab/openhab-addons#15817 building now. The first few bindings already passed the schema verification. :) EDIT: they all passed the verification :) |
This PR contains core extensions to the AddonInfo class as foundation work to in order to support the discovery of addons to be suggested to be installed on setup.
Changes in AddonInfo
List<AddonInfo>
List<AddonInfo>
DTO.Other Changes in this PR
addon.xml
information for all addons which it gets by reading all*.xml
files which will have been installed in the/userdata/adddons/
folder.Depends on openhab/openhab-addons#15780
Pre requisite for #3806
PR with documentation changes openhab/openhab-docs#2150
Signed-off-by: Andrew Fiddian-Green [email protected]