search

eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 787 forks source link

REST API: (Optionally) include hidden widgets in sitemap API response #6856

Closed maniac103 closed 5 years ago

maniac103 commented 5 years ago

Brings REST API response in line with SSE events, so that clients have a full view of all widgets which they can later update via SSE.

See https://github.com/openhab/openhab-android/issues/1094 for the discussion which triggered this PR.

maniac103 commented 5 years ago

One thing I wasn't sure is whether the query parameter is needed or whether we can do the behavior change unconditionally?

lolodomo commented 5 years ago

Be sure to not break neither Basic UI (SSE mode ot Edge mode) nor Classic UI.

maniac103 commented 5 years ago

Does either of them use the REST API? As in the current version of the patch the default behavior is unchanged, I don't see how they could break either way.

lolodomo commented 5 years ago

Yes all UI use the sitemap REST API.

Where is defined the default value of your new parameter ?

Why is there a new visibility field in WidgetDTO ? Apparently not set.

maniac103 commented 5 years ago

Where is defined the default value of your new parameter ?

The default should be false, as that's the initialization value when declaring a boolean (see https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html).

Why is there a new visibility field in WidgetDTO ? Apparently not set.

It's set here: https://github.com/eclipse/smarthome/pull/6856/files#diff-bc1e5e7e0e70cf1d2ea48adce29d293eR511 As for the 'why', I thought that's obvious ;-) It distinguishes hidden from visible widgets and equals the 'visibility' field in the SSE event.

lolodomo commented 5 years ago

LGTM

Did you test (chart switch) that Classic UI is still working ? Same question for Basic UI (SSE mode and Edge mide)?

maniac103 commented 5 years ago

Did you test (chart switch) that Classic UI is still working ?

Can you point me to where Classic UI uses the REST API? As far as I can tell from looking into the org.eclipse.smarthome.ui.classic package, classic UI HTML is rendered server-side and REST API isn't involved.

Same question for Basic UI (SSE mode and Edge mide)?

Basic UI seems to use REST API only in Edge mode (in SSE mode only the subscription endpoint is used), but as mentioned, I have kept the default behavior intact, so as long as the includeHidden query parameter isn't set, the only behavior change caused by this PR is the inclusion of the JSON member 'visibility' with a value of 'true' for each widget. I've verified this doesn't disturb BasicUI by running it after forcing it into Edge mode (set featureSupport.eventSource to false).

lolodomo commented 5 years ago

You're right, I mix up different UIs now ! it is Android app that is using the sitempa REST API ! Classic UI and Basic UI have a servlet on the server that use the internal UI registry. These servlets were probably not filtering the hidden widgets.

So for me, your change is OK

kaikreuzer commented 5 years ago

Thanks @maniac103! Please note that you have missed adapting the unit tests, so this cannot be merged here. I have ported this PR to https://github.com/openhab/openhab-core/pull/499 and fixed the tests, so we can simply close this PR here.