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

Extend sitemap syntax for icon #3378

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Feb 17, 2023

The icon value can now contain until 3 segments separated by a semi-column.
First segment is the icon source. Example: oh, if, iconify, material, f7, ...
Second segment is the icon set (and can be empty). Example: classic
Third segment is the icon name (and can contain hyphen). Example: temperature

In case only two segments are provided, the first segment is the icon source and the second the icon name. "classic" icon set is assumed.
In case only one segment is provided, the icon source is assumed to be the openHAB server and its classic icon set and the value is then the icon name.

Ability to surround the value with simple or double quotes is kept for better backward compatibility.

Signed-off-by: Laurent Garnier [email protected]

@lolodomo lolodomo requested a review from a team as a code owner February 17, 2023 00:38
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 24, 2023

I encounter difficulties to define the right syntax. Current PR is not working as expected.

ID cannot be used as icon segment as "-" should be accepted.

@mherwege
Copy link
Contributor

mherwege commented Feb 24, 2023

How about creating a new terminal ICON_ID:

terminal ICON_ID:
    ('a'..'z' | 'A'..'Z' | '_') ('a'..'z' | 'A'..'Z' | '_' | '-' | '0'..'9')* |
    '"' ('a'..'z' | 'A'..'Z' | '_') ('a'..'z' | 'A'..'Z' | '_' | '-' | '0'..'9')* '"' |
    "'" ('a'..'z' | 'A'..'Z' | '_') ('a'..'z' | 'A'..'Z' | '_' | '-' | '0'..'9')* "'";

That also avoids the variants in each input type (provided the quotes are handled properly elsewhere, but it should as it worked with STRING terminals).
I simplified the terminal syntax, removing the option to start with ^ or a number, but that is probably OK for icon id's.

Keep the valueConverter and put ICON_ID under Icon in line 151 (instead of STRING | ID).

@lolodomo
Copy link
Contributor Author

I tried yesterday to create a new terminal id but my attempt leaded to an error when building. I will try again with your proposal.

@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 25, 2023

@mherwege : did you get some difficulties when you updated the sitemap syntax ?
With my last changes, compiling is now OK.
I have only updated the bundle org.openhab.core.model.sitemap in my production environment and restarted the server.Here are the logs I got:

14:16:05.417 [INFO ] [del.core.internal.ModelRepositoryImpl] - Loading model 'test.sitemap'
14:16:05.881 [INFO ] [del.core.internal.ModelRepositoryImpl] - Loading model 'test2.sitemap'
...
14:16:50.922 [WARN ] [del.core.internal.ModelRepositoryImpl] - Configuration model 'test.sitemap' is either empty or cannot be parsed correctly!
14:16:50.948 [WARN ] [del.core.internal.ModelRepositoryImpl] - Configuration model 'test2.sitemap' is either empty or cannot be parsed correctly!

I have no syntax error displayed, that is a good point.
But finally the loading is apparently failing.

@mhilbush
Copy link
Contributor

@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 25, 2023

@mhilbush : I have only a problem with my sitemap files.
And I updated only one of my bundles (the other are from snapshot 3301):

openhab> bundle:list -s | grep model
 46 x Active   x  80 x 2.1.9                  x io.swagger.core.v3.swagger-models
189 x Active   x  80 x 4.0.0.202302040305     x org.openhab.core.model.core
190 x Active   x  80 x 4.0.0.202302040307     x org.openhab.core.model.item
191 x Active   x  80 x 4.0.0.202302040309     x org.openhab.core.model.item.ide
192 x Active   x  80 x 4.0.0.202302040309     x org.openhab.core.model.item.runtime
193 x Active   x  80 x 4.0.0.202302040313     x org.openhab.core.model.lsp
194 x Active   x  80 x 4.0.0.202302040307     x org.openhab.core.model.persistence
195 x Active   x  80 x 4.0.0.202302040308     x org.openhab.core.model.persistence.ide
196 x Active   x  80 x 4.0.0.202302040308     x org.openhab.core.model.persistence.runtime
197 x Active   x  80 x 4.0.0.202302040312     x org.openhab.core.model.rule
198 x Active   x  80 x 4.0.0.202302040312     x org.openhab.core.model.rule.ide
199 x Active   x  80 x 4.0.0.202302040312     x org.openhab.core.model.rule.runtime
200 x Active   x  80 x 4.0.0.202302040311     x org.openhab.core.model.script
201 x Active   x  80 x 4.0.0.202302040311     x org.openhab.core.model.script.ide
202 x Active   x  80 x 4.0.0.202302040311     x org.openhab.core.model.script.runtime
203 x Active   x  80 x 4.0.0.202302251312     x org.openhab.core.model.sitemap
204 x Active   x  80 x 4.0.0.202302040309     x org.openhab.core.model.sitemap.ide
205 x Active   x  80 x 4.0.0.202302040309     x org.openhab.core.model.sitemap.runtime
206 x Active   x  80 x 4.0.0.202302040309     x org.openhab.core.model.thing
207 x Active   x  80 x 4.0.0.202302040310     x org.openhab.core.model.thing.ide
208 x Active   x  80 x 4.0.0.202302040310     x org.openhab.core.model.thing.runtime

