openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
923 stars 424 forks source link

Sitemap event subscription for whole sitemap instead of one page only #3650

Closed TAKeanice closed 5 months ago

TAKeanice commented 1 year ago

Motivation:

I am trying to implement a display based on sitemaps (improving upon the m5panel project). As my display interprets the sitemap slightly different than the BasicUI, and in general, different displays might interpret sitemaps in different ways, the subscription mechanism does not fit: An implementation for TV-sized screens may want to display all of the pages of a sitemap at once, while a small mobile display like the m5panel does not want to issue a new request on each of the frequently occuring navigations, but rather show the page navigated to with the up-to-date widgets straight away.

Current situation:

It took me quite a while to figure out how the subscriptions work for the basic ui: The backend is involved in changing the page of the subscription on navigation to another page. This implies, that at all times only one page can be visible and kept up to date. Subscriptions are currently limited to a single page per event stream, and an alternative display implementation would have to open one subscription for each page it wants to keep up to date.

Proposed change:

Allow subscriptions for all pages of a sitemap at once (i.e., send updates for all widgets in the whole sitemap instead of only the widgets on a single page)

Implementation option:

https://github.com/openhab/openhab-core/pull/3635 already refactors some of the SitemapSubscriptionService and the SitemapResource. Maybe it´s possible for @J-N-K to increase the scope of the PR while he´s at it? Otherwise, I am open to contributing a change myself. Please let me know :)

TheTrueRandom commented 1 year ago

@TAKeanice Maybe for your usecase it could even make sense to subscribe to the websocket stream and filter the events based on topics you are interested in. From my experience it is really fast and easy to work with.

TAKeanice commented 1 year ago

@TheTrueRandom Thank you for the suggestion. I would prefer to stick with the Sitemap events though, since the format is very similar to the format in which I get the sitemap itself (a json with information about the widget and the item, not all of the fields but the relevant ones), and the events already come in filtered. Also, the http protocol is easier to handle than sockets.

lolodomo commented 1 year ago

Allow subscriptions for all pages of a sitemap at once (i.e., send updates for all widgets in the whole sitemap instead of only the widgets on a single page)

I am not sure it makes sense. A sitemap could contain a very big number of pages, in particular due to the item groups.

J-N-K commented 1 year ago

I tend to agree with @lolodomo. What is the use-case? Maybe I didn't understand it.

TAKeanice commented 1 year ago

@J-N-K The usecase is a display that for some reason needs to update all the widgets within a Sitemap when they change, not only the ones on one particular page.

TAKeanice commented 1 year ago

I am not sure it makes sense. A sitemap could contain a very big number of pages, in particular due to the item groups.

I think it's just a question of where the effort of mapping events to widgets on the Sitemap should take place: on the server or the display. I think having the choice is worth it.

J-N-K commented 1 year ago

The problem I see is (like @lolodomo) that this likely will have performance issues. We'll then face the discussion why that is and demand to "fix it". Similar to the discussion in #3648 where something that was never designed the way it is used now needs to be "fixed" because someone uses it wrong.

TAKeanice commented 1 year ago

I think the discussion there was about the documentation, not the implementation. But ok, so I guess it is up to me to provide an implementation and see how to make it perform well. When I am ready, may I ask for help in performance testing?

lolodomo commented 1 year ago

I don't understand the need. What UI will display all sitemap pages at the same time??

TAKeanice commented 1 year ago

I don't understand the need. What UI will display all sitemap pages at the same time??

I already gave an example, a TV-sized screen might want to do something like that. Even a desktop browser could benefit from a somewhat different representation of a sitemap, which does not require as much navigation as the current implementation of BasicUI. The current implementation of the subscription api and BasicUI is geared towards smartphone-sized displays, and to a workflow where it is ok to freshly load the state of all widgets on a page when navigating there. The rather small and fully backwards compatible code changes I made in https://github.com/openhab/openhab-core/pull/3652, that do not alter any of the current use cases, just allow for a little bit more flexibility and choice in how to implement a sitemap display.

lolodomo commented 1 year ago

If you want to display several pages, you can already subscribe to each of them, this is what is doing the Android app. In case you have a big screen and you want to display 4 pages for example, why not simply create a subscription for each of them ?

Even with a simple sitemap and group items, you could probably quickly reach 100 pages for a sitemap. I could count it with my own sitemap. Did you count it for your sitemap ?

TAKeanice commented 1 year ago

Yes, it is considerably smaller, in the range 10-20. Is one subscription for each page really more efficient than subscribing to the whole sitemap? Can't we let the developers choose?

rkoshak commented 1 year ago

I think the discussion there was about the documentation, not the implementation.

No, that was a bug. It was just a bug in another repo instead of where the it was reported. Issue was filed.

TAKeanice commented 1 year ago

I don't know if you've noticed, so don't feel pushed, but I prepared a pull request with an implementation of what I am asking for here. I also asked for advice on how to test the PR, because I have never built and deployed openHAB to my rasperry pi before.

openhab-bot commented 1 year ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/m5paper-interface-panel/110902/67

TAKeanice commented 1 year ago

I know there is a lot going on and maintaining open source projects like this one takes a large amount of time. Nevertheless, I want to make one more attempt to make my call for help on testing and performance testing heard here.

As a maintainer and someone who already did implementations which improved sitemap performance, @J-N-K , would it be feasible for you to provide a small integration test skeleton on https://github.com/openhab/openhab-core/pull/3652 (you should even be able to directly push onto the branch)? I just don´t know how to get started with an integration test on sitemaps. I would then fill that skeleton and test whether I implemented everything correctly. Hopefully I will also be able see how much of a performance impact my change could have.

Another way I could imagine to test this is to build a version of OpenHab myself and deploy it to my setup. If you think this is the better route, could you point me to instructions on how to compile openhab and swap it out on an openhabian raspberry?

lolodomo commented 5 months ago

@J-N-K : to be closed.