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.

Sign up for GitHub

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

Jump to bottom

Sitemap and item config parsing adjustments #1843

Merged
merged 9 commits into from
May 5, 2023
Merged

Sitemap and item config parsing adjustments #1843

merged 9 commits into from
May 5, 2023

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Apr 15, 2023 •
edited by florian-h05
Loading

  • 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.

@mherwege mherwege requested a review from a team as a code owner April 15, 2023 06:17
This was referenced Apr 15, 2023
Merged
Merged
@relativeci
Copy link

relativeci bot commented Apr 15, 2023 •
edited
Loading

Job #954: Bundle Size — 15.75MiB (+0.04%).

0080f1a(current) vs f464fbe main#953(baseline)

⚠️ Bundle contains 16 duplicate packages

Metrics (2 changes)
                 Current
Job #954		      Baseline
Job #953
Initial JS 1.73MiB(+0.09%) 1.73MiB
Initial CSS 608.85KiB(~+0.01%) 608.8KiB
Cache Invalidation 93.94% 93.94%
Chunks 219 219
Assets 689 689
Modules 1717 1717
Duplicate Modules 85 85
Duplicate Code 1.73% 1.73%
Packages 137 137
Duplicate Packages 15 15
Total size by type (3 changes)
                 Current
Job #954		      Baseline
Job #953
CSS 858.58KiB (~+0.01%) 858.53KiB
Fonts 526.1KiB 526.1KiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.24MiB (+0.05%) 9.23MiB
Media 295.6KiB 295.6KiB
Other 4.73MiB (+0.04%) 4.73MiB

View job #954 report View main branch activity

florian-h05
florian-h05 requested changes Apr 15, 2023
View reviewed changes
Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

I have tried to add Items from textual definition with the example from the core PR, however the icon value is not recognized and therefore not set. It seems that the functions to process the value are missing in items-lexer.nearley as well as for sitemap-lexer.nearley.

@lolodomo
Copy link
Contributor

Look at #1839. Would be great if it could be fixed before tomorrow.

@florian-h05
Copy link
Contributor

#1839 is a different issue, the problem with the Item lexer is that whatever you type as icon name is not saved to the Item, because the lexer does not work properly.

@mherwege
Copy link
Contributor Author

I will need to check again. I thought it was working for me with the examples from the core PR and this this PR. If it is not for you, there may still be something wrong. Unfortunately, I will not be able to do it before tomorrow night. I don’t have access to my PC at the moment.

@J-N-K
Copy link
Member

J-N-K commented Apr 15, 2023

Is this PR only needed to make the new feature work or is it needed to fix something?

@lolodomo
Copy link
Contributor

lolodomo commented Apr 15, 2023 •
edited
Loading

Is this PR only needed to make the new feature work or is it needed to fix something?

Probably only if you want to import in Main UI items containing the extended syntax with several segments for the category/icon ?

@florian-h05
Copy link
Contributor

Is this PR only needed to make the new feature work or is it needed to fix something?

It seems it is only needed to to allow the new icon syntax in the „Add Items from textual definition“ and Sitemap editor’s codetab.

@J-N-K
Copy link
Member

J-N-K commented Apr 15, 2023

Ok, so it‘s not blocking M2.

@florian-h05
Copy link
Contributor

BTW, I‘ve just checked what the transformation editor in the UI does after the recent core changes. It needs some adjustment to be fully operational, but IMO it‘s non-blocking since nothing really broke. I‘ve already worked a bit on the adjustment, should be able to open a PR soon.

@mherwege
fix lexer assignments
91a8ea1 
Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

I know it is a bit late for the milestone, but I committed a fix to the parser.
Note that it also contains a fix for the syntax of an identifier in the items parser (bringing it in line with the one in the sitemap parser). The items parser was accepting more than it should have. This wasn't a real issue as long as the provided items file was valid, but would also import with invalid identifiers.

@florian-h05
Copy link
Contributor

@ghys Feel free to take over the review, I won’t have the time to finish it anytime soon.

mherwege added 2 commits April 26, 2023 16:10
@mherwege
fix inputHint validation
7d7e3a8 
Signed-off-by: Mark Herwege <[email protected]>
@mherwege
increase validation dialog size
eb3234c 
Signed-off-by: Mark Herwege <[email protected]>
florian-h05
florian-h05 reviewed May 4, 2023
View reviewed changes
Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

Code LGTM & everything works fine now!

I have just one minor question regarding the unit tests, thanks for improving them!

bundles/org.openhab.ui/web/src/pages/settings/pages/sitemap/__tests__/sitemap-edit_jest.spec.js Outdated Show resolved Hide resolved
@mherwege
restore valuecolor test
911719f 
Signed-off-by: Mark Herwege <[email protected]>
florian-h05
florian-h05 approved these changes May 5, 2023
View reviewed changes
Copy link
Contributor

@florian-h05 florian-h05 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!

@florian-h05 florian-h05 added this to the 4.0 milestone May 5, 2023
@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels May 5, 2023
@florian-h05 florian-h05 merged commit cb253f6 into openhab:main May 5, 2023
@mherwege mherwege deleted the sitemap_config branch May 7, 2023 12:57
@mherwege mherwege mentioned this pull request Jun 15, 2023
Merged
florian-h05 pushed a commit that referenced this pull request Jun 18, 2023
@mherwege
Sitemap parser: Fix negative numbers not allowed for visibility (#1928)
49de601 
The sitemap DSL parser was not able to correctly recognize negative numbers.

Regression from #1843 where explicit
recognition of hyphens was introduced for new icon syntax.

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
Reviewers

@florian-h05 florian-h05 florian-h05 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mherwege @lolodomo @florian-h05 @J-N-K