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

Script & Item edit: Add/Fix dirty handling #2374

Merged
merged 7 commits into from Feb 23, 2024
Merged

Script & Item edit: Add/Fix dirty handling #2374

merged 7 commits into from Feb 23, 2024

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Feb 18, 2024 •
edited

  • Add dirty handling for script-edit.
  • Fix dirty handling of item-edit always saying dirty due to wrong initialization order.
  • Code improvements to other dirty handlings.

@florian-h05 florian-h05 requested a review from a team as a code owner February 18, 2024 12:40
@florian-h05
Copy link
Contributor Author

@jimtng Can you please test this PR?

@florian-h05 florian-h05 changed the title Script edit: Add dirty handling & Code improvements to other dirty handling Script edit: Add dirty handling & Item edit: Fix dirty handling Feb 18, 2024
@florian-h05 florian-h05 changed the title Script edit: Add dirty handling & Item edit: Fix dirty handling Script & Item edit: Add/Fix dirty handling Feb 18, 2024
@relativeci RelativeCI
Copy link

relativeci bot commented Feb 18, 2024 •
edited

Job #1651: Bundle Size — 11.01MiB (+0.02%).

b5bd854(current) vs a1f5091 main#1646(baseline)

Warning

Bundle contains 19 duplicate packages – View duplicate packages

Bundle metrics  Change 1 change
                 Current
Job #1651		      Baseline
Job #1646
No change  Initial JS 1.85MiB 1.85MiB
No change  Initial CSS 607.89KiB 607.89KiB
Change  Cache Invalidation 17.59% 22.24%
No change  Chunks 220 220
No change  Assets 242 242
No change  Modules 3088 3088
No change  Duplicate Modules 164 164
No change  Duplicate Code 1.77% 1.77%
No change  Packages 150 150
No change  Duplicate Packages 18 18
Bundle size by type  Change 1 change Regression 1 regression
                 Current
Job #1651		      Baseline
Job #1646
Regression  JS 9.2MiB (+0.02%) 9.2MiB
Not changed  CSS 889.44KiB 889.44KiB
Not changed  Fonts 526.1KiB 526.1KiB
Not changed  Media 295.6KiB 295.6KiB
Not changed  IMG 140.74KiB 140.74KiB
Not changed  HTML 1.24KiB 1.24KiB
Not changed  Other 871B 871B

View job #1651 report View florian-h05:dirty branch activity View project dashboard

@jimtng
Copy link
Contributor

jimtng commented Feb 18, 2024

This doesn't seem to check whether a rule module is dirty when the Back link is clicked. This is the screen I'm referring to:

Rules -> select a rule -> Select an existing module (trigger / action / condition) but other than a script. Make changes in this screen then click Back (not Save). It should prompt when dirty.

image

@florian-h05
Copy link
Contributor Author

florian-h05 commented Feb 18, 2024 •
edited

This doesn't seem to check whether a rule module is dirty when the Back link is clicked.

I haven't worked on this, glad that you reminded me that this probably also need dirty handling.
However this might be a bit more complicated, IMO this should be done in a different PR.
This PR focuses on the dirty handling of all pages, therefore I only worked on script-edit and item-edit.

@florian-h05
Copy link
Contributor Author

@jimtng FYI I have edited my last comment, what you tested is out of scope of this PR as I only focused on the actual pages, not dirty handling for popups.

@jimtng
Copy link
Contributor

jimtng commented Feb 18, 2024

@jimtng FYI I have edited my last comment, what you tested is out of scope of this PR as I only focused on the actual pages, not dirty handling for popups.

Fair enough. I wasn't aware that the other areas needed fixing (they of course did, evidenced by this PR). The popup was what was on my mind :)

@florian-h05
Copy link
Contributor Author

@jimtng It now became a bit bigger than expected, but now it works fine in my tests.
Can you please re-test the script edit dirty handling?

jimtng
jimtng reviewed Feb 18, 2024
View reviewed changes
Copy link
Contributor

@jimtng jimtng left a comment

Choose a reason for hiding this comment

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

What I found so far:

  • Run Now will forcibly save current changes without prompting/warning
  • After Run Now, edit the script again. This should make it dirty but it doesn't warn about the new changes when clicking Back out of the script editor. The changes made after clicking Run Now is lost!

bundles/org.openhab.ui/web/src/pages/settings/rules/script/script-edit.vue Outdated Show resolved Hide resolved
@florian-h05
Copy link
Contributor Author

I cannot reproduce that issue, for me this works fine.

@florian-h05
Various code improvements
94af196 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Fix module label & description cannot be set
fc1f184 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Add dirty handling to script edit
5775f35 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Minor enhancements
8c43bbd 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@jimtng
Copy link
Contributor

jimtng commented Feb 23, 2024

What I found so far:

  • Run Now will forcibly save current changes without prompting/warning

This is still the case. Shouldn't we prompt user whether they wanted to save first before running?

  • After Run Now, edit the script again. This should make it dirty but it doesn't warn about the new changes when clicking Back out of the script editor. The changes made after clicking Run Now is lost!

It seems that this is not the case now.

@florian-h05
Copy link
Contributor Author

Shouldn't we prompt user whether they wanted to save first before running?

I would tent to say no, because I guess when you click "Run Now" you want to run what you have actually written, and not what is the saved state on the server. WDYT?

@jimtng
Copy link
Contributor

jimtng commented Feb 23, 2024

I would like to be be prompted before it saves my changes.

@florian-h05
Ask whether to save if dirty on run now
ed62965 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Fix styling of script-general-settings
71a7a0f 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

I have added a dialog for that.
From my POV we are ready to merge, WDYT?

@jimtng
Copy link
Contributor

jimtng commented Feb 23, 2024

I don't like the current behaviour.

The prompt should say

Changes not saved yet
Do you want to save the changes before running the script?
[Cancel] [Save]

When cancel is clicked, don't run.

Right now it runs the old script prior to editing, which is not what I see on the screen, and that, to me, is confusing.

@florian-h05
Update dialog
b5bd854 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

Pushed 👍

@jimtng
Copy link
Contributor

jimtng commented Feb 23, 2024

Perfect!

I've just noticed something - which is happening even before this PR:

  • Edit a rule
  • Edit a script module (e.g. action)
  • Make changes in the code editor
  • Click Back - you'll get dirty prompt, select Cancel to get back to the script editor
  • Click Save
  • Now click Back

This broke the address bar and the UI forcing me to do a page refresh to recover. Does this happen to you?

@florian-h05
Copy link
Contributor Author

Does this happen to you?

Yes.

I will merge this PR as it is unrelated to it, but it should be taken care of that.

@florian-h05 florian-h05 merged commit def6eff into openhab:main Feb 23, 2024
6 checks passed
@florian-h05 florian-h05 deleted the dirty branch February 23, 2024 16:48
@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI bug Something isn't working and removed enhancement New feature or request labels Feb 23, 2024
@florian-h05 florian-h05 added this to the 4.2 milestone Feb 23, 2024
@jimtng
Copy link
Contributor

jimtng commented Feb 24, 2024

@florian-h05 I've just tried this:

  • View an item
  • Click edit, make no change
  • Click Back - I got the dirty warning.

@florian-h05
Copy link
Contributor Author

I cannot reproduce this with the dev server on main (commit 69f4876).

This was referenced Mar 3, 2024
Closed
Merged
kaikreuzer pushed a commit that referenced this pull request Mar 3, 2024
@jimtng
Script Editor: Fix script loading (#2442)
0f343f3 
Regression from #2374

The problem is when I clicked on a file-based script in the UI, the
content of the script didn't load.

Before:
<img width="601" alt="image"
src="https://github.com/openhab/openhab-webui/assets/2554958/b3351f27-8ae6-4463-91ee-583fce0cf873">


After:
<img width="670" alt="image"
src="https://github.com/openhab/openhab-webui/assets/2554958/596629cb-459d-43c0-992e-7221e2a505dd">

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimtng jimtng jimtng left review comments

Assignees
No one assigned
Labels
bug Something isn't working main ui Main UI
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@florian-h05 @jimtng