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

Remove "Other Services" section from main settings page #1891

Merged
merged 2 commits into from Jun 4, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented May 12, 2023

Add-on service settings are configurable from the respective add-on's page and therefore the "Other Services" section of the main settings page is not needed anymore.
Services provided by openHAB core that were previously listed unter "Other Services" were moved to "System Services".

Persistence services are now configure from the respective add-ons page.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K requested a review from a team as a code owner May 12, 2023 18:51
@relativeci
Copy link

relativeci bot commented May 12, 2023

Job #1018: Bundle Size — 15.77MiB (~-0.01%).

65e8372(current) vs 101d223 main#1017(baseline)

⚠️ Bundle contains 16 duplicate packages

Metrics (no changes)
                 Current
Job #1018
     Baseline
Job #1017
Initial JS 1.73MiB 1.73MiB
Initial CSS 608.89KiB 608.89KiB
Cache Invalidation 93.95% 93.95%
Chunks 219 219
Assets 689 689
Modules 1720 1720
Duplicate Modules 85 85
Duplicate Code 1.72% 1.72%
Packages 138 138
Duplicate Packages 15 15
Total size by type (2 changes)
                 Current
Job #1018
     Baseline
Job #1017
CSS 858.62KiB 858.62KiB
Fonts 526.1KiB 526.1KiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.26MiB (~-0.01%) 9.26MiB
Media 295.6KiB 295.6KiB
Other 4.73MiB (~-0.01%) 4.73MiB

View job #1018 reportView main branch activity

@florian-h05
Copy link
Contributor

I wonder how we should handle the settings for other addons, because those are also reachable from the addon page. The “problem” I see with removing them here, is that accessing them via the addon page are several clicks more.

@J-N-K
Copy link
Member Author

J-N-K commented May 13, 2023

True, there are some more clicks. On the other hand it is far more consistent, since - as you mentioned - other add-ons are also configured from their add-on page.

I think removing them from the right columns was also the result of the discussion in #1463 (e.g. #1463 (comment)).

@florian-h05
Copy link
Contributor

On the other hand it is far more consistent, since - as you mentioned - other add-ons are also configured from their add-on page.

I meant that when persistence add-ons are removed from the "Other Services" section other add-ons are still there, however reading your linked post, I wonder if the other services section should be removed completely. Everything that can be configured there can be done through the add-on page.

@J-N-K
Copy link
Member Author

J-N-K commented May 13, 2023

In general: yes. But for some reason it is currently not possible to configure e.g. basicui or javascript scripting from the add-ons page, I guess the config-description URI is probably not correctly set (confirmed: openhab/openhab-addons#14985)

There are also other setting like the LSP that don't have an add-on page.

@lolodomo
Copy link
Contributor

lolodomo commented May 13, 2023

Take care before merging. I don't know if this is the same for persistence services, but I see problems for openhabcloud and voiceRSS when I go to the setting from the addon page:

For openhab cloud, no settings are shown in bindings/addons page:
image

For VoiceRSS, the setting is shown but the value is not filled (while it is from the main settings):
image

I will check with a next snapshot if the problem is solved for BasicUI.

So, for persistence services, please check that it is working from addons before removing the access from the main settings.

@J-N-K
Copy link
Member Author

J-N-K commented May 13, 2023

It works with InfluxDB, and with the fix I just provided for JDBC.

@lolodomo
Copy link
Contributor

It works with InfluxDB, and with the fix I just provided for JDBC.

Fine ;)

Even if I am a little off-topic, maybe you have an idea why it does not work for openhab cloud and voicerss ?

@J-N-K
Copy link
Member Author

J-N-K commented May 13, 2023

I also checked DynamoDB, JPA and MongoDB and they work. RRD4J has no configuration.

@J-N-K
Copy link
Member Author

J-N-K commented May 13, 2023

For openhabcloud it's due to the issue that was fixed today by @holgerfriedrich: the config-description-uri was wrong. For voicerss the service-id is wrong.

@florian-h05
Copy link
Contributor

@J-N-K What is the state here?

@lolodomo
Copy link
Contributor

I checked with success configuration access from add-on page for openHAB cloud, BasicUI, VoiceRSS and Vosk.

@J-N-K
Copy link
Member Author

J-N-K commented May 22, 2023

IMO persistence services are fine, I did not check the others.

@florian-h05
Copy link
Contributor

I've checked IO (openHAB Cloud), UI (BasicUI), MISC (Metrics Service), automation (JS Scripting).
Problematic is the rule-based speech intepreter (org.openhab.rulehli) because is is provided by core and therefore no add-on, but has the category voice. Even though it belongs to voice, it should be changed to misc or system because it is provided by openHAB core.

@@ -201,7 +201,7 @@ export default {
// can be done in parallel!
servicesPromise.then((data) => {
this.systemServices = data.filter(s => s.category === 'system')
this.otherServices = data.filter(s => s.category !== 'system')
this.otherServices = data.filter(s => s.category !== 'system' && s.category !== 'persistence')
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI I tested this to filter out all add-ons:

Suggested change
this.otherServices = data.filter(s => s.category !== 'system' && s.category !== 'persistence')
this.otherServices = data.filter(s => !['system', 'automation', 'bindings', 'io', 'persistence', 'transformations', 'ui', 'voice'].includes(s.category))

Copy link
Member Author

Choose a reason for hiding this comment

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

Problematic is the rule-based speech intepreter (org.openhab.rulehli) because is is provided by core and therefore no add-on, but has the category voice. Even though it belongs to voice, it should be changed to misc or system because it is provided by openHAB core.

I agree. I also think that LSP should be system, because that is probably the only non-add-on service left.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make an exception for LSP and the rule-based speech intepreter and keep them here in MainUI without breaking their settings in existing setups?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the rule-based speech intepreter could become an add-on?

Copy link
Contributor

Choose a reason for hiding this comment

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

LSP and rule-based speech intepreter are the only non-add-on services left (AFAIK), for LSP I agree that it should be system and for rule-bases sp...ter I would say either system or misc. These two options would make keep them visible in MainUI.

Rule-based sp..ter would be in the system settings section with system or the only one in the Other Services section with misc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing the category will not break the settings. I'm just not sure if changing the translations works the way I have done it, see openhab/openhab-core#3625

Copy link
Contributor

@lolodomo lolodomo May 23, 2023

Choose a reason for hiding this comment

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

The most important is to not loose settings in existing user setups.
Then showing them in system or other services is a detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the translations: The main properties file should work this way I think, but as translations shouldn’t be done through the files (IIRC) I guess they need to be uploaded to CrowdIn.

@J-N-K
Copy link
Member Author

J-N-K commented May 30, 2023

How do we proceed here?

@florian-h05
Copy link
Contributor

I haven't checked whether Rule HLI and LSP are now displayed under the "System Settings" tab, but if they now are I'd finally remove the "Other Services" section.

@florian-h05
Copy link
Contributor

@J-N-K Do you want to contribute in this PR? I could review and merge …

Signed-off-by: Jan N. Klug <github@klug.nrw>
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!

@florian-h05 florian-h05 changed the title Remove persistence services from Main-Settings page Remove "Other Services" section from main settings page Jun 4, 2023
@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels Jun 4, 2023
@florian-h05 florian-h05 added this to the 4.0 milestone Jun 4, 2023
@florian-h05 florian-h05 merged commit a8ef9cc into openhab:main Jun 4, 2023
5 checks passed
@ghys
Copy link
Member

ghys commented Jun 13, 2023

Just to defend my original design, consider this:

On an iPhone - and half of the openHAB userbase have iPhones if we look at analytics on the website, so it's not that we want to copycat its design, but it can help for users familiar with the design to figure out where things are - you have your App Store and you have your Settings app.

Installed apps add entries to the Settings app, below the system options:

The "Other Services" section was meant to be that, a section where individual add-ons could have their entries for (OSGi configurable) services they inject into the framework.
On an iPhone, you don't go to the App Store to configure your app, you expect to find it in the Settings.

@J-N-K
Copy link
Member Author

J-N-K commented Jun 13, 2023

I get you point, but it seems a bit awkward and hard to understand that you configure a persistence (or IO) add-on in one place (on the main setting page) and it's logging at another place (on the add-on page) while both settings are in the same place (the add-on page) for bindings.

@J-N-K J-N-K deleted the persistence branch June 13, 2023 16:04
@florian-h05
Copy link
Contributor

As an iPhone user myself I have already noticed the similarities your original design Yannick, however I agree that it is a bit confusing to have the settings split over a few places.
Unfortunately, most iOS apps have settings inside the app besides in the system Settings, so having to configure the addon from the addon‘s page is no real problem IMO.

@ghys
Copy link
Member

ghys commented Jun 13, 2023

I'm not really arguing, just explaining 🤗 .
In fact maybe having the add-on's (formerly only binding's) configuration in the add-on store was a bad idea after all, as we could just have add-ons add entries to the settings menu like on iOS. So you would have e.g. "Philips Hue" directly under Settings and users wouldn't have to look for it in the add-on store, which was in the end really meant for browsing, installing and uninstalling, nothing more.
As for the logging, in fact it could have been a single consolidated page where you would configure levels for all loggers at once.

The removal of "Other Services" however offers an opportunity to adjust the Settings menu to keep it balanced when presented on two columns, as we could layout it like this:

  • Left column
    • 1st-tier Configuration: Things, Model, Items, Pages
    • 2nd-tier Configuration: Persistence, Tags (?), Transformations - no need to have a section header title for those, just separate them from the 1st tier. There would be room for more, for instance "Metadata Namespaces" (allow the user to define their own namespaces).
    • Automation: Rules, Scenes, Scripts, Schedule
  • Right column
    • Add-ons: Bindings, Automation, UI, Other
    • Services (formerly System Services)

@florian-h05
Copy link
Contributor

Sound like a great idea!

I have one comment:

2nd-tier Configuration: Persistence, ...

I'd probably keep persistence out of the main settings and only have it under the add-on settings in the right column.

Could you probably open a PR so we can have a look?

@florian-h05
Copy link
Contributor

@ghys I have opened a new issue to discuss and coordinate the redesing of add-on configuration, see #1935.

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.

None yet

4 participants