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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -267,14 +267,10 @@ public void modelChanged(String modelName, EventType type) {
for (Entry<String, PageChangeListener> listenerEntry : pageChangeListeners.entrySet()) {
String sitemapWithPage = listenerEntry.getKey();
String sitemapName = extractSitemapName(sitemapWithPage);
String pageId = extractPageId(sitemapWithPage);

if (sitemapName.equals(changedSitemapName)) {
EList<Widget> widgets = collectWidgets(sitemapName, pageId);
listenerEntry.getValue().sitemapContentChanged(widgets);
listenerEntry.getValue().sitemapContentChanged();
}
}

}

}
Expand Up @@ -213,8 +213,13 @@ private boolean definesVisibility(Widget w, String name) {
return false;
}

public void sitemapContentChanged(EList<Widget> widgets) {
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.

changeEvent.pageId = pageId;
changeEvent.sitemapName = sitemapName;
callback.onEvent(changeEvent);
}
}

}
@@ -0,0 +1,18 @@
/**
* Copyright (c) 2014-2017 by the respective copyright holders.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*/
package org.eclipse.smarthome.io.rest.sitemap.internal;

/**
* Event to notify the browser that the sitemap has been changed
*
* @author Stefan Triller - Initial Contribution
*
*/
public class SitemapChangedEvent extends SitemapEvent {
public final String TYPE = "SITEMAP_CHANGED";
}
Expand Up @@ -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...

// no valid number, so the requested page id does not exist
}
}
Expand Down
10 changes: 10 additions & 0 deletions extensions/ui/org.eclipse.smarthome.ui.basic/web-src/smarthome.js
Expand Up @@ -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

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.

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

} 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);

}
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;

}

if (!(data.widgetId in smarthome.dataModel) && (data.widgetId !== smarthome.UI.page)) {
return;
}
Expand Down

Large diffs are not rendered by default.