-
Notifications
You must be signed in to change notification settings - Fork 54
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
Implement automatic sync #63
Conversation
32c6635
to
fdf5abd
Compare
b1eecd5
to
f269afe
Compare
Wait, nevermind, Netflix does offer percentage watched. Fixed it. Also added an option to filter items by minimum percentage watched, so now the automatic sync picks up the value specified in that option. |
859452e
to
afe565c
Compare
Think I'm done here. This is how it ended up working:
I think that's how it should work. Let me know if you think I need to change something. |
|
Hmm. I think it's better with a text, "not supported", or an icon with the same as a tooltip.
I think 80 or 90 should be the default value here.
Hmm. That't tricky. Also some items doesn't match anything, but they can be manually fixed later when syncing the old way, or will the date prevent this from be fixed?
|
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.
Wow. Huge amount of work here. A bit hard to review all the code, but I will checkout and test this.
Yeah, sorry about that. I didn't know if I should make smaller PRs, because it feels a bit spammy. Going commit-by-commit should be easier than going by files changed, because I tried to keep changes where they belong in each commit. |
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 seem to get this error when trying to load history (manually) on all pages:
Access to XMLHttpRequest at 'https://www.netflix.com/Activate' from origin 'chrome-extension://bgdoldpgfpjehilgjeophjhlpemiopbi' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.
I get this also on master, so not anything wrong with this PR, but just need to investigate if it's a new chrome-verison or something else. Also happens on other streaming services.
No need to apologize. I'm not always able to review things ASAP, so if having things together makes more sense to you, just to it that way. I'm fine with either. |
Probably a permission error? Did you try disabling and enabling the Netflix sync again? |
I think there are some migrating problems with BrowserStorage. When using 'clearStorage' on this branch I was not even able to log in again. I think the browser storage should be more stable as switching to branched with different set of options shouldn't break the extension. Instead extra options should be silently ignored, or logged. |
Did you forget to translate |
Let me know when translations are ready for another overhaul. When testing this I was not able to get the auto sync to work. I've put the value to 1 day, but I'm not quite sure why it's not working as expected. |
Did you sync manually first and wait 24 hours? You can trigger the automatic sync sooner by commenting this line: https://github.com/trakt-tools/universal-trakt-scrobbler/pull/63/files#diff-6504efd683d6af1367f550ccb51266b4c3989bf158e0d4091225a626698c315dR209 |
Made some improvements:
|
14b8eb1
to
7e85a4c
Compare
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.
Also I get this error in the console, but it seems like everything is working:
Error in event handler: TypeError: Cannot set property 'value' of undefined
at _BrowserStorage.addOption
Noted, I'll see if I can reproduce and fix. I have a few more commits to push here and then I should be done. |
…ction while the icon is rotating
20ae913
to
dd4b8df
Compare
I guess it could be possible to duplicate items by clearing the last sync date and syncing an older item, because then the auto sync will go through the same items that it did before and add them to the cache. I should probably also clear the cache when the last sync date is cleared. If you didn't do that, it could be failing to detect new items and always running through the same ones. Is this the NRK auto sync? One thing you could do is open the background page console when the auto sync is happening (you can always trigger it manually by commenting the |
… date for that service
I can reproduce the duplicating cache issue now, trying to find the problem. |
Ah, I think I figured it out. It's probably because Just pushed the fix. |
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.
Good job! I noticed an annoyance. Since you can go from the options page to the sync page, in a new tab: The old Option Page still keeps the explanation mark, and you have to do a refresh to make it go away. Not a show-stopper, but a minor issue I guess. One option would be to not open in a new tab when clicking on the sync-icon.
I noticed that as well. I can just add a listener to the storage and refresh the component when the storage changes. |
The component should refresh now. I think that's it, all done here. |
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.
Very nice work!
Will fix #8 and fix #11 and lay down the foundation for #47
Still have some stuff to do, but wanted to discuss things.
I changed the UI for enabling/disabling services (as seen in the image below) to include an option to enable the automatic sync and decided to add separate options for scrobble/sync as well. If a switch is greyed out it's because the service doesn't support it. Let me know what you think.
How it works right now:
percentageWatched
(because that info is not available for some services) or items withpercentageWatched
higher or equal to 10 are synced (this number is arbitrary and can be changed).One of the downsides is that all new items since the last sync will be synced, which might include items that the user hasn't actually finished watching yet (can be overcome with
percentageWatched
, but Netflix, for example, doesn't have that info).Another downside is that you can't correct items, so if an item matched with a wrong item on Trakt, the wrong item will be synced, and I'm not entirely sure how to deal with this.
I'm currently working on these issues:
percentageWatched
Thoughts?
Will rebase once #62 is merged.