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

Basic UI: fix refresh of hidden/visible image/chart #4534

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
4 participants
@lolodomo
Copy link
Contributor

commented Nov 12, 2017

Fixes #4492
Fixes #4507
Ignore refresh when inappropriate

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2017

@resetnow : please confirm that I don't need anymore to provide the file smarthome.js in the web directory but only the one in the web-src directory ?

If you can review, that would be cool.

@resetnow

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2017

I don't need anymore to provide the file smarthome.js in the web directory

That's correct.

If you can review, that would be cool.

Later today.

@resetnow
Copy link
Contributor

left a comment

I would also check if the browser actually makes requests for images that are hidden with display: none;.

_t.visible = state;
};

_t.disactivateRefresh = function() {

This comment has been minimized.

Copy link
@resetnow

resetnow Nov 13, 2017

Contributor

You probably meant deactivateRefresh

This comment has been minimized.

Copy link
@lolodomo

lolodomo Nov 13, 2017

Author Contributor

Yes

_t.disactivateRefresh = function() {
if (interval !== undefined) {
clearInterval(interval);
interval = undefined;

This comment has been minimized.

Copy link
@resetnow

resetnow Nov 13, 2017

Contributor

JS type are weird but I think here null is more suitable as it's used in the rest of the code

This comment has been minimized.

Copy link
@lolodomo

lolodomo Nov 13, 2017

Author Contributor

Ok I will change that.

String url = chartUrl + "&t=" + (new Date()).getTime();
String url;
if (!itemUIRegistry.getVisiblity(w)) {
url = "images/none.png";

This comment has been minimized.

Copy link
@resetnow

resetnow Nov 13, 2017

Contributor

Feels a bit weird here, should be a static const maybe?

This comment has been minimized.

Copy link
@lolodomo

lolodomo Nov 13, 2017

Author Contributor

Ok, why not.

interval = setInterval(function() {
if (_t.image.clientWidth === 0) {
clearInterval(interval);
return;
}
_t.image.setAttribute("src", _t.url + "&t=" + Date.now());
if (_t.image.getAttribute("src").startsWith(_t.url)) {

This comment has been minimized.

Copy link
@resetnow

resetnow Nov 13, 2017

Contributor

Couldn't figure out what this is for.

This comment has been minimized.

Copy link
@lolodomo

lolodomo Nov 13, 2017

Author Contributor

It is to avoid a refresh of the image using the proxied URL when the image was set with a raw data for example.

This comment has been minimized.

Copy link
@lolodomo

lolodomo Nov 13, 2017

Author Contributor

Maybe I should rather set a variable to store the information whether we should refresh or not.

@lolodomo lolodomo force-pushed the lolodomo:hidden_chart branch 2 times, most recently from ef3cb60 to da6d8e0 Nov 14, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2017

@resetnow : I have taken into account all your comments except one that remains to be considered.

Basic UI: fix refresh of hidden/visible image/chart
Fixes #4492
Fixes #4507
Ignore refresh when inappropriate

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

@lolodomo lolodomo force-pushed the lolodomo:hidden_chart branch from da6d8e0 to 40391b8 Nov 15, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2017

@resetnow : everything is now ok and working well.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

@resetnow If you can approve the changes, we should be good to merge!

@resetnow
Copy link
Contributor

left a comment

LGTM

@maggu2810 maggu2810 merged commit d3301aa into eclipse:master Nov 20, 2017

2 checks passed

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

@lolodomo lolodomo deleted the lolodomo:hidden_chart branch Nov 20, 2017

kaikreuzer added a commit that referenced this pull request Nov 20, 2017

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

@kaikreuzer kaikreuzer added the bug label Dec 15, 2017

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.