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

Reload BasicUI if sitemap has been changed #3846

Merged

Conversation

triller-telekom
Copy link
Contributor

This catches the IndexOutofBoundException from #3836

Signed-off-by: Stefan Triller stefan.triller@telekom.de

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@@ -1709,6 +1709,16 @@
value,
title;

if(data.TYPE === "SITEMAP_CHANGED") {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to your formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing :) Grunt expects this format or it complains and does not generate the minified version of the file

@@ -515,7 +515,7 @@ public Widget getWidget(Sitemap sitemap, String id) {
for (int i = 2; i < id.length(); i += 2) {
w = ((LinkableWidget) w).getChildren().get(Integer.valueOf(id.substring(i, i + 2)));
}
} catch (NumberFormatException e) {
} catch (NumberFormatException | IndexOutOfBoundsException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather check two lines above if the value is within the bounds. That's cheaper than the control flow by the IOOBE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I will change it. Though the NumberFormatException was caught here I thought it doesn't hurt to catch the other one too...

@@ -1709,6 +1709,16 @@
value,
title;

if(data.TYPE === "SITEMAP_CHANGED") {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it also necessary to check if it is this sitemap which changed? I.e. have you tried having to browser windows open with different sitemaps and check that only the affected one gets redirected to its main page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While fixing this bug I noticed that its almost impossible to detect in which page the user currently is if the the order of the pages has been changed. Thats why there is a reload for every sitemap change, also for browsers which are on the first page. Just replacing the widgets would have let to inconsistent views in the browser. This solution is now much cleaner.

updateItemsAndWidgets(widgets);
public void sitemapContentChanged() {
for (SitemapSubscriptionCallback callback : distinctCallbacks) {
SitemapChangedEvent changeEvent = new SitemapChangedEvent();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really make sense to create a new event for each callback and not create it once and send it to all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, creating one event is sufficient.

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@sjsf
Copy link
Contributor

sjsf commented Jul 18, 2017

okay from my side.
@resetnow your call.

window.location.href = parts[0] + "?sitemap="+data.sitemapName;
} else {
window.location.href = oldLocation;
}
Copy link
Contributor

Choose a reason for hiding this comment

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


_t.pause();
return;

var oldLocation = window.location.href;
var parts = oldLocation.split("?");
if (parts.length > 1) {
window.location.href = parts[0] + "?sitemap="+data.sitemapName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Spaces:

"?sitemap=" + data

if (parts.length > 1) {
window.location.href = parts[0] + "?sitemap="+data.sitemapName;
} else {
window.location.href = oldLocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

window.location.reload(true);

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@sjsf sjsf merged commit c555c35 into eclipse-archived:master Jul 18, 2017
@triller-telekom triller-telekom deleted the basicUIsitemapBrowserReload branch July 18, 2017 15:10
@kaikreuzer
Copy link
Contributor

This catches the IndexOutofBoundException from #3836

Does it mean that #3836 is hence fixed now and can be closed?

@triller-telekom
Copy link
Contributor Author

Yes it can be closes because the sitemap problem is solved and the other problem regarding the non working D-Link camera has been extracted into #3870

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
@kaikreuzer kaikreuzer added the bug label Dec 15, 2017
sjsf added a commit to sjsf/smarthome that referenced this pull request May 2, 2018
This partially reverts eclipse-archived#3846.

fixes eclipse-archived#5522
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
kaikreuzer pushed a commit that referenced this pull request May 3, 2018
* 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 #3846.
fixes #5522
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
ermartens pushed a commit to ermartens/smarthome that referenced this pull request Jun 15, 2018
…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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants