Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reload BasicUI if sitemap has been changed #3846

Merged
merged 4 commits into from Jul 18, 2017

Conversation

Projects
None yet
4 participants
@triller-telekom
Copy link
Contributor

commented Jul 17, 2017

This catches the IndexOutofBoundException from #3836

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

Reload BasicUI if sitemap has been changed
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

@triller-telekom triller-telekom force-pushed the triller-telekom:basicUIsitemapBrowserReload branch from a373b6c to f076d92 Jul 17, 2017

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

if(data.TYPE === "SITEMAP_CHANGED") {

This comment has been minimized.

Copy link
@sjka

sjka Jul 17, 2017

Contributor

What happened to your formatter?

This comment has been minimized.

Copy link
@triller-telekom

triller-telekom Jul 18, 2017

Author Contributor

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) {

This comment has been minimized.

Copy link
@sjka

sjka Jul 17, 2017

Contributor

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

This comment has been minimized.

Copy link
@triller-telekom

triller-telekom Jul 18, 2017

Author Contributor

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") {

This comment has been minimized.

Copy link
@sjka

sjka Jul 17, 2017

Contributor

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?

This comment has been minimized.

Copy link
@triller-telekom

triller-telekom Jul 18, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Jul 17, 2017

Contributor

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?

This comment has been minimized.

Copy link
@triller-telekom

triller-telekom Jul 18, 2017

Author Contributor

Agreed, creating one event is sufficient.

triller-telekom added some commits Jul 18, 2017

Check boundaries instead of catching Exception
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Javascript format fixes
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@sjka

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

okay from my side.
@resetnow your call.

@sjka

sjka approved these changes Jul 18, 2017

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

This comment has been minimized.

Copy link
@resetnow

resetnow Jul 18, 2017

Contributor

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

This comment has been minimized.

Copy link
@resetnow

resetnow Jul 18, 2017

Contributor

Spaces:

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

This comment has been minimized.

Copy link
@resetnow

resetnow Jul 18, 2017

Contributor
window.location.reload(true);
use reload function for browser refresh + return early
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

@sjka sjka merged commit c555c35 into eclipse:master Jul 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@triller-telekom triller-telekom deleted the triller-telekom:basicUIsitemapBrowserReload branch Jul 18, 2017

@kaikreuzer

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

sjka added a commit to sjka/smarthome that referenced this pull request May 2, 2018

refresh internal widget collection on sitemap change
This partially reverts eclipse#3846.

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

kaikreuzer added a commit that referenced this pull request May 3, 2018

refresh internal widget collection on sitemap change (#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 #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

refresh internal widget collection on sitemap change (eclipse#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#3846.
fixes eclipse#5522
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.