Reload BasicUI if sitemap has been changed #3846
Reload BasicUI if sitemap has been changed #3846
Conversation
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
a373b6c
to
f076d92
Compare
@@ -1709,6 +1709,16 @@ | |||
value, | |||
title; | |||
|
|||
if(data.TYPE === "SITEMAP_CHANGED") { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>
okay from my side. |
window.location.href = parts[0] + "?sitemap="+data.sitemapName; | ||
} else { | ||
window.location.href = oldLocation; | ||
} |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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>
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 |
This partially reverts eclipse-archived#3846. fixes eclipse-archived#5522 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
* 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>
…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>
This catches the IndexOutofBoundException from #3836
Signed-off-by: Stefan Triller stefan.triller@telekom.de