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

Fix addon name localisation before installation #3908

Merged
merged 2 commits into from
Dec 9, 2023

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Dec 9, 2023

Fixes openhab/openhab-webui#2204
Replaces #3905

The issue is that no addon info is available for distribution addons before installation. The only thing available is the feature description and the addon endpoint gets it from the addon data, not the addonInfo data. So a merge of addonInfo is not even happening.
This change introduces a flag in AddonInfo indicating if it is the master info or not. When the info is coming from the addon repository scan, it is flagged to not be the master data and name and description will be ignored (because localization info is not available). Addon services will check if the addon info is master info, and only then update name and description.

@mherwege mherwege requested a review from a team as a code owner December 9, 2023 17:38
@mherwege
Copy link
Contributor Author

mherwege commented Dec 9, 2023

@andrewfg FYI
@kaikreuzer I think this should solve the isse.

@andrewfg
Copy link
Contributor

andrewfg commented Dec 9, 2023

I think this should solve the issue.

@mherwege / @kaikreuzer shall I do my usual full build clean install test before you merge it? i.e. tomorrow.

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.

Thanks, let's have a try with this.

@kaikreuzer kaikreuzer merged commit 708a954 into openhab:main Dec 9, 2023
2 checks passed
@kaikreuzer kaikreuzer added this to the 4.1 milestone Dec 9, 2023
@andrewfg
Copy link
Contributor

andrewfg commented Dec 9, 2023

let's have a try with this.

@kaikreuzer / @mherwege all Ok; it works on my full build.

@jimtng
Copy link
Contributor

jimtng commented Jan 20, 2025

@mherwege should we just revert this instead of #4572? isMasterAddon was added for the sole purpose that #4572 is removing. The only difference is the NA that would also get restored by reverting this.

Alternatively we could revert this, and then remove the NA stuff?

@mherwege
Copy link
Contributor Author

mherwege commented Jan 21, 2025

@jimtng Indeed, I think this should be reverted. I don't think there is any harm in having the NA treatment back, and that keeps us on the safe side. It might be needed when loading if all providers are not available at the same time. I am not sure about that, so prefer keeping it.

If we revert this, is there a need to update the developer documentation to point out the name and description are required and they should be provided in full in the addon.xml? Reviewer instructions?

@jimtng
Copy link
Contributor

jimtng commented Jan 21, 2025

If we revert this, is there a need to update the developer documentation to point out the name and description are required and they should be provided in full in the addon.xml? Reviewer instructions?

This one needs to be updated:
https://www.openhab.org/docs/developer/utils/i18n.html#using-custom-keys

@jimtng
Copy link
Contributor

jimtng commented Jan 21, 2025

@mherwege I'll prepare a PR to revert this

jimtng added a commit to jimtng/openhab-core that referenced this pull request Jan 21, 2025
This reverts commit 708a954
but with some modifications

Signed-off-by: Jimmy Tanagra <[email protected]>
@jimtng
Copy link
Contributor

jimtng commented Jan 21, 2025

#4573

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial setup: @text/xxx instead of binding name
4 participants