-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
[blockly] Persistence: Enhance existing blocks & add blocks to cover all available actions #2596
Conversation
#2083 Bundle Size — 10.68MiB (+0.15%).Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
Current #2083 |
Baseline #2082 |
|
---|---|---|
Initial JS | 1.88MiB |
1.88MiB |
Initial CSS | 607.91KiB |
607.91KiB |
Cache Invalidation | 18.2% |
24.45% |
Chunks | 223 |
223 |
Assets | 246 |
246 |
Modules | 2894 |
2894 |
Duplicate Modules | 149 |
149 |
Duplicate Code | 1.85% |
1.85% |
Packages | 97 |
97 |
Duplicate Packages | 2 |
2 |
Bundle size by type 1 change
1 regression
Current #2083 |
Baseline #2082 |
|
---|---|---|
JS | 8.86MiB (+0.19% ) |
8.85MiB |
CSS | 891.55KiB |
891.55KiB |
Fonts | 526.1KiB |
526.1KiB |
Media | 295.6KiB |
295.6KiB |
IMG | 140.74KiB |
140.74KiB |
HTML | 1.24KiB |
1.24KiB |
Other | 871B |
871B |
Bundle analysis report Branch mherwege:blockly_persistence Project dashboard
@stefan-hoehn Could you please help me with the shadow block issue? Apart from that, I think this is complete. @florian-h05 Would the approach I took here keep the backward compatibility as you had in mind? |
OK, I figured it out. It should work now. |
@mherwege First of all, I amazed that you provided this rather comprehensive change in blockly, so Kudos. Just to set expectations, I wasn't able to look into github the last week and I won't be able to review and test this in the next 3 weeks due to absence. |
@stefan-hoehn Thank you for the Kudos. No worries if this gets delayed. I guess it may have to wait for afte 4.2 in that case if @florian-h05 is not reviewing. But I am OK with that. |
2e1e67b
to
38e7ce5
Compare
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
38e7ce5
to
ad0f607
Compare
I understand the confusion. The oh_persist block as I designed it always has the state as the first field, also when persisting at current time or a series. Unless I make a separate field dor the time selection (which means I will have an extra line in the block when I persist at current time, making the block larger), that one dropdown will have to represent the meaning. Do you have a suggestion for a better label? |
I think it looks good and I am happy you provided it. I feel that persist value of item at time IMHO is okay and actually have no better idea atm. Therefore 🤘👍 from my side |
Thinking about it, maybe ‘state (at current time)’ and ‘state (at specified time)’ would be clearer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Wrt to the remark about the block:
I agree that it is not optimal, but due to lack of better ideas I will merge this now - the added value outweighs this minor remark.
Can you commit right now? If not, I can do it. |
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@mherwege I will push this minor change now so we can merge this and start the build soon. |
Thanks. |
Closes #2595.
This PR further extends the Blockly capabilities to include more of the persistence extensions.
Specifically enhanced the
oh_get_persistvalue
block:Between
methods of already existing methodscountSince
,countUntil
andcountBetween
countStateChangesSince
,countStateChangesUntil
andcountStateChangesBetween
getAllStatesSince
,getAllStatesUntil
,getAllStatesBetween
returning an Array of timestamp and state pairsString
,Number
,Quantity
,Timestamp
). These will be shown if relevant for the specific method (and limited to the relevant ones). The sorting of the options in the dropdown is such that the first in the list will generate code that is backward compatible. But it now allows to retrieve all of the return information, including for QuantityTypes. (only available for GraalJS)Added methods to the
oh_get_persistence_lastupdate
block (depends on: openhab/openhab-js#350)lastChange
,nextChange
Created a new block
oh_delete_persistedvalues
to delete persisted values:deleteAllStatesSince
,deleteAllStatesUntil
,deleteAllStatesBetween
Created a new block
oh_persist
to persist values:currentState
: persists a state at the current timestateAt
: persists a state to a given timestateList
: persists a TimeSeriesInitial testing shows this is generating the correct code.
EDIT: Resolved the issue below.
I have however identified one issue I was not able to resolve: depending on the persistence method, I need 0, 1 or 2 date fields. By default, there are only shadow blocks shown for the date field available when dropping the block on the canvas initially. As the first method in the list has only 1 date parameter, even when switching the method to one requiring 2, there is still only one shadow block shown. Switching to a method with 0 date field parameters and then back to a method with 1 or 2 even completely removes the shadow blocks. I don't know if and how this can be solved.
EDIT: As this is not being reviewed yet, I completed the development to also include the persistence actions that were not covered yet. I therefore created 2 extra blocks and extended the existing block.