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

Bug: startTime delayed (max pollFrequency) #2

Open
qistoph opened this issue Apr 2, 2019 · 16 comments
Open

Bug: startTime delayed (max pollFrequency) #2

qistoph opened this issue Apr 2, 2019 · 16 comments
Labels
bug Something isn't working

Comments

@qistoph
Copy link
Owner

qistoph commented Apr 2, 2019

An update cycle is run periodically (see pollFrequency). The module is only shown if during this update cycle the current time is between startTime and endTime.

It would be better if the module would be shown as soon as the current time reaches startTime.

@qistoph qistoph added the bug Something isn't working label Apr 2, 2019
@radokristof
Copy link

I already had some times that it won't even show the module even if it is still in the window and several pollFrequency updates made.

@qistoph
Copy link
Owner Author

qistoph commented Mar 5, 2020

Any idea why?

@radokristof
Copy link

Not really, I haven't looked into it yet. I just discovered that the module should be visible and it was still hidden, even 1 hour after it should be on. A simple MagicMirror restart solved it, so I don't really get why it was still hidden. Looking at the code, it should check this every pollFrequency...
I did some enhancements to the code, I will create a PR.

@radokristof
Copy link

I have probably found the issue:

if ( this.hidden ) {

Here change hidden to isHidden

@qistoph
Copy link
Owner Author

qistoph commented Mar 10, 2020

WOW! Thank you so much! I don't have time to test it at the moment, but it definitely looks really suspicious.

@radokristof
Copy link

I'm still testing it, but it seems that this solved it for me.
Regarding the original issue, I thought about this, but I'm not sure how it can be achieved with the current code design. What I thought of is using cron jobs (for each destination) and if it is started by cron, simply start the timer at that time.
However this means that every destination needs to be seperated and ran in its own thread with it's own interval (which is not so bad, you could specify pollFrequency for each destination), but it needs a major overhaul.
Also ending is not so obvious with this. Maybe another cron which cancels the timer when reached...

@qistoph
Copy link
Owner Author

qistoph commented Mar 10, 2020

I was thinking something along:

At any time it's possible to calculate how long until the module needs to be shown. That could be used as delay before showing. If we set that as the delay until next update (instead of pollFreq.), the module would update and show at that time.

@radokristof
Copy link

Oh, that seems easier...
However you can do this only for the global config startTime and endTime (because only one "timer" running now). Or you calculate for each destination and use the closest one (and after you just use the pollFrequency). If isHidden is true again, then you calculate the closest one again...
However I don't know how you can manage calendarOptions in this way.

@radokristof
Copy link

@qistoph Unfortunately, I'm my module still won't show up sometimes (even after changing that isHidden parameter - also there is a default hidden parameter also which is changes to true when you hide the module, so this shouldn't be a problem).
What I can't figure out right now is when you hide the module and the suspend method is called, you clear the interval there, which means there is no timeframe check and polling.
However what will trigger the resume function (in order to start polling again)?

@qistoph
Copy link
Owner Author

qistoph commented Mar 16, 2020

I have probably found the issue:

if ( this.hidden ) {

Here change hidden to isHidden

That seems to fix it indeed! Thank you so much! Patched and pushed.

@radokristof
Copy link

For me unfortunately not. It was still times when it didn't wanted to show up.
I have simplified the code as much as I can, I'm testing it, but it seems to be working.
The problem seems correct, that removing the interval in suspend, the polling will stop completely, that's why it will not show up after all destinations are hidden.

@qistoph
Copy link
Owner Author

qistoph commented Mar 16, 2020

Hmmm, that might have been intentional. Since the module is using credits I wanted people to have control over the amount of credits the module is using. By limiting the visible time, the idea was, to also limit the amount of credits used. Maybe a complete redesign would be a better start...

@radokristof
Copy link

Yes that's right. However my approach will not change this behavior, the amount of credit used will be the same.
Assume you have a 10 min pollFrequency set, you poll every 10 min. If the getData function starts, it will check if any destination is in the timeframe specified, and if not there will be no data sent to Google.
However if you remove this 10 min pollFrequency completely, you can't check every 10 min if is there any destination in the timeframe.

@radokristof
Copy link

radokristof commented Mar 22, 2020

I have eliminated all of the module not appering bug. It works for a week now without a problem.
I can make a PR is you like that, however my changes brings some breaking API changes (like the global hide/show is not there - this is just a duplicate so I don't think it has any benefit, and other things).
Also if the alternative route name is too long and overlaps, this has also been solved by truncating the text.

@qistoph
Copy link
Owner Author

qistoph commented Mar 28, 2020

Yes that's right. However my approach will not change this behavior, the amount of credit used will be the same.
Assume you have a 10 min pollFrequency set, you poll every 10 min. If the getData function starts, it will check if any destination is in the timeframe specified, and if not there will be no data sent to Google.
However if you remove this 10 min pollFrequency completely, you can't check every 10 min if is there any destination in the timeframe.

That sounds nice indeed.

I have eliminated all of the module not appering bug. It works for a week now without a problem.
I can make a PR is you like that, however my changes brings some breaking API changes (like the global hide/show is not there - this is just a duplicate so I don't think it has any benefit, and other things).
Also if the alternative route name is too long and overlaps, this has also been solved by truncating the text.

A PR sounds nice. I would however like to keep supporting the global show/hide, because that is probably in use in other people's setups. I'm also using it to show/hide modules using the Remote Control module. It's also used in modules that support user profiles (facial recognition to show a person's own modules).

@radokristof
Copy link

radokristof commented Mar 28, 2020

Ok I will create the PR soon.

I would however like to keep supporting the global show/hide, because that is probably in use in other people's setups. I'm also using it to show/hide modules using the Remote Control module. It's also used in modules that support user profiles (facial recognition to show a person's own modules).

Hmm, I may not be right, but I think this change still allows these. When you use Remote Control you can still show hide the module, because it will call the hide method (that is still there). I don't really know how the profiles work, but I assume that you have to specify the modules or destinations for each user. That will still work.
However you are right, this also will bring another API breaking change... At least for those who just use the global show hide and nothing for destinations (because than it will show the destinations always).
Maybe you could try my fork how it works in your scenario like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants