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

[sitemap] Provide information about widget label source to clients #3804

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

maniac103
Copy link
Contributor

The label can be populated from a label specified on the respective sitemap widget, or (if no label was specified) from the label of the backing item. Allow clients to differentiate between both cases. This allows clients (BasicUI, mobile apps) to e.g. show the label for charts if they were specified on the sitemap widget and to hide it when it just was populated from the item label.

Related to openhab/openhab-webui#2065

@maniac103 maniac103 requested a review from a team as a code owner September 20, 2023 09:23
@maniac103
Copy link
Contributor Author

Open question: How can I actually test this in a development environment, IOW, how can I run the locally built core classes? There's documentation on how to run locally built bindings, but I haven't found the same (or generally, any kind of development documentation) for -core. Did I miss something?

Comment on lines 25 to 33
public static WidgetLabelSource forWidget(Widget w, String label) {
if (label == null || label.isEmpty()) {
return None;
} else if (w.getLabel() != null) {
return SitemapDefinition;
} else {
return ItemLabel;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In case the label is set to "[%s]" for example, either on sitemap element or on item, your method will return SitemapDefinition or ItemLabel, but not None. Is it really what you expect ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. After all, there is a label to work with, even if it almost only consists of the item state. I introduced NONE only because if there's no label at all, neither ITEM nor SITEMAP do really fit.

Comment on lines 21 to 23
SitemapDefinition,
ItemLabel,
None;
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention is rather to use uppercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@lolodomo
Copy link
Contributor

You probably forgot one origin: the linked channel.

@maniac103 maniac103 force-pushed the sitemap-widget-label-source branch from d8ce4c5 to b20d2f8 Compare September 25, 2023 09:45
@maniac103
Copy link
Contributor Author

You probably forgot one origin: the linked channel.

As far as I can tell from the code there's only the widget, the item label and the item name which the label can be taken from. I've forgotten the latter, so I completely revamped the approach to also cover that one.
Or do you by 'linked channel' refer to this piece of the logic? If so, IMO it's fine to group it under 'item label' as well, since it doesn't really matter where exactly the item label is fetched from.

The label can be populated from a label specified on the respective
sitemap widget, or (if no label was specified) from the label of the
backing item. Allow clients to differentiate between both cases.

Related to openhab/openhab-webui#2065

Signed-off-by: Danny Baumann <dannybaumann@web.de>
@maniac103 maniac103 force-pushed the sitemap-widget-label-source branch from d93cebb to b3c7734 Compare October 17, 2023 09:47
@maniac103
Copy link
Contributor Author

The force-push to b3c7734 just rebases onto main; its patch set should be identical to d93cebb.

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.

Ok for me. Just one comment to be added to clearly explain what means NONE source. Because NONE covers two different things in your implementation.

Signed-off-by: Danny Baumann <dannybaumann@web.de>
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.

Looks good to me.

Signed-off-by: Danny Baumann <dannybaumann@web.de>
@maniac103
Copy link
Contributor Author

Last commit just added some documentation for the enum values.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 3, 2023

@openhab/core-maintainers : is it possible to review and merge this PR ? I have already done a full pre-review.
@maniac103 is waiting for it for the Android app.

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.

lgtm, thanks!

@kaikreuzer kaikreuzer merged commit fb6f192 into openhab:main Nov 7, 2023
@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature of the Core label Nov 7, 2023
@kaikreuzer kaikreuzer added this to the 4.1 milestone Nov 7, 2023
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 of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants