-
-
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
[marketplace] Add a 3rd Party marketplace #2592
Conversation
This adds a new marketplace addon provider which allows to add marketplaces maintained by 3rd party organizations (similar to alternative app stores on your smartphone). The configuration allows one or multiple marketplaces which provide their addons via JSON files (instead of a topic in the community forum). Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
I wish this will be accepted. |
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.
I am fine with this new service. From my pov we should call is just JsonAddonService
without the phrase "Marketplace".
private static final Map<String, AddonType> TAG_ADDON_TYPE_MAP = Map.of( // | ||
"automation", new AddonType("automation", "Automation"), // | ||
"binding", new AddonType("binding", "Bindings"), // | ||
"misc", new AddonType("misc", "Misc"), // | ||
"persistence", new AddonType("persistence", "Persistence"), // | ||
"transformation", new AddonType("transformation", "Transformations"), // | ||
"ui", new AddonType("ui", "User Interfaces"), // | ||
"voice", new AddonType("voice", "Voice")); |
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.
Can this be refactored to be a global constant for all AddonService
s? Or do we expect different capabillties of those?
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.
Easy, but the question is what the correct place would be. In general an AddonService
is not required to supply all AddonType
s, however each AddonService
could pick what is available. We do not have an AddonConstants
class or something similar which would be a good fit. Another solution would be to make the map public
in the CommunityMarketplaceAddonService
but that would mix different AddonServices which should IMO be avoided.
I hate duplicated code, but IMO here it seems the best solution.
.../main/java/org/openhab/core/addon/marketplace/internal/json/JsonMarketplaceAddonService.java
Outdated
Show resolved
Hide resolved
...arketplace/src/main/java/org/openhab/core/addon/marketplace/internal/json/AddonEntryDTO.java
Outdated
Show resolved
Hide resolved
.../main/java/org/openhab/core/addon/marketplace/internal/json/JsonMarketplaceAddonService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@cweitkamp Thanks for the review. I have addressed all but one comment (the refactoring of the duplicated map). |
.../main/java/org/openhab/core/addon/marketplace/internal/json/JsonMarketplaceAddonService.java
Outdated
Show resolved
Hide resolved
.../main/java/org/openhab/core/addon/marketplace/internal/json/JsonMarketplaceAddonService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
*/ | ||
@Component(immediate = true, configurationPid = "org.openhab.core.marketplace.json", // | ||
property = Constants.SERVICE_PID + "=org.openhab.core.marketplace.json") | ||
@ConfigurableService(category = "system", label = "JSON 3rd Party Addon Service", description_uri = JsonAddonService.CONFIG_URI) |
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.
From an UX perspective - it's the 3rd configurable service related to add-ons that will appear under "Settings > System Services" with barely any parameter which is beginning to become a lot and quite messy. If anything we should really think about reducing this proliferation and merging related configurations (I've talked about this before).
- Add-on Management + Community Marketplace + JSON 3rd Party Addon Service (also note the difference in spelling of Add-on/Addon + JSON/Json in the existing "Json Storage"!) => Add-on Management
- Regional Settings + Ephemeris => Regional Settings
- Audio + Voice => Audio & Voice
- Maybe also Sitemap + Charts => ?
Maybe have configurable services for the sole purposes of providing the configuration and have the actual services inject them?
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.
I thought about that, too. The problem is that we - at least I know of no way to do it - can't group in the UI within a category. If we merge everything under e.g. system:addon
(which in general would be preferred IMO), it's not easily visible to the user which parameter affects which service.
I'll fix the spelling.
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.
Would it help to introduce another level in der config description URI? E.g. system:addon:management
, system:addon:marketplace
and system:addon:json
? The UI could then group by the second level (if present).
The good thing to this approach is that it is fully backward compatible since the service PID stays the same and therefore the configuration is also used after the config description URI changed. This also leaves us time to decide what the best approach is, since it would not create a problem if it is changed after the release.
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
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 agree on the grouping of settings in the UI. This PR does not need to wait for a solution for that as we obviously have to change other ConfigurableService
s too to reach that goal.
* [marketplace] add a 3rd Party compatible marketplace This adds a new marketplace addon provider which allows to add marketplaces maintained by 3rd party organizations (similar to alternative app stores on your smartphone). The configuration allows one or multiple marketplaces which provide their addons via JSON files (instead of a topic in the community forum). Signed-off-by: Jan N. Klug <jan.n.klug@rub.de> GitOrigin-RevId: 8a29b07
This adds a new
AddonService
implementation that allows to add marketplaces maintained by 3rd party individuals (similar to alternative app stores on the smartphone).The configuration allows one or multiple marketplaces which provide their addons via JSON files (instead of a topic in the community forum). Installation is done by re-using the already present .jar, .kar, .json, .yaml bundle installers (same as community marketplace).
The JSON referenced by the URL encapsulates an array of one or more addon definitions like
This could also be used to distribute version-specific (by using different JSON files) bindings with back ported feature or bugfixes.
Signed-off-by: Jan N. Klug jan.n.klug@rub.de