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

Add a new settings menu for persistence #2229

Merged
merged 10 commits into from
Dec 29, 2023
Merged

Add a new settings menu for persistence #2229

merged 10 commits into from
Dec 29, 2023

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Dec 20, 2023 •
edited by florian-h05
Loading

Persistence policies can be configured from the UI. However, it is difficult to find, and one needs to know where to look. Apart from a documentation issue, I think persistence is core enough to merrit its own entry in the settings, so it becomes more prominent.

See for example here: https://community.openhab.org/t/after-reboot-string-values-are-empty/152064/16

This adds a persistence entry below transformations to the settings menu.

The new persistence config page allows to set the default persistence, and links to defining the policies for each installed persistence add-on.
There also is a "add" list entry to install more persistence add-ons.
If none is installed, it will explain persistence and link to the docs and the add-on store.

The default persistence settings has been removed from the system settings, and the link to the persistence policy config from the add-on settings page has been converted to a button.

@mherwege
sort settings
ecc65ea 
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
persistence menu
08bd6d0 
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
refine
47dc017 
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege requested a review from a team as a code owner December 20, 2023 09:32
@relativeci RelativeCI
Copy link

relativeci bot commented Dec 20, 2023 •
edited
Loading

Job #1337: Bundle Size — 15.84MiB (+0.52%).

6cb0d4e(current) vs b7270d2 main#1158(baseline)

Important

Bundle introduced 1 and removed 1 duplicate package – View changed duplicate packages

Warning

Bundle introduced 13 new packages: @jsep-plugin/regex, @jsep-plugin/arrow, @jsep-plugin/object and 10 more – View changed packages

Bundle metrics  Change 10 changes Regression 5 regressions Improvement 1 improvement
                 Current
Job #1337		      Baseline
Job #1158
Regression  Initial JS 1.9MiB(+13.83%) 1.67MiB
Regression  Initial CSS 609.75KiB(+0.14%) 608.89KiB
Change  Cache Invalidation 93.81% 93.95%
Change  Chunks 217(-0.91%) 219
Change  Assets 683(-0.87%) 689
Change  Modules 3036(+78.59%) 1700
Regression  Duplicate Modules 170(+88.89%) 90
Improvement  Duplicate Code 1.6%(-17.95%) 1.95%
Regression  Packages 152(+10.14%) 138
Regression  Duplicate Packages 16(+6.67%) 15
Bundle size by type  Change 3 changes Regression 3 regressions
                 Current
Job #1337		      Baseline
Job #1158
Regression  JS 9.31MiB (+0.6%) 9.25MiB
Regression  Other 4.75MiB (+0.53%) 4.73MiB
Regression  CSS 861.19KiB (+0.2%) 859.49KiB
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.23KiB 1.23KiB

View job #1337 report View mherwege:settings branch activity

@hmerk
Copy link

hmerk commented Dec 24, 2023 •
edited
Loading

None of the previously defined links were removed, as it makes sense to be able to access it from multiple places.

While I second your PR, I disagree on not removing the existing persistence link. As both are shown, it is a little bit irritating, which one to use. Therefore I vote to remove the old link under system settings.

@mherwege
Copy link
Contributor Author

@hmerk Fine with removing it from the systems settings menu if nobody else objects. What about the link at the top of the persistence add-on?

@hmerk
Copy link

hmerk commented Dec 24, 2023

If it is easier to access via your new menu, it could be removed as well.
Right now it just looks like s headline...

@mherwege
remove persistence system settings
2f616d4 
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
Copy link
Contributor Author

If it is easier to access via your new menu, it could be removed as well. Right now it just looks like s headline...

I think it is easy to access from the new menu, but I also think it still has its place in the add-on configuration screen. Configuring persistence kind of falls in between configuring an add-on and a system wide configuration. And I rather have 2 places with the option, so a user finds it where they are looking for it. Therefore I changed the link to a button there, making it more obvious.
I did remove the persistence system setting menu.

@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels Dec 26, 2023
@florian-h05
Remove space between transformations and persistence settings entries
d0469cd 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Update empty state placeholder wording
4e60d1c 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Use redirect for documentation link
61141f1 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Fix dirty check not working
76520ab 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Minor refactoring
eab36d2 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Align style to MainUI standards & Replace + button
6cb0d4e 
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
florian-h05
florian-h05 approved these changes Dec 29, 2023
View reviewed changes
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.

LGTM, thanks!
Good to give the persistence configuration more visibility.

I have changed a few things:

  • Remove space between transformations and persistence setting entries. The space above the transformations is meant to separate 1st- from 2nd-level configuration.
  • Update the empty state placeholder wording.
  • Use a redirect link for the documentation button. This way we can easily adjust where this button links to without having to change the code.
  • Fix dirty check (alert when unsaved changed on leave) not working.
  • Minor code refactoring in the load method. From my POV there is no catch required as no 404 should occur because the org.openhab.persistence service should always be available.
  • Align the style to general MainUI styling & Replace the plus button with an "add" list entry or a new button on the empty state placeholder:

image

image

@florian-h05 florian-h05 added this to the 4.2 milestone Dec 29, 2023
@florian-h05 florian-h05 changed the title Settings menu for persistence Add a new settings menu for persistence Dec 29, 2023
@florian-h05 florian-h05 merged commit 2fe6ae9 into openhab:main Dec 29, 2023
6 checks passed
@mherwege
Copy link
Contributor Author

Thanks.

@mherwege mherwege deleted the settings branch December 30, 2023 08:20
@florian-h05 florian-h05 mentioned this pull request Dec 30, 2023
Merged
florian-h05 added a commit that referenced this pull request Dec 30, 2023
@florian-h05
Settings pages: Fix x-overflow and large spaces on phone screens & Ch…
bf2f257 
…ange icon for persistence config (#2248)

Follow-up for #2229.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florian-h05 florian-h05 florian-h05 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
3 participants
@mherwege @hmerk @florian-h05