-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Let's try this again! Fixes status bar timer stat #64
base: master
Are you sure you want to change the base?
Conversation
@hadynz sorry for the confusion earlier; this is the change that I meant to make! Please let me know if this is unwelcome or you want to see edits to it. |
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.
Thanks so much for the changes in this PR. Accuracy of stats and updates of status bar were on my list. Appreciate that you've jumped on this.
import moment from 'moment'; | ||
|
||
let lastSyncText = ''; | ||
setInterval(() => lastSyncText = `Last sync ${moment($statusBarStore.lastSyncDate).fromNow()}.`, 3000); |
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 believe that simply re-rendering the status bar text from the store will show the correct time. The store is currently the source of truth for what information should be shown to the user.
So all that you might need to do (without testing this idea) is to simply instruct the component to re-read data from the store:
let lastSyncText = $statusBarStore.text;
setInterval(() => {
lastSyncText = $statusBarStore.text
}, 3000);
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 think users might need multi-second accuracy on when sync last happened. So updated the text every minute might be good enough.
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.
Interestingly, the value in the derived status bar store is never updated. Setting the interval here works and the text is updated but the value never changes.
I'm not super familiar with Svelte; is there a way to force re-derivation of the store periodically?
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.
Sorry if I sent you on a goose chase without enough guidance.
This is a really good example from the Svelte docs that resembles what we want to do. Basically "time" is wrapped in a special store object whose value can be set externally using readable
. This is where we can configure how the timer should behave. This store object can then be passed in the existing derived
method as a parameter.
Let me know how you go. You are very close!
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 I've refactored this a bit to get a little closer to what you're hopefully looking for! I've created a store like you mentioned to hold the time, and I've added a derived store in syncSession.ts
in order to handle the text formatting.
This works for me locally! Please let me know what you'd like to see changed.
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.
FWIW I've continued to use this fork locally and it's been stable and the timing is correct! Please let me know if you'd like me to change or close this PR in some way.
I love this plugin. One tiny thing that bothers me is the timer on the status bar never updates the last-synced time; the current status bar always reads "Last sync a few seconds ago."
This is a small change that will mean the status bar will look something like this after a while:
I arbitrarily picked 3 seconds as the time for the text to refresh, but I'm not convinced that's the best option. Suggestions are appreciated for this constant.
Note also that this removes the book synced count, which has been historically super inaccurate for me (and is on my list of things to tinker with as well). I can add it back in if you'd prefer.
Please let me know if this type of change is unwelcome or if you'd like to see any modifications!