Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

[Core] Fixed wrong order of widgets after sitemap changes #5523

Merged
merged 3 commits into from May 3, 2018
Merged

[Core] Fixed wrong order of widgets after sitemap changes #5523

merged 3 commits into from May 3, 2018

Conversation

sjsf
Copy link
Contributor

@sjsf sjsf commented May 2, 2018

This partially reverts #3846.

fixes #5522
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

@sjsf
refresh internal widget collection on sitemap change
0b03add 
This partially reverts eclipse-archived#3846.

fixes eclipse-archived#5522
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf sjsf added the UI label May 2, 2018
@sjsf
untangle model update notifiaction order
0a46773 
The SitemapSubscriptionService now gets informed by the SitemapProvider
about a model change.

Before, it was a matter of luck if the SitemapSubscriptionService or the
SitemapProvider got informed first about a model change by the
ModelRepository. If the SitemapSubscriptionService was so unlucky to be
the first one, it got a stale model from the SitemapProvider and hence
it again the calculation of the widget IDs was wrong.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf
Copy link
Contributor Author

sjsf commented May 2, 2018

While doing some manual testing, I realized that there is another potential source for wrong widget ID calculations: The SitemapSubscriptionService and the SitemapProvider both registered themselves as ModelRepositoryChangeListeners at the ModelRepository. The former uses the latter to get a cached instance of the model.
There is no guarantee though about the order in which they get their modelChanged() methods called. So in case the SitemapSubscriptionService got called first, it used the stale widget copy given by the SitemapProvider, which can't know anything about the new model at this time.
I have added a second commit which fixes this aspect by letting the SitemapSubscriptionService subscribe at the SitemapProvider for changes instead of the ModelRepository directly, thereby making sure it only gets notified when its trusted provider also knows about the change.

@sjsf sjsf mentioned this pull request May 2, 2018
Closed
triller-telekom
triller-telekom reviewed May 3, 2018
View reviewed changes
Copy link
Contributor

@triller-telekom triller-telekom left a comment •
edited

Choose a reason for hiding this comment

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

Thank you for this bugfix. I can confirm that I can now change the sitemap on a running system and afterwards the assignments of widgets to items is correct again.

While implementing the sitemap reload feature I accidentally removed these two calls:

collectWidgets(sitemapName, pageId);
updateItemsAndWidgets(widgets);

which are necessary.

One small comment regarding the use of the public keyword on interfaces.

...eclipse.smarthome.model.sitemap/src/org/eclipse/smarthome/model/sitemap/SitemapProvider.java Outdated
*
* @param listener
*/
public void addModelChangeListener(ModelRepositoryChangeListener listener);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the other methods in this interface also have the public keyword, but they are superfluous in an interface...

@sjsf
removed public modifiers from SitemapProvider interface
19a6525 
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
triller-telekom
triller-telekom approved these changes May 3, 2018
View reviewed changes
Copy link
Contributor

@triller-telekom triller-telekom left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM now.

@kaikreuzer kaikreuzer merged commit 379069a into eclipse-archived:master May 3, 2018
@sjsf sjsf deleted the fixWidgetReordering branch May 4, 2018 07:28
@kaikreuzer kaikreuzer changed the title refresh internal widget collection on sitemap change [Core] Fixed wrong order of widgets after sitemap changes May 20, 2018
@kaikreuzer kaikreuzer added the bug label May 20, 2018
ermartens pushed a commit to ermartens/smarthome that referenced this pull request Jun 15, 2018
@sjsf
refresh internal widget collection on sitemap change (eclipse-archive…
4e80a82 
…d#5523)

* refresh internal widget collection on sitemap change
* removed public modifiers from SitemapProvider interface
* untangle model update notifiaction order

The SitemapSubscriptionService now gets informed by the SitemapProvider
about a model change.

Before, it was a matter of luck if the SitemapSubscriptionService or the
SitemapProvider got informed first about a model change by the
ModelRepository. If the SitemapSubscriptionService was so unlucky to be
the first one, it got a stale model from the SitemapProvider and hence
it again the calculation of the widget IDs was wrong.

This partially reverts eclipse-archived#3846.
fixes eclipse-archived#5522
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@htreu htreu added this to the 0.10.0 milestone Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@triller-telekom triller-telekom triller-telekom approved these changes

Assignees
No one assigned
Labels
APIBreaking bug UI
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.

Sitemap updates the wrong widgets after reordering
4 participants
@sjsf @triller-telekom @htreu @kaikreuzer