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

charts - generalize evaluating expressions for charts #1638

Closed

Conversation

crnjan
Copy link
Contributor

@crnjan crnjan commented Jan 13, 2023

This is based on #1419. Also contains fixes from #1634.

fixes #1418
fixes #1295
fixes #1102
fixes #1072

It's still WIP, need to perform more tests, any help greatly appreciated

EDIT: went through cases reported in above issues and seems to work now. Also, as far as I tested (so far) locally with my setup seems to work - but probably don't have all use-cases setup so probably missed something - therefore would be much appreciated if any volunteer tries it out too, thank you!

@crnjan crnjan requested a review from a team as a code owner January 13, 2023 21:20
@relativeci
Copy link

relativeci bot commented Jan 13, 2023

Job #711: Bundle Size — 15.97MiB (~+0.01%).

f0ced0f(current) vs a0b13e4 main#710(baseline)

Metrics (no changes)
                 Current
Job #711
     Baseline
Job #710
Initial JS 1.73MiB 1.73MiB
Initial CSS 608.52KiB 608.52KiB
Cache Invalidation 90.48% 90.48%
Chunks 218 218
Assets 688 688
Modules 2011 2011
Duplicate Modules 108 108
Duplicate Code 1.8% 1.8%
Packages 133 133
Duplicate Packages 15 15
Total size by type (2 changes)
                 Current
Job #711
     Baseline
Job #710
CSS 856.56KiB 856.56KiB
Fonts 1.08MiB 1.08MiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.04MiB (~-0.01%) 9.04MiB
Media 295.6KiB 295.6KiB
Other 4.59MiB (~+0.01%) 4.59MiB

View job #711 reportView main branch activity

@crnjan crnjan marked this pull request as draft January 13, 2023 21:24
@crnjan crnjan force-pushed the feature/charts_support_for_expressions branch from 42b6eea to 04ea52b Compare January 13, 2023 21:25
@crnjan crnjan force-pushed the feature/charts_support_for_expressions branch from e9de334 to 844b65c Compare January 13, 2023 22:18
@ghys
Copy link
Member

ghys commented Jan 14, 2023

Thanks! Generalizing expressions in chart widgets is a plus for sure! It's looking good code wise but I'll have a test run soon.

@crnjan crnjan force-pushed the feature/charts_support_for_expressions branch from 437a6d8 to b561156 Compare January 14, 2023 08:32
@crnjan
Copy link
Contributor Author

crnjan commented Jan 14, 2023

Created a test jar, please run:

bundle:update org.openhab.ui https://github.com/crnjan/openhab-webui/raw/feature/diff_poc/bundles/org.openhab.ui/target/org.openhab.ui-4.0.0-SNAPSHOT_eval.jar

in order to try it out. If there is any case not working, please let me know - and provide a (minimal) working sample (yaml) so I can try locally.

If needed, revert back to official UI jar with

bundle:update org.openhab.ui https://ci.openhab.org/job/openHAB-WebUI/lastSuccessfulBuild/artifact/bundles/org.openhab.ui/target/org.openhab.ui-4.0.0-SNAPSHOT.jar

@crnjan crnjan force-pushed the feature/charts_support_for_expressions branch from 7d0d05a to c96e243 Compare January 14, 2023 10:35
@crnjan crnjan marked this pull request as ready for review January 14, 2023 19:30
@crnjan crnjan force-pushed the feature/charts_support_for_expressions branch from 2caa28f to 26b04b9 Compare January 14, 2023 19:52
@crnjan
Copy link
Contributor Author

crnjan commented Jan 14, 2023

Accidentally pushed (potential) fix for #1072, introducing onStates

        markArea:
          - component: oh-mark-area
            config:
              item: LivingRoomRadiatorPower
              onStates: CLOSED 

I can revert if needed ... also feel free to suggest a better name ...

@crnjan crnjan force-pushed the feature/charts_support_for_expressions branch from 13d6db3 to 5213551 Compare January 14, 2023 20:03
@ghys
Copy link
Member

ghys commented Jan 16, 2023

I can revert if needed ... also feel free to suggest a better name ...

It's sort-of related in a general sense (allow customizing charts beyond some arbitrary/common sense defaults, for those who need this customization), so no need to open a separate PR. The name onStates is fine, maybe markStates alternatively or simply on or states. Not sure which I like better? Since oh-mark-area isn't currently described and won't appear in config sheets or https://next.openhab.org/docs/ui/components/#chart-page-components (probably something to remedy one day) it's kind of obscure anyway :)

By the way it accidentally works when you feed that parameter a string instead of an array (because includes is both in the prototype of Array and String) but it might not work 100% as the user expects (for instance onStates: ONOFF will match both ON and OFF, also onStates: ="ABC123" will match AB, BC, C12... but onStates: 1 won't match 1, if needs to be onStates: [1]). Maybe transforming it into a single-element array if it's not an array would be safest.

NB: I tried this with a scene Number item but noticed it's actually a string in the API response in this format in my case:

image

Since "4.0" == 4 in JS maybe it would be valuable to use the non-strict equality somehow to check if there's a match?
Like:

onStates.findIndex((s) => s == p.state || s == rollingState) >= 0

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Suggestions for the above comment.

The rest of the PR LGTM!

lolodomo and others added 13 commits January 17, 2023 14:44
…enhab#1612)

This is the same fix as openhab#1611 but for OH 4.0 main branch.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Reference openhab/openhab-addons#14013.
Reference openhab/openhab-addons#14096.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: jsetton <jeremy.setton@gmail.com>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: jsetton <jeremy.setton@gmail.com>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog boris.krivonog@inova.si

Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
crnjan and others added 9 commits January 17, 2023 14:44
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
* Fixes typos

Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
…/oh-mark-area.js

Co-authored-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
…/oh-mark-area.js

Co-authored-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Michael Krug <michi.krug@gmail.com>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants