Skip to content

Conversation

@jsjames
Copy link
Contributor

@jsjames jsjames commented May 9, 2025

This PR is dependent on the #3179 which adds support for the a new chart series for state types. This PR further adds support and default behavior for discrete items to use this new state-series. Depending on the selected items, it will use separate grids - one using a y-axis of a oh-value-series and one use oh-category-series (discrete items).

Note, that there seems to be a issue with echarts where if the chart is showing when updates are made where there are multiple value-axis with different units AND there are multiple grids, there is an echart exception complaining about y- and x-axis having to be on the same chart. To address this, i added a showChart=false in the updateItems function. However, this now causes an exception in the vue framework:

smart-select-class.js:105 Uncaught TypeError: Cannot read properties of undefined (reading 'options')
    at HTMLInputElement.handleInputChange (smart-select-class.js:105:41)
    at HTMLDivElement.handleLiveEvent (dom7.module.js:320:48)

@florian-h05, before spending more time debugging:

  • will the oh-state-series Charts: Add oh-state-series to render state transitions over time #3179 make it into 5.0?
  • what are your thoughts on changing analyzer to support this series type for discrete items?
  • any suggestions/thoughts on the above error? This one seems to be some interaction of echarts/vue and difficult to track down. Was thinking if/when we update the vue3, this behavior might be totally different?

@jsjames jsjames requested review from florian-h05 and ghys as code owners May 9, 2025 14:44
@jsjames jsjames changed the title [MainUI] Support of oh-state-series chart in Analyzer pages [MainUI] Support of oh-state-series chart in Analyzer pages (WIP) May 9, 2025
@relativeci
Copy link

relativeci bot commented May 9, 2025

#3273 Bundle Size — 11.28MiB (-0.02%).

90ee77b(current) vs 005e58d main#3264(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes
                 Current
#3273
     Baseline
#3264
No change  Initial JS 2.01MiB 2.01MiB
No change  Initial CSS 577.53KiB 577.53KiB
Change  Cache Invalidation 18.86% 18.61%
No change  Chunks 251 251
No change  Assets 274 274
Change  Modules 2995(+0.03%) 2994
No change  Duplicate Modules 148 148
No change  Duplicate Code 1.69% 1.69%
No change  Packages 97 97
No change  Duplicate Packages 2 2
Bundle size by type  Change 1 change Improvement 1 improvement
                 Current
#3273
     Baseline
#3264
Improvement  JS 9.49MiB (-0.02%) 9.49MiB
No change  CSS 871KiB 871KiB
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 jsjames:analyzer-state-seriesProject dashboard


Generated by RelativeCIDocumentationReport issue

@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI awaiting other PR Depends on another PR labels May 16, 2025
@florian-h05
Copy link
Contributor

Hi @jsjames and thanks for your PRs!
I am currently reviewing #3179, so those two PRs will definitely make it into 5.0.
Wrt to your other questions:

what are your thoughts on changing analyzer to support this series type for discrete items?

I like the idea of having an analyzer available for discrete Items 👍

any suggestions/thoughts on the above error? This one seems to be some interaction of echarts/vue and difficult to track down

I will try to track this down myself.

@florian-h05 florian-h05 force-pushed the analyzer-state-series branch from 15b7b1d to 232ec39 Compare May 20, 2025 22:44
@florian-h05
Copy link
Contributor

Rebased this PR.

@florian-h05 florian-h05 removed the awaiting other PR Depends on another PR label May 21, 2025
@florian-h05 florian-h05 requested a review from Copilot May 21, 2025 09:37

This comment was marked as outdated.

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 now, I have also been able to fix the TypeError.
Waiting for the discussion about the Dimmer.

@florian-h05 florian-h05 added this to the 5.0 milestone May 21, 2025
@florian-h05 florian-h05 force-pushed the analyzer-state-series branch from 80b6800 to ede1965 Compare May 21, 2025 10:49
@jsjames
Copy link
Contributor Author

jsjames commented May 21, 2025

Code LGTM now, I have also been able to fix the TypeError. Waiting for the discussion about the Dimmer.

Thanks for fixing that - not having done much in Vue, i am interested to learn how you were able to track that down?

@florian-h05
Copy link
Contributor

i am interested to learn how you were able to track that down?

The error message hinted to the Framework7 smart select.
Its only usage inside the analyzer is in the item picker and when trying to reproduce the issue, I noticed it does mainly occur when the Item picker is opened. I then checked what is affected when showChart is changed.

@jsjames jsjames force-pushed the analyzer-state-series branch from e7b377b to 6ae8682 Compare June 17, 2025 16:55
@florian-h05 florian-h05 changed the title [MainUI] Support of oh-state-series chart in Analyzer pages (WIP) Analyzer: Add support of oh-state-series Jun 17, 2025
@florian-h05
Copy link
Contributor

What would be nice would be to provide both options for Dimmer Items so the user can select the representation.

@jsjames
Copy link
Contributor Author

jsjames commented Jun 18, 2025

What would be nice would be to provide both options for Dimmer Items so the user can select the representation.

I thought about that and it would be good. I was looking at the logic to do this though - and it seems there are a lot of nuances in the code with the different types of graphs/charts and then which series types are allowed. Wondering if we should delay this until 5.1 and/or the vue 3. Maybe we can clean up the code a bit so its easier to understand.

@jsjames jsjames force-pushed the analyzer-state-series branch from 6ae8682 to de9e6c5 Compare June 20, 2025 14:35
@florian-h05
Copy link
Contributor

Do we need Vue 3 to clean up the code? I don’t see a real technical reason for that.

@jsjames
Copy link
Contributor Author

jsjames commented Jun 22, 2025

Do we need Vue 3 to clean up the code? I don’t see a real technical reason for that.

Correct, no technical reason - just that I didn't want to start restructuring things so close to the 5.0 release. I can take a look at it more this week if you'd like.

jsjames and others added 2 commits June 28, 2025 16:27
Added support for discrete items to use oh-state-series on a separate grid
BUG: Had to add this.showChart = false in updateItems in order for echarts to update the options w/o error when there are multiple unit y-axis value-series. However, with that change, when selecting additional items from the item list - it now generates a Vue rendterItem error.

Signed-off-by: Jeff James <[email protected]>
Signed-off-by: Florian Hotze <[email protected]>
florian-h05 and others added 3 commits June 28, 2025 16:27
Fixes:

```
smart-select-class.js:105 Uncaught TypeError: Cannot read properties of undefined (reading 'options')
    at HTMLInputElement.handleInputChange (smart-select-class.js:105:41)
    at HTMLDivElement.handleLiveEvent (dom7.module.js:320:48)
```

Signed-off-by: Florian Hotze <[email protected]>
Fixed xAxisIndex when both value and state graphs are displayed

Signed-off-by: Jeff James <[email protected]>
@jsjames jsjames force-pushed the analyzer-state-series branch from de9e6c5 to cd0649a Compare June 28, 2025 23:31
@jsjames jsjames changed the title Analyzer: Add support of oh-state-series Analyzer: Refactor and Add support of oh-state-series Jun 28, 2025
@jsjames
Copy link
Contributor Author

jsjames commented Jun 28, 2025

Do we need Vue 3 to clean up the code? I don’t see a real technical reason for that.

@florian-h05 - I did a refactor / clean-up of the code. I've tested fairly thoroughly, but would welcome more testing. The feature to select between State and Line/Area for a Dimmer is now possible.

I still have some console.log messages for debugging - are those automatically removed in the production build process or do I have to manually remove them?

@jsjames jsjames requested review from Copilot and florian-h05 June 28, 2025 23:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the analyzer to support the new oh‑state‑series while standardizing coordinate system initialization and chart configuration across multiple views. Key changes include:

  • Introducing oh‑state‑series support and refactoring series initialization in chart‑time.js.
  • Updating coordinate system handling (coordSettings) and UI adjustments in analyzer.vue.
  • Adapting visual map rendering and i18n keys across chart‑calendar.js, chart‑aggregate.js, and localization files.

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
bundles/org.openhab.ui/web/src/pages/analyzer/chart-time.js Updated series initialization to account for discrete items via oh‑state‑series and adjusted grid handling.
bundles/org.openhab.ui/web/src/pages/analyzer/chart-calendar.js Modified coordinate and visualMap rendering using coordSettings.
bundles/org.openhab.ui/web/src/pages/analyzer/chart-aggregate.js Introduced DIMENSION_MAP for dimension mapping and updated axis initialization.
bundles/org.openhab.ui/web/src/pages/analyzer/analyzer.vue Refactored coordinate system handling, series option updates, and list iterations to use coordSettings; updated UI controls.
bundles/org.openhab.ui/web/src/pages/analyzer/analyzer-helpers.js Added helper methods for parsing units, axis rendering, and visual map generation.
bundles/org.openhab.ui/web/src/components/widgets/chart/series/oh-state-series.js Implemented oh‑state‑series support with a note on deprecated API usage.
bundles/org.openhab.ui/web/src/components/item/item-state-preview.vue Extended item type checks to include String items.
i18n files Updated translation keys to reflect the switch from “week” to “isoWeek”.

</thead>
<tbody>
<tr v-for="(options, item) in seriesOptions" :key="item">
<tr v-for="(options, idx) in seriesOptions" :key="idx">
Copy link

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

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

[nitpick] Using the array index as a key can lead to issues with list reordering; consider using a unique identifier from the series options if available.

Suggested change
<tr v-for="(options, idx) in seriesOptions" :key="idx">
<tr v-for="(options, idx) in seriesOptions" :key="options.id">

Copilot uses AI. Check for mistakes.
Dimmer can now be either a line/bar/area or state graph
Fixed xAxisIndex when both value and state graphs are displayed

Signed-off-by: Jeff James <[email protected]>
@jsjames jsjames force-pushed the analyzer-state-series branch from cd0649a to 90ee77b Compare June 28, 2025 23:51
@florian-h05 florian-h05 removed this from the 5.0 milestone Jul 13, 2025
@florian-h05
Copy link
Contributor

@jsjames Can you release this please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request main ui Main UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants