Skip to content

Thing and Item Details: Use core's file-format service to display code in YAML and DSL #3180

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented May 3, 2025

Depends on:

Partially resolve openhab/openhab-core#4585

  • Add segmented buttons to select between YAML and DSL
  • Add "Revert" button
  • Add "Copy" to clipboard button
  • Add a movable Parser Error popup to display the errors returned from the parser API call
  • Adjust the Item and Thing editor's hints to the new syntax
  • Fix some issues with editor's hint/completions
  • Add channel type hint
  • Disable the item metadata hint since it's never supported
  • Fix / catch Thing firmware previously unhandled error when loading
image image

Parse error details shown in a movable popup
image

image

YAML Hints adjusted to the new YAML syntax
image

image image

I added this one hint for the channel type:
image

@jimtng jimtng requested review from ghys and florian-h05 as code owners May 3, 2025 13:16
Copy link

relativeci bot commented May 3, 2025

#3120 Bundle Size — 11.2MiB (+0.05%).

3f787a1(current) vs 5579ef8 main#3110(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
#3120
     Baseline
#3110
Regression  Initial JS 2MiB(~+0.01%) 2MiB
No change  Initial CSS 577.53KiB 577.53KiB
Change  Cache Invalidation 20.43% 18.14%
No change  Chunks 250 250
No change  Assets 273 273
Change  Modules 2978(+0.1%) 2975
No change  Duplicate Modules 151 151
No change  Duplicate Code 1.74% 1.74%
No change  Packages 97 97
No change  Duplicate Packages 2 2
Bundle size by type  Change 2 changes Regression 2 regressions
                 Current
#3120
     Baseline
#3110
Regression  JS 9.41MiB (+0.06%) 9.4MiB
Regression  CSS 868.77KiB (~+0.01%) 868.72KiB
No change  Fonts 526.1KiB 526.1KiB
No change  Media 295.6KiB 295.6KiB
No change  IMG 140.74KiB 140.74KiB
No change  HTML 1.38KiB 1.38KiB
No change  Other 871B 871B

Bundle analysis reportBranch jimtng:thing-details-codeProject dashboard


Generated by RelativeCIDocumentationReport issue

@jimtng jimtng force-pushed the thing-details-code branch from e45644a to bc07f37 Compare May 3, 2025 13:24
@lolodomo
Copy link
Contributor

lolodomo commented May 3, 2025

I like it.

You finally decided to strip YAML first lines

Would it be possible to have a horizontal scrollbar to be able to see the full DSL?

@jimtng
Copy link
Contributor Author

jimtng commented May 3, 2025

There are vertical and horizontal scrollbars, just that they're not visible by default until you tried to scroll. That's the default behaviour (from the OS maybe?)

@jimtng
Copy link
Contributor Author

jimtng commented May 3, 2025

The YAML headers are stripped (version, things: and thinguid) and the rest are unindented. As a result of this, if someone were to copy paste from here, they'd have to re-add the indentation.

There's also the "Copy File Definition" button on the main "Thing" tab that was added before, which will give them the full yaml.

Maybe we can move that button into the bottom toolbar

@lolodomo
Copy link
Contributor

lolodomo commented May 3, 2025

There's also the "Copy File Definition" button on the main "Thing" tab that was added before, which will give them the full yaml.

Yes, exact. So that's fine for me.

@lolodomo
Copy link
Contributor

lolodomo commented May 3, 2025

Did you already handle errors and warnings when editing?
Edited code is parsed only when saving the thing ?

@jimtng
Copy link
Contributor Author

jimtng commented May 3, 2025

Maybe we can move that button into the bottom toolbar

Come to think of it, if we moved the "copy file" button into the "Code" section, that would be a bit confusing, especially since the yaml code is not exactly the same as the yaml file. So let's leave the copy file button where it is now.

@jimtng
Copy link
Contributor Author

jimtng commented May 3, 2025

Did you already handle errors and warnings when editing?
Edited code is parsed only when saving the thing ?

Edited code gets parsed whenever you switch away from Code tab or when switching yaml <=> dsl. If the editor isn't dirty (modified), parsing is skipped.

One thing I'm thinking about:
Right now you're passing on the warnings back in the json (when parsing). I haven't taken that into account yet.
Also you're returning some (all?) errors as HTTP error and when that happens the errors are in plain text not json I think.

It would be easier for the ui code to get the errors in json too so the actual error message can be displayed, not just the generic http error name (eg. "Bad Request")

@lolodomo
Copy link
Contributor

lolodomo commented May 3, 2025

Also you're returning some (all?) errors as HTTP error and when that happens the errors are in plain text not json I think.

It would be easier for the ui code to get the errors in json too so the actual error message can be displayed, not just the generic http error name (eg. "Bad Request")

I can do it but look at this answer when I proposed it:
openhab/openhab-core#4779 (comment)

@lolodomo
Copy link
Contributor

lolodomo commented May 3, 2025

I don't know if you saw my comment (elsewhere) to rename the tab to make it clear it is generated code and not original code. what is your opinion about that?

@jimtng
Copy link
Contributor Author

jimtng commented May 3, 2025

Maybe hold off on the json error code. I just had a bit of experiment. Whilst I could display the errors in the body, it's a lot to display. Too long for a small dialog box. The editor's built in yaml support can point out obvious syntax errors so hopefully that helps.

When I tried dsl, I got this:

Error parsing DSL [1,90]: no viable alternative at input '192'
[1,94]: no viable alternative at input '168'
[1,98]: no viable alternative at input '1'
[1,100]: no viable alternative at input '107'
[3,21]: no viable alternative at input 'Stop'
[4,29]: no viable alternative at input 'Duration'
[9,37]: no viable alternative at input 'Media'
[10,35]: no viable alternative at input 'Track'
[14,21]: no viable alternative at input 'Mute'
[15,35]: no viable alternative at input 'URL'
[19,39]: no viable alternative at input 'Episode'
[20,25]: no viable alternative at input 'Idling'
[21,29]: no viable alternative at input 'Subtitle'
[22,25]: no viable alternative at input 'Volume'
[24,29]: missing EOF at 'Location'
[33,44]: mismatched character '<EOF>' expecting '"'

and tbh I'm not sure it's very helpful.

Let's go ahead without this enhancement for now.

I don't know if you saw my comment (elsewhere) to rename the tab to make it clear it is generated code and not original code. what is your opinion about that?

I saw your post about that. I think let's just stay with "CODE". "GEN CODE" will raise more questions, and "GENERATED CODE" is too long and will also make people wonder.

@lolodomo
Copy link
Contributor

lolodomo commented May 3, 2025

For YAML, I implemented some checks (errors and warnings) that would be helpful if displayed.

@jimtng
Copy link
Contributor Author

jimtng commented May 3, 2025

I saw the yaml one. Whilst it seems helpful, I'm not sure where to display it. It's too big / too much text for the dialog. There may be a way to make codemirror editor to handle it / show it. See https://github.com/openhab/openhab-webui/blob/efe3fefd4f82402b055c86a33cc379075d0efdf2/bundles/org.openhab.ui/web/src/components/config/controls/script-editor.vue#L294C21-L306

We'll need the line numbers in a similar manner instead of/in addition to just a message.

Yaml isn't the main issue, because as you see we already have a built in (generic) yaml syntax checker on the editor.

The DSL one is the problem.

An easy solution is to make DSL read-only. Right now you could edit the DSL just like you could edit the YAML.

@jimtng jimtng force-pushed the thing-details-code branch from 14d7cdd to d6e0946 Compare May 4, 2025 04:53
@jimtng jimtng marked this pull request as draft May 4, 2025 07:36
@jimtng jimtng force-pushed the thing-details-code branch from d6e0946 to d7b60ff Compare May 5, 2025 03:14
@jimtng jimtng changed the title Thing Details: Use core's file-format service to display code in YAML and DSL Thing and Item Details: Use core's file-format service to display code in YAML and DSL May 5, 2025
@jimtng
Copy link
Contributor Author

jimtng commented May 5, 2025

Refactored the code editor into a component so it can be used in both Thing Details page and Item Details page

@jimtng jimtng force-pushed the thing-details-code branch from d7b60ff to d0f45d0 Compare May 5, 2025 03:18
@lolodomo
Copy link
Contributor

lolodomo commented May 5, 2025

Yaml isn't the main issue, because as you see we already have a built in (generic) yaml syntax checker on the editor.

What is interesting is the additionnel checks I implemented like valid item type value, valid item dimension value, ...

@jimtng
Copy link
Contributor Author

jimtng commented May 6, 2025

I've decided to display the full YAML, including version:, things: (or items:), and the uid. It should avoid confusion and it gives a similar result/structure as "Copy to clipboard".

I managed to make it so the first 3 lines are un-editable when it's YAML

@jimtng
Copy link
Contributor Author

jimtng commented May 7, 2025

Updated the screenshots. YAML syntax is shown exactly as provided, including version:, things:, thinguid:. They are no longer stripped as they were done initially.

I hope we can finalise this before the 17th. I won't have a lot of time to work on this after that time until end of June. @lolodomo are the Items (DSL and YAML) ready? I'm trying to combine all the available core PRs to create a complete version, but I can't see a YAML Item converter. Also I mustn't have combined them properly because the combined version returned Bad requests for Things too.

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2025

I hope we can finalise this before the 17th. I won't have a lot of time to work on this after that time until end of June. @lolodomo are the Items (DSL and YAML) ready? I'm trying to combine all the available core PRs to create a complete version, but I can't see a YAML Item converter.

I am sorry but we are waiting for @openhab/core-maintainers to merge stuff.
My availability will also be largely reduced at the end of this week!
To help you, I propose to group several PRs so that you can have one big PR that offers everything including the conversion API for things and items. So I will change PR openhab/openhab-core#4779 to cover things and items but it will make it now dependent on openhab/openhab-core#4776 which itself depends on openhab/openhab-core#4753. And I will also include openhab/openhab-core#4630 in it. Regarding the last one, I made progress this morning, metadata are now properly parsed for DSL items, remains an adjustment for channel links.

@jimtng
Copy link
Contributor Author

jimtng commented May 7, 2025

I propose to group several PRs so that you can have one big PR that offers everything including the conversion API for things and items. So I will change PR openhab/openhab-core#4779 to cover things and items but it will make it now dependent on openhab/openhab-core#4776 which itself depends on openhab/openhab-core#4753. And I will also include openhab/openhab-core#4630 in it. Regarding the last one, I made progress this morning, metadata are now properly parsed for DSL items, remains an adjustment for channel links.

Are they currently separated so that they can be merged independently? If so, perhaps leave them as is. I just need to figure out how to merge them all on my local git so I can build a single complete version. I think I only attempted to merge 3 PRs last time. I'll try all the 4 you mentioned here.

@jimtng
Copy link
Contributor Author

jimtng commented May 7, 2025

I tried to combine those 4 PRs but I failed :(

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2025

I created PR openhab/openhab-core#4793 that contains everything, you can play with it. Ir replaces two PRs and is based on the PR that adds YAML for items.
The PR is not 100% finished, especially the DSL part for items.

@jimtng
Copy link
Contributor Author

jimtng commented May 7, 2025

I created PR openhab/openhab-core#4793 that contains everything, you can play with it. Ir replaces two PRs and is based on the PR that adds YAML for items. The PR is not 100% finished, especially the DSL part for items.

Thank you! I will test this now.

@jimtng jimtng force-pushed the thing-details-code branch 4 times, most recently from 48d8e09 to a52881d Compare May 8, 2025 05:14
@jimtng jimtng force-pushed the thing-details-code branch 2 times, most recently from b8d993c to 3e2fba6 Compare May 8, 2025 12:04
@jimtng jimtng force-pushed the thing-details-code branch from 3e2fba6 to 151ac5f Compare May 8, 2025 12:05
@jimtng
Copy link
Contributor Author

jimtng commented May 8, 2025

Marking certain sections of the editor (the UID bit) as read-only poses a problem when the user selects all text and pastes new text, amongst other operations. The read-only part doesn't get overwritten by the paste operation. Making it "work" involves adding too much extra code and I think it outweighs the benefit. I'm removing this feature.

There is already a check to ensure that the uid isn't modified, and the parser will complain on syntax errors.

Signed-off-by: Jimmy Tanagra <[email protected]>
@jimtng
Copy link
Contributor Author

jimtng commented May 8, 2025

@lolodomo wdyt about the "Copy" button I added in the Code tab (See the original post for screenshot)?

I noticed that the code produced in the code tab has all the channels defined by the Thing, vs the "Generate DSL/YAML File" function will only return the channels that have been customised, so it's usually a lot more compact.

The "Copy" button in the code tab can probably be used by users to paste into forums, but they can of course just select all + Ctrl+C too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting other PR Depends on another PR enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thing/Item: move into core the conversions to/from YAML code done by MainUI
3 participants