Skip to content
This repository has been archived by the owner. It is now read-only.

Reload BasicUI if sitemap has been changed #3846

Merged

Conversation

@triller-telekom
Copy link
Contributor

@triller-telekom triller-telekom commented Jul 17, 2017

This catches the IndexOutofBoundException from #3836

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

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@triller-telekom triller-telekom force-pushed the basicUIsitemapBrowserReload branch from a373b6c to f076d92 Jul 17, 2017
@@ -1709,6 +1709,16 @@
value,
title;

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

@sjsf sjsf Jul 17, 2017

What happened to your formatter?

Copy link
Contributor Author

@triller-telekom triller-telekom Jul 18, 2017

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

@sjsf sjsf Jul 17, 2017

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

@triller-telekom triller-telekom Jul 18, 2017

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

@sjsf sjsf Jul 17, 2017

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

@triller-telekom triller-telekom Jul 18, 2017

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

@sjsf sjsf Jul 17, 2017

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

@triller-telekom triller-telekom Jul 18, 2017

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 sjsf commented Jul 18, 2017

okay from my side.
@resetnow your call.

sjsf
sjsf approved these changes Jul 18, 2017
window.location.href = parts[0] + "?sitemap="+data.sitemapName;
} else {
window.location.href = oldLocation;
}
Copy link
Contributor

@resetnow resetnow Jul 18, 2017


_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

@resetnow resetnow Jul 18, 2017

Spaces:

"?sitemap=" + data

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

@resetnow resetnow Jul 18, 2017

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
2 checks passed
@triller-telekom triller-telekom deleted the basicUIsitemapBrowserReload branch Jul 18, 2017
@kaikreuzer
Copy link
Contributor

@kaikreuzer kaikreuzer commented Jul 27, 2017

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

@triller-telekom triller-telekom commented Jul 27, 2017

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 issue May 2, 2018
This partially reverts eclipse-archived#3846.

fixes eclipse-archived#5522
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
kaikreuzer added a commit that referenced this issue 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 issue 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants