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

Implement automatic sync #63

Merged
merged 48 commits into from
May 26, 2021
Merged

Implement automatic sync #63

merged 48 commits into from
May 26, 2021

Conversation

rafaelgomesxyz
Copy link
Member

@rafaelgomesxyz rafaelgomesxyz commented May 3, 2021

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.

New UI

How it works right now:

  1. For the automatic sync to work, the user has to do a manual sync first, for the date to be saved (this will be good for Give user option to do initial sync and prevent loading synced history #47).
  2. After the user does the first manual sync, the background page of the extension will start checking, every time the user opens the browser, if it's been the number of days specified in the options since the last sync.
  3. If it's time to sync again, the sync will happen silently in the background page. Only items with no percentageWatched (because that info is not available for some services) or items with percentageWatched 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:

  • Indicate that the sync is happening somehow (maybe change the extension icon like it happens for scrobbles?)
  • Allow the user to visualize what was synced / what failed to sync somehow (maybe add a button to the extension popup that the user can click and be taken to a summary page of the last sync?)
  • Allow the user to specify a threshold for percentageWatched

Thoughts?

Will rebase once #62 is merged.

@rafaelgomesxyz rafaelgomesxyz changed the base branch from dynamic-content-scripts to master May 4, 2021 17:15
@rafaelgomesxyz
Copy link
Member Author

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.

@rafaelgomesxyz
Copy link
Member Author

Think I'm done here.

This is how it ended up working:

  1. For the automatic sync to work, the user has to do a manual sync first, for the date to be saved (this will be good for Give user option to do initial sync and prevent loading synced history #47).
  2. After the user does the first manual sync, the background page of the extension will start checking, every hour, if it's been the number of days specified in the options since the last sync.
  3. If it's time to sync again, the sync will happen silently in the background page (the extension icon will rotate to indicate that it's happening). Only items with no percentageWatched (because that info is not available for some services) or items with percentageWatched higher or equal to the value specified in the options are synced.
  4. A button is added to the extension popup that opens a summary page where the user can see what was automatically synced / failed to sync. They can correct wrong items there.
  5. If the user tries to load the history for the service that was automatically synced, it will stop loading the history once it reaches the last item that was synced (also good for Give user option to do initial sync and prevent loading synced history #47). This can be reset in the options.

I think that's how it should work. Let me know if you think I need to change something.

@rafaelgomesxyz rafaelgomesxyz marked this pull request as ready for review May 16, 2021 18:13
@rafaelgomesxyz
Copy link
Member Author

rafaelgomesxyz commented May 16, 2021

Wait, I think I found a couple bugs that I still need to fix:

  • Disabling the automatic sync seems to remove permissions.
  • If the user syncs the first history page, it won't load any more pages, because the date from the first page will be saved as the last sync date, and the next pages are older. A possible solution would be to store a copy of the last sync date and use that for as long as the user keeps loading pages and syncing. The only problem would be that if the user reloads/leaves the page, it will go back to not loading any more pages, but in that case the user could just go to the options page and reset the last sync date.

@MrMamen
Copy link
Member

MrMamen commented May 16, 2021

Will fix #8 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.

Hmm. I think it's better with a text, "not supported", or an icon with the same as a tooltip.

How it works right now:

  1. For the automatic sync to work, the user has to do a manual sync first, for the date to be saved (this will be good for Give user option to do initial sync and prevent loading synced history #47).
  2. After the user does the first manual sync, the background page of the extension will start checking, every time the user opens the browser, if it's been the number of days specified in the options since the last sync.
  3. If it's time to sync again, the sync will happen silently in the background page. Only items with no percentageWatched (because that info is not available for some services) or items with percentageWatched higher or equal to 10 are synced (this number is arbitrary and can be changed).

I think 80 or 90 should be the default value here.

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.

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?

I'm currently working on these issues:

  • Indicate that the sync is happening somehow (maybe change the extension icon like it happens for scrobbles?)
  • Allow the user to visualize what was synced / what failed to sync somehow (maybe add a button to the extension popup that the user can click and be taken to a summary page of the last sync?)
  • Allow the user to specify a threshold for percentageWatched

Thoughts?
You could open a page when a sync has occurred with info, but this might be annoying. So perhaps your idea is better.

README.md Outdated Show resolved Hide resolved
Copy link
Member

@MrMamen MrMamen left a 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.

@rafaelgomesxyz
Copy link
Member Author

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.

Copy link
Member

@MrMamen MrMamen left a 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.

src/modules/options/components/StreamingServiceOptions.tsx Outdated Show resolved Hide resolved
@MrMamen
Copy link
Member

MrMamen commented May 16, 2021

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.

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.

@rafaelgomesxyz
Copy link
Member Author

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.

Probably a permission error? Did you try disabling and enabling the Netflix sync again?

@MrMamen
Copy link
Member

MrMamen commented May 16, 2021

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 the 'clearStorage' on master I get it to work again. When switching to this branch I've been experienced a few strange issues. Like:
TypeError: Cannot read property 'amazon-prime' of undefined at _BrowserStorage.addOption (background.js:113956)

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.

@rafaelgomesxyz
Copy link
Member Author

rafaelgomesxyz commented May 16, 2021

Better? The ban icon shows "Not Available" when you hover over it.

@rafaelgomesxyz
Copy link
Member Author

Did you forget to translate serviceSync or is Sync just Sync in NB? Also, I'd wait to translate until I fix the other stuff because I'm adding more messages.

@MrMamen
Copy link
Member

MrMamen commented May 20, 2021

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.

@rafaelgomesxyz
Copy link
Member Author

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

@rafaelgomesxyz
Copy link
Member Author

Made some improvements:

  • Added a button to continue loading history past the last sync date: this prevents the user from having to go to the options and clear the last sync date if they want to load older history, and makes it more straightforward
  • Replaced browser history with hash history: this makes it possible not only to refresh pages, but also to go directly to specific pages (for example, the user can go directly to the Netflix history page without having to select it in the service list first)
  • Improved options layout: the external link icon allows the user to go directly to the history page of the service, and the error icon appears when the user hasn't synced manually yet, to indicate that the automatic sync is not activated (I think this makes it easier to understand how it works?)

@rafaelgomesxyz rafaelgomesxyz force-pushed the automatic-sync branch 2 times, most recently from 14b8eb1 to 7e85a4c Compare May 23, 2021 17:02
Copy link
Member

@MrMamen MrMamen left a 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

src/modules/background/background.ts Outdated Show resolved Hide resolved
@rafaelgomesxyz
Copy link
Member Author

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.

@rafaelgomesxyz
Copy link
Member Author

Not quite sure how I managed to get into this state. But the sync-cache seems to duplicate. When going to the reivew-page I now get 22 duplicates of the same three items.

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 now - value.lastSync >= value.autoSyncDays * 86400 line) and take a look at the requests, to see if it's going through the same items over and over again.

@rafaelgomesxyz
Copy link
Member Author

I can reproduce the duplicating cache issue now, trying to find the problem.

@rafaelgomesxyz
Copy link
Member Author

I can reproduce the duplicating cache issue now, trying to find the problem.

Ah, I think I figured it out. It's probably because SyncStore is not being reset during the auto sync, so the items remain there, and if it auto syncs again in the same browser session, it'll add them to the cache again.

Just pushed the fix.

MrMamen
MrMamen previously approved these changes May 26, 2021
Copy link
Member

@MrMamen MrMamen left a 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.

@rafaelgomesxyz
Copy link
Member Author

I noticed that as well. I can just add a listener to the storage and refresh the component when the storage changes.

@rafaelgomesxyz
Copy link
Member Author

The component should refresh now. I think that's it, all done here.

Copy link
Member

@MrMamen MrMamen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selected entries needs to be reset when syncing Add option to sync automatically every x days
2 participants