[Meteoalerte] Add an IconProvider #14811

clinique · 2023-04-15T07:14:57Z

This PR adds an icon provider to MeteoAlerte binding so svg icon of weather alert are served by the binding.
Served icons are also dynamically colored based on the alert level.

Signed-off-by: clinique <[email protected]>

clinique · 2023-04-15T07:32:09Z

Icons are automatically provided :

and they are dynamic :

Signed-off-by: clinique <[email protected]>

lolodomo · 2023-04-15T08:25:11Z

Looking at the captures, it looks like you used a grey as fill color. It will lead to very poor contrast in dark mode.
Please leave currentColor, the icon will then adapt itself to bright or dark themes.

Providing colored icons based on the item state is certainly adapted in that case.

lolodomo · 2023-04-15T08:28:19Z

Providing colored icons based on the item state is certainly adapted in that case.

But this is apparently not the way you did it.

lolodomo · 2023-04-15T08:33:03Z

You do not use AbstractResourceIconProvider?

...lerte/src/main/java/org/openhab/binding/meteoalerte/internal/handler/MeteoAlerteHandler.java

...teoalerte/src/main/java/org/openhab/binding/meteoalerte/internal/MeteoAlertIconProvider.java

...lerte/src/main/java/org/openhab/binding/meteoalerte/internal/handler/MeteoAlerteHandler.java

...teoalerte/src/main/java/org/openhab/binding/meteoalerte/internal/MeteoAlertIconProvider.java

lolodomo · 2023-04-15T09:47:17Z

bundles/org.openhab.binding.meteoalerte/src/main/resources/OH-INF/thing/thing-types.xml

-			<channel id="avalanches" typeId="alert-level">
-				<label>Etat Avalanches</label>
-			</channel>
+			<channel id="vague-submersion" typeId="vague-submersion"/>


Migration instructions would be appreciated as you changed all thing types.

I neutralized the update instructions currently because I'm facing the same error than reported here.

bundles/org.openhab.binding.meteoalerte/src/main/resources/OH-INF/thing/thing-types.xml

clinique · 2023-04-15T16:10:29Z

Looking at the captures, it looks like you used a grey as fill color. It will lead to very poor contrast in dark mode. Please leave currentColor, the icon will then adapt itself to bright or dark themes.

Providing colored icons based on the item state is certainly adapted in that case.

I have tested in black theme and white theme. The gray icon does the job.

clinique · 2023-04-15T16:42:02Z

I'm going to push an update with many of your observations addressed but I still have some cleansing to do.

Signed-off-by: clinique <[email protected]>

…he binding. Will investigate. Signed-off-by: clinique <[email protected]>

lolodomo · 2023-04-15T18:00:36Z

I really like the idea to provide default icon for very specific channels when there is no adapted icon already present in the classic icon set.

Signed-off-by: clinique <[email protected]>

clinique · 2023-04-17T14:08:30Z

New icon set :

The grey circle changes upon item state :

Signed-off-by: clinique <[email protected]>

Remove hyphen replacement by underscores. Signed-off-by: clinique <[email protected]>

...teoalerte/src/main/java/org/openhab/binding/meteoalerte/internal/MeteoAlertIconProvider.java

Signed-off-by: clinique <[email protected]>

lolodomo · 2023-04-23T09:18:40Z

Ok, we have now only 3 opened comments:

the icon priority that should be set to 0 IMHO
the migration stuff : it looks like you encountered a problem ? Did you test it ? Do you need an additional test by someone else ?
You said you adjusted the orange but I can't find in which commit ?

clinique · 2023-04-23T09:35:04Z

Ok, we have now only 3 opened comments:

the icon priority that should be set to 0 IMHO

You're correct, now that the iconset is filtered on the binding_id it makes fully sense.

the migration stuff : it looks like you encountered a problem ? Did you test it ? Do you need an additional test by someone else ?

Yes, as soon as it is renamed to "xml", the launch in eclipse fails and I'm not able to test. Apparently this happens only with eclipse due to a "sombre histoire de dépendances". I'll rename it, you'll tell me if you experiment the same.

You said you adjusted the orange but I can't find in which commit ?

Sure I did it but can not find it neither. I'm experiencing issues with dying eclipse since I updated to Ubuntu 23.04 so mixing between eclipse and vscode. It's tough today.

clinique · 2023-04-23T10:49:03Z

I changed to colors according to this template :

I tried using the colors proposed by OpenUV but I see them not being flashy enough.

Signed-off-by: clinique <[email protected]>

lolodomo · 2023-04-23T11:37:05Z

I will wait for the merge of openhab/openhab-webui#1849 first.

lolodomo · 2023-04-23T18:54:22Z

Thing migration tested (for the first time for me) and the results are mixed.
I encounter first a thing status :NOT_YET_READY and later a warning about channel types or config descriptions for thing missing in registry and finally a warning about failing normalize configuration :

20:13:32.967 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from UNINITIALIZED (HANDLER_MISSING_ERROR): Handler factory not found to UNINITIALIZED (NOT_YET_READY)
...
20:15:33.465 [WARN ] [.core.thing.internal.ThingManagerImpl] - Channel types or config descriptions for thing 'meteoalerte:department:b1daefd4dc' are missing in the respective registry for more than 120s. This should be fixed in the binding.
20:15:33.518 [INFO ] [.core.thing.internal.ThingManagerImpl] - Updating 'meteoalerte:department:b1daefd4dc' from version 0 to 1
20:15:33.554 [WARN ] [.core.thing.internal.ThingManagerImpl] - Failed to normalize configuration for thing 'meteoalerte:department:b1daefd4dc': {thing/channel=Type description for {0} not found although we checked the presence before.}
20:15:33.676 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from UNINITIALIZED (NOT_YET_READY) to INITIALIZING
20:15:33.694 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from INITIALIZING to UNKNOWN
20:15:35.855 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from UNKNOWN to ONLINE
20:15:35.941 [INFO ] [openhab.event.ItemStateChangedEvent  ] - Item 'Alertes_Departement_Icone_Pluie_Inondation' changed from NULL to raw type (image/svg+xml): 9062 bytes
20:15:35.954 [INFO ] [openhab.event.ItemStateChangedEvent  ] - Item 'Alertes_Departement_Etat_Canicule' changed from NULL to 0
20:15:35.968 [INFO ] [openhab.event.ItemStateChangedEvent  ] - Item 'Alertes_Departement_Etat_Grand_Froid' changed from NULL to 0

@J-N-K (as our expert in thing migration): is all this normal?

Finally the thing in userdata/jsondb/org.openhab.core.thing.Thing.json looks updated with the new channel types UID. But icons are not displayed in MainUI ! And something very important and certainly a bug, "itemType" is now missing for the migrated channels. MainUI considers switch as default.

If I restart the bundle, I still have first the NOT_YET_READY status:

20:33:07.259 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from UNINITIALIZED (HANDLER_MISSING_ERROR) to UNINITIALIZED (NOT_YET_READY)
20:33:07.915 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from UNINITIALIZED (NOT_YET_READY) to INITIALIZING
20:33:07.933 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from INITIALIZING to UNKNOWN
20:33:09.905 [INFO ] [hab.event.ThingStatusInfoChangedEvent] - Thing 'meteoalerte:department:b1daefd4dc' changed from UNKNOWN to ONLINE

Icons still not displayed in MainUI.

If I restart OH, still no icon displayed in MainUI.
But I see now that my MainUI bundle was restored to the one from the distribution. I don't know at which step it occurred and I even do not understand why.
So I will have to restart all my migration tests.

As a temporary conclusion, the thing migration seems to be partially OK but itemType property is missing in channels after the migration leading to problems. I also encounter strange warnings and this NOT_YET_READY thing status.

lolodomo · 2023-04-23T19:00:20Z

Channel after migration:

        {
          "uid": "meteoalerte:department:b1daefd4dc:avalanches",
          "id": "avalanches",
          "channelTypeUID": "meteoalerte:avalanches",
          "kind": "STATE",
          "defaultTags": [],
          "properties": {},
          "configuration": {}
        }

And the same channel for a newly created thing using MainUI:

        {
          "uid": "meteoalerte:department:test:avalanches",
          "id": "avalanches",
          "channelTypeUID": "meteoalerte:avalanches",
          "itemType": "Number",
          "kind": "STATE",
          "label": "Avalanches",
          "description": "Niveau d\u0027alerte aux avalanches",
          "defaultTags": [],
          "properties": {},
          "configuration": {}
        },

Label and description fields are also missing after the migration.
Should itemType, label and description be added to the instructions XML ?

lolodomo · 2023-04-23T19:11:05Z

Orange is good now I believe.

lolodomo · 2023-04-23T19:12:05Z

Current status: only remaining one problem with the channels migration.

lolodomo · 2023-04-29T11:03:13Z

