-
Notifications
You must be signed in to change notification settings - Fork 7
Added rescan interval #4
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the PR. I'll have a look at it when I have time. |
skuethe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the same indentation as the rest of the code. So no tab's, only spaces.
Thanks for your fast updates. I saw you pushed changes to README concerning the tab to spaces change. |
| if (typeof this.config.repositoryConfig.path !== "undefined" && this.config.repositoryConfig.path !== null) { | ||
| this.sendSocketNotification('FETCH_IMAGE_LIST'); | ||
|
|
||
| if (this.config.scanInterval != -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this timer is, that it will conflict with the updateInterval timer.
Also, when you have startPaused set to true it will still reload the images and continue with the image cycle on every scanInterval trigger. This breaks usage of the startPaused setting and somewhat the one of updateInterval.
So we need to think of a way to respect these settings before triggering a reload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Conflict
Could you please explain this in more detail? I'm not really sure I see how there's a conflict. The two timers should be independent.
- startPaused set to true
I think the issue is that this.load() is getting called on line 107 when startPaused is set to true? Consequently even though startPaused is set to true, the image will keep cycling.
So to fix that, the IMAGE_LIST notification needs to be disconnected from loading an image - at least after an image has already been loaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Conflict
Well this also has to do with the fact that we are calling this.load() on every IMAGE_LIST notification received. In this case both timers are triggering the load of the next image. That's what I mean with "conflict".
- startPaused set to true
Well this.load() is always called in the resumeImageLoading function. Regardless of what startPaused is set to.
startHidden actually handles the call when receiving the notification.
After giving it a bit of thought, it is probably best to just get rid of lines 411 to 413.
We are calling the resumeImageLoading function already during MM²'s extended build-in resume function. So that should get the image loading started in the first place.
But this needs to be tested to see if the app behaves like it should with all the different config settings - especially startHidden and startPaused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. It does look as if they would solve the initial issue. But I agree that it's necessary to check the startHidden and startPaused. I'll give it a go here and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, no, doesn't seem to work. The image loading never starts.
A messier fix might be to add a firstImageLoaded flag to indicate whether a first image has loaded or not - and not to call resumeImageLoading if firstImageLoaded is true.
But it seems inelegant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we delete lines 411 to 413 we need to also adjust these lines.
But the problem actually still persists. No display is being shown, because the image loading just takes more time.
So actually invoking the image showing on the IMAGE_LIST socketNotification still makes sense (lines 411 to 413).
So I would agree the easiest solution probably is to introduce some kind of flag / parameter to indicate if it is an "initial image loading" process or an "update image loading" process.
Some thoughts without going deeper into details:
- maybe add an additional
UPDATE_IMAGE_LISTsocketNotification? - somehow wait for
this.imageList's lenght to be> 0? - maybe work with
asyncandawaitto wait for some executions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now testing a version that uses an UPDATE_IMAGE_LIST socket notification. On reception, all it does it replace the imageList. It doesn't do anything regarding resumption.
I haven't followed up on waiting for the imageList length to be >0 or using async/wait. These changes don't seem to be necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above version seems to work. However, I've been having difficulty with the calendar not updating as well, so I'm going to take the simpler option of just restarting MM every day.
Feel free to use the push or not as you like :)
Added rescan interval as an option to rescan the image repository periodically.