I am wondering if this is required or not that I recompile and deploy a new version of the bundles org.openhab.core.model.sitemap.ide and org.openhab.core.model.sitemap.runtime ?

@mhilbush
Copy link
Contributor

Ok, then I guess unrelated to what was reported on the forum.

I opened an issue for that problem.

#3400

@lolodomo
Copy link
Contributor Author

When I recompile from main (with my changes) and deploy the bundle, it works again, so this is certainly something more in link with my changes.
That's why I would like to have feedback from @mherwege who handled and tested an updated sitemap syntax very recently.

@mherwege
Copy link
Contributor

@lolodomo I have been testing this in the Eclipse IDE. I did an mvn install on core.model.sitemap, didn’t touch the ide or runtime projects. I did have all three open (+ the basicui project) so they where picked up when resolving the launch bnd file. And that worked for testing. I did not try to move the file into an install of OH and test outside of the IDE.
The important part is to have all code generated properly and included. An mvn clean install on the sitemap repository should make sure of that. The message you get about the sitemaps points to something it cannot interpret in the sitemaps, probably because the new code is not included properly or does not work as expected. It may help to check the generated code to see if something is not as expected there.

@lolodomo
Copy link
Contributor Author

Unfortunately, I have still an old Eclipse not fully compatible with Java 17. I can edit and compile code but I can't run OH inside Eclipse.
During this time, I try to move to your changed syntax with the added "input" element and I did not encounter this same kind of problem. So clearly, this is a problem with my own updated sitemap syntax.

@lolodomo lolodomo force-pushed the sitemap_model_icon branch 3 times, most recently from 0115ca0 to 7134bd5 Compare February 26, 2023 19:31
@lolodomo lolodomo force-pushed the sitemap_model_icon branch from 7134bd5 to f3c60dc Compare April 11, 2023 11:08
@lolodomo lolodomo changed the title [WIP] Update sitemap syntax for icon Extend sitemap syntax for icon Apr 11, 2023
@lolodomo
Copy link
Contributor Author

Here is an example of sitemap showing the different cases:

sitemap test label="Tests"
{
        Frame label="Icon ref without quotes" {
                Default item=CurrentTemp icon=temperature
                Default item=CurrentHumidity icon=classic:humidity
                Default item=CurrentPressure icon=oh:classic:pressure
                Default item=CurrentPressureTrend icon=material::arrow_drop_down
                Default item=SetpointTemp icon=iconify:wi:day-sunny-overcast iconcolor=["green"]
                Default item=TestString icon=material::favorite iconcolor=["yellow"]
        }
        Frame label="Icon ref with quotes" {
                Default item=CurrentTemp icon="temperature"
                Default item=CurrentHumidity icon="classic:humidity"
                Default item=CurrentPressure icon="oh:classic:pressure"
                Default item=CurrentPressureTrend icon="material::arrow_drop_down"
                Default item=SetpointTemp icon="iconify:wi:day-sunny-overcast" iconcolor=["green"]
                Default item=TestString icon="material::favorite" iconcolor=["yellow"]
        }
}

And the result in Basic UI:
image

@lolodomo lolodomo force-pushed the sitemap_model_icon branch from f3c60dc to 527ad75 Compare April 11, 2023 11:18
@lolodomo
Copy link
Contributor Author

I found a way to change the syntax that works as expected.
PR is now ready for review.

Copy link
Contributor

@clinique clinique left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo
Copy link
Contributor Author

It seems the build succeeded but with the now so famous final "Finished: UNSTABLE".

@lolodomo lolodomo mentioned this pull request Apr 11, 2023
The icon value can now contain until 3 segments separated by a semi-column.
First segment is the icon source. Example: oh, if, iconify, material, f7, ...
Second segment is the icon set (and can be empty). Example: classic
Third segment is the icon name (and can contain hyphen). Example: temperature

In case only two segments are provided, the first segment is the icon source and the second the icon name. "classic" icon set is assumed if icon source is "oh".
In case only one segment is provided, the icon source is assumed to be the openHAB server and its classic icon set and the value is then the icon name.

Ability to surround the value with simple or double quotes is kept for better backward compatibility.

Signed-off-by: Laurent Garnier <[email protected]>

Fix
@lolodomo lolodomo force-pushed the sitemap_model_icon branch from 527ad75 to aebe73d Compare April 11, 2023 18:04
@lolodomo
Copy link
Contributor Author

I finally changed the expectation when the icon value contains only 2 segments to be more compliant with Main UI syntax.
Here is an example of sitemap showing the different cases:

sitemap test label="Tests"
{
        Frame label="Icon ref without quotes" {
                Default item=CurrentTemp icon=temperature
                Default item=CurrentHumidity icon=oh:humidity
                Default item=CurrentPressure icon=oh:classic:pressure
                Default item=CurrentPressureTrend icon=material:arrow_drop_down
                Default item=SetpointTemp icon=iconify:wi:day-sunny-overcast iconcolor=["green"]
                Default item=TestString icon=material:favorite iconcolor=["yellow"]
        }
        Frame label="Icon ref with quotes" {
                Default item=CurrentTemp icon="temperature"
                Default item=CurrentHumidity icon="oh:humidity"
                Default item=CurrentPressure icon="oh:classic:pressure"
                Default item=CurrentPressureTrend icon="material:arrow_drop_down"
                Default item=SetpointTemp icon="iconify:wi:day-sunny-overcast" iconcolor=["green"]
                Default item=TestString icon="material:favorite" iconcolor=["yellow"]
        }
}

And the result in an updated version of Basic UI:
image

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Apr 12, 2023
@J-N-K J-N-K added this to the 4.0 milestone Apr 12, 2023
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM

@J-N-K J-N-K merged commit fa37a46 into openhab:main Apr 12, 2023
@lolodomo lolodomo deleted the sitemap_model_icon branch April 12, 2023 15:47
@lolodomo
Copy link
Contributor Author

@mherwege : do you think it requires a change in the sitemap generator of MainUI?

@mherwege
Copy link
Contributor

mherwege commented Apr 14, 2023

@lolodomo Yes, it does. It generates it with quotes around the icon name.
The challenge with all of this is that it requires special treatment without quotes. The json in the rest messages always has quotes.
I will look into this, working on some other validation improvements there also.

@J-N-K
Copy link
Member

J-N-K commented Apr 14, 2023

@mherwege Do we need to revert this for M2?

@lolodomo
Copy link
Contributor Author

lolodomo commented Apr 14, 2023

The quotes are still an available option in sitemap.
It is for item definition that I removed this option.
So if the sitemap generator uses quotes for the icon, that is fine and there is probably nothing to change.

@mherwege
Copy link
Contributor

mherwege commented Apr 15, 2023

@lolodomo It is OK when writing out the code for the sitemap. But I tested your example sitemap above. The variant without quotes now makes the sitemap code interpretation fail. The parser needs to be in sync with the xtext parser. I will see what I can do.
There may also be an issue when importing items from items files with the new syntax now.
@J-N-K I don't think we need to revert, as there is an easy workaround: Remove the quotes from the textual definition when importing items/writing sitemap code in the UI. We can fix it for the next milestone.

@J-N-K
Copy link
Member

J-N-K commented Apr 15, 2023

Next milestone is scheduled for sunday, that‘s why I‘m asking.

@mherwege
Copy link
Contributor

@lolodomo @J-N-K I created openhab/openhab-webui#1843 that includes fixes for this, tested with the examples provided.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab4-m1-earlybird/145193/25

florian-h05 pushed a commit to openhab/openhab-webui that referenced this pull request May 5, 2023
- Adapt sitemap and item lexers to changes in icon name syntax
- Restrict the elements that can be added to a sitemap
- Added extra sitemap validations (in line with xtext validation in
core)
- Added test for sitemap parser and validation

This solves the issue in main UI created by openhab/openhab-core#3539 and openhab/openhab-core#3378.

Signed-off-by: Mark Herwege <[email protected]>
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.

6 participants