@clinique : with openhab/openhab-core#3576 the migration is now working well. The only problem is that if you had a channel linked to an item (so without category), the item will not inherit your new channel category after migration:

That means users will have to edit all their items to add the category.

I will merge this PR as soon as openhab/openhab-core#3576 and openhab/openhab-webui#1849 are merged.

lolodomo

LGTM, thank you

lolodomo · 2023-04-29T11:08:45Z

Just one thing: it would be cool if you could add a "Icon" chapter in the documentation, to list all the available icons provided by the binding.

clinique · 2023-04-29T11:47:39Z

Just one thing: it would be cool if you could add a "Icon" chapter in the documentation, to list all the available icons provided by the binding.

I'll put that on my todo list.

lolodomo · 2023-05-03T10:18:07Z

openhab/openhab-webui#1849 is now merged.
Still waiting for openhab/openhab-core#3576

lolodomo · 2023-05-07T19:20:49Z

Core PR has just been merged.

@clinique : do I wait for an improvement of the documentation (listing all available icons) ?

Signed-off-by: clinique <[email protected]>

clinique · 2023-05-08T14:50:06Z

@lolodomo : done

lolodomo

Thank you @clinique , just two remarks.

bundles/org.openhab.binding.meteoalerte/README.md

Signed-off-by: clinique <[email protected]>

lolodomo

LGTM, thank you Gael

* Solving activation / deactivation of IT4Wifi thing glitches. * Some code enhancements * Addition of an IconProvider --------- Signed-off-by: clinique <[email protected]> Signed-off-by: Thomas Burri <[email protected]>

* Solving activation / deactivation of IT4Wifi thing glitches. * Some code enhancements * Addition of an IconProvider --------- Signed-off-by: clinique <[email protected]> Signed-off-by: Matt Myers <[email protected]>

* Solving activation / deactivation of IT4Wifi thing glitches. * Some code enhancements * Addition of an IconProvider --------- Signed-off-by: clinique <[email protected]> Signed-off-by: Jørgen Austvik <[email protected]>

clinique added 3 commits April 13, 2023 16:24

Solving activation / deactivation of IT4Wifi thing glitches.

4127e33

Signed-off-by: clinique <[email protected]>

Some code enhancements

7eaad30

Signed-off-by: clinique <[email protected]>

Addition of an IconProvider

1696623

Signed-off-by: clinique <[email protected]>

clinique mentioned this pull request Apr 15, 2023

Adding "category" property to things openhab/openhab-core#3467

Closed

clinique added 2 commits April 15, 2023 09:22

Some more modifications

e2d3ef9

Signed-off-by: clinique <[email protected]>

Removing unexpected files

632f48b

Signed-off-by: clinique <[email protected]>

jlaur changed the title ~~[Meteoalerte] Adding an IconProvider~~ [Meteoalerte] Add an IconProvider Apr 15, 2023

Sat corrections

2ebc913

Signed-off-by: clinique <[email protected]>

clinique self-assigned this Apr 15, 2023

clinique added the enhancement An enhancement or new feature for an existing add-on label Apr 15, 2023

clinique requested review from lolodomo and J-N-K April 15, 2023 07:45

lolodomo requested changes Apr 15, 2023

View reviewed changes

clinique added 2 commits April 15, 2023 18:43

Code review #1

63cac20

Signed-off-by: clinique <[email protected]>

There is a problem with my instructions.xml, it break the launch of t…

3fbfadf

…he binding. Will investigate. Signed-off-by: clinique <[email protected]>

clinique added 2 commits April 17, 2023 13:16

Code cleansing

71e1636

Signed-off-by: clinique <[email protected]>

Icons updated to be inline with classic icon set.

734b1a4

Signed-off-by: clinique <[email protected]>

clinique added 2 commits April 18, 2023 01:10

Code cleansing

8ade7fb

Signed-off-by: clinique <[email protected]>

Adding the Meteo France icon.

208ccdd

Remove hyphen replacement by underscores. Signed-off-by: clinique <[email protected]>

clinique requested a review from lolodomo April 18, 2023 10:04

lolodomo mentioned this pull request Apr 19, 2023

Fix icons not displayed & not dynamic on all pages openhab/openhab-webui#1849

Merged

lolodomo requested changes Apr 19, 2023

View reviewed changes

...teoalerte/src/main/java/org/openhab/binding/meteoalerte/internal/MeteoAlertIconProvider.java Outdated Show resolved Hide resolved

...teoalerte/src/main/java/org/openhab/binding/meteoalerte/internal/MeteoAlertIconProvider.java Outdated Show resolved Hide resolved

Correcting leftovers

2d61ed4

Signed-off-by: clinique <[email protected]>

Corrections suggested by @lolodomo

e5550c7

Signed-off-by: clinique <[email protected]>

lolodomo added the awaiting other PR Depends on another PR label Apr 23, 2023

lolodomo mentioned this pull request Apr 23, 2023

Thing migration: missing fields in DB after updating a channel (changing its channel type) openhab/openhab-core#3574

Closed

lolodomo approved these changes Apr 29, 2023

View reviewed changes

lolodomo removed the awaiting other PR Depends on another PR label May 7, 2023

Adding the list of available icons

e1c21f5

Signed-off-by: clinique <[email protected]>

lolodomo requested changes May 8, 2023

View reviewed changes

bundles/org.openhab.binding.meteoalerte/README.md Outdated Show resolved Hide resolved

bundles/org.openhab.binding.meteoalerte/README.md Show resolved Hide resolved

Added vague-submersion and enhanced icon naming

17e1d3f

Signed-off-by: clinique <[email protected]>

lolodomo approved these changes May 8, 2023

View reviewed changes

lolodomo merged commit 0bd39a8 into openhab:main May 8, 2023

lolodomo added this to the 4.0 milestone May 8, 2023

clinique deleted the meteoalerte_iconserver branch May 8, 2023 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Meteoalerte] Add an IconProvider #14811

[Meteoalerte] Add an IconProvider #14811

clinique commented Apr 15, 2023

clinique commented Apr 15, 2023

lolodomo commented Apr 15, 2023

lolodomo commented Apr 15, 2023

lolodomo commented Apr 15, 2023

lolodomo Apr 15, 2023

clinique Apr 15, 2023

clinique Apr 17, 2023

clinique commented Apr 15, 2023 •

edited

Loading

clinique commented Apr 15, 2023

lolodomo commented Apr 15, 2023

clinique commented Apr 17, 2023 •

edited

Loading

lolodomo commented Apr 23, 2023 •

edited

Loading

clinique commented Apr 23, 2023

clinique commented Apr 23, 2023

lolodomo commented Apr 23, 2023 •

edited

Loading

lolodomo commented Apr 23, 2023

lolodomo commented Apr 23, 2023 •

edited

Loading

lolodomo commented Apr 23, 2023

lolodomo commented Apr 23, 2023

lolodomo commented Apr 29, 2023

lolodomo left a comment

lolodomo commented Apr 29, 2023

clinique commented Apr 29, 2023

lolodomo commented May 3, 2023

lolodomo commented May 7, 2023

clinique commented May 8, 2023

lolodomo left a comment

lolodomo left a comment

[Meteoalerte] Add an IconProvider #14811

[Meteoalerte] Add an IconProvider #14811

Conversation

clinique commented Apr 15, 2023

clinique commented Apr 15, 2023

lolodomo commented Apr 15, 2023

lolodomo commented Apr 15, 2023

lolodomo commented Apr 15, 2023

lolodomo Apr 15, 2023

Choose a reason for hiding this comment

clinique Apr 15, 2023

Choose a reason for hiding this comment

clinique Apr 17, 2023

Choose a reason for hiding this comment

clinique commented Apr 15, 2023 • edited Loading

clinique commented Apr 15, 2023

lolodomo commented Apr 15, 2023

clinique commented Apr 17, 2023 • edited Loading

lolodomo commented Apr 23, 2023 • edited Loading

clinique commented Apr 23, 2023

clinique commented Apr 23, 2023

lolodomo commented Apr 23, 2023 • edited Loading

lolodomo commented Apr 23, 2023

lolodomo commented Apr 23, 2023 • edited Loading

lolodomo commented Apr 23, 2023

lolodomo commented Apr 23, 2023

lolodomo commented Apr 29, 2023

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented Apr 29, 2023

clinique commented Apr 29, 2023

lolodomo commented May 3, 2023

lolodomo commented May 7, 2023

clinique commented May 8, 2023

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

clinique commented Apr 15, 2023 •

edited

Loading

clinique commented Apr 17, 2023 •

edited

Loading

lolodomo commented Apr 23, 2023 •

edited

Loading

lolodomo commented Apr 23, 2023 •

edited

Loading

lolodomo commented Apr 23, 2023 •

edited

Loading