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

Adding "category" property to things #3467

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clinique
Copy link
Contributor

@clinique clinique commented Mar 20, 2023

Following the suggestion I made here.

Thing-Type and Thing XML schemas where already holding a category field.
This adds the ability to add it in .things files, with the same syntax than .items file (eg:

magic:color-light:magicColor @"theplace" <lightbulb>

I'm missing the ability to inherit thing category from thing-type category (did not find where it happens).

Using the API explorer, I get visibility on category as a field of the thing. If this gets approved (and probably enhanced), it could then be added in web-ui.

@clinique clinique requested a review from a team as a code owner March 20, 2023 13:05
@lolodomo
Copy link
Contributor

We are currently refactoring/extending what is a category for an item, so I would prefer to wait for the change for items.
I already started.

@clinique
Copy link
Contributor Author

We are currently refactoring/extending what is a category for an item, so I would prefer to wait for the change for items. I already started.

Sure, there is no hurry until final OH4 :)
Where is your work around items ?

@lolodomo
Copy link
Contributor

lolodomo commented Mar 25, 2023

Where is your work around items ?

In fact, my plans were to do it in two steps: first step was to update the icon attribute in sitemap (PR #3378). Second step was to apply a similar change to item category.
I am still fighting with the way to update the sitemap syntax. I will try again during the weekend.

@lolodomo
Copy link
Contributor

The thing icon will then be used only when setting up the system, e.g. in the page showing all the things ?
I am just not sure it is worth the effort ?

@clinique
Copy link
Contributor Author

The thing icon will then be used only when setting up the system, e.g. in the page showing all the things ? I am just not sure it is worth the effort ?

As said in the comment in the wishlist, it's a start to make the UI more fancy. Bindings also has icons that are only displayed in documentation. The provided icon could be used for things representing the group. I also would like to think on how could binding provide additional icon sets to streamline the user experience when creating items.

@lolodomo
Copy link
Contributor

I also would like to think on how could binding provide additional icon sets to streamline the user experience when creating items.

I believe it should not be too much difficult. The binding should implement an IconProvider with the binding id as icon set id and that icon provider would then search icons in binding folder "resources/OH-INF/icon". Then, the icon could be acceded by using "oh:bindingId:icon", for example "oh:netatmo:welcome_camera" assuming you provided the icon "welcome_camera.svg" or "welcome_camera.png".
The only difficulty I can guess is how to define the icon provider priority.
You can look at class ClassicIconProvider.

@clinique
Copy link
Contributor Author

Yes, I know about the IconProvider but I'm not sure it's the correct approach if we want to promote it accross all bindings. That would mean one in each binding ? It then should be good to add it in the binding skeleton.

@lolodomo
Copy link
Contributor

lolodomo commented Apr 14, 2023

By the way, your idea to have a binding providing a dedicated icons is a really good idea. I remember we adviced to download few netatmo icons and create custom icons with that, for the wifi status for example. This could be simplified if the binding was providing these icons.
No change in core framework is required, this can already be implemented in any binding.
These icons will then be usable by the user and will render properly in Basic UI and in the Android app, they will also in MainUI after a bug is fixed.

@lolodomo
Copy link
Contributor

Yes, I know about the IconProvider but I'm not sure it's the correct approach if we want to promote it accross all bindings. That would mean one in each binding ? It then should be good to add it in the binding skeleton.

We have a discovery service in most of bindings, so why not an icon service.
I just imagine it will be less commonly used by developers than discovery services. So maybe not necessarily needed to include that in the binding skeleton.

@clinique
Copy link
Contributor Author

Yes, I'm doing a POC with the MeteoAlerte binding.

@J-N-K
Copy link
Member

J-N-K commented Apr 14, 2023

If it turns out that a lot of code is shared for every IconProvider we can also add that to an AbstractIconProvider in core so that the code needed in the add-ons is very small (probably only namespace and location of the items, if they are included in the bundle).

@lolodomo
Copy link
Contributor

There is already an abstract class on core.

@clinique
Copy link
Contributor Author

clinique commented Apr 15, 2023

Pushed PR here .
BTW I saw that the "IconSet" in core is a perfect candidate to become a record (note for later).

@J-N-K
Copy link
Member

J-N-K commented Apr 20, 2023

One problem with record is that our current GSON is not able to handle them. We have to find a solution for stream first (see #3321)

@J-N-K
Copy link
Member

J-N-K commented Apr 20, 2023

Do we still need these thing categories when we have custom iconsets and icon providers in add-ons?

@clinique
Copy link
Contributor Author

Yes, delivering on purpose iconset providers give sense to this PR. I just want to be active on an idea I proposed in the wishlist.

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.

3 participants