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

Merge with scrobbler #23

Merged
merged 27 commits into from
Jul 19, 2020
Merged

Merge with scrobbler #23

merged 27 commits into from
Jul 19, 2020

Conversation

rafaelgomesxyz
Copy link
Member

Fixes #19

Still need to merge the actual scrobblers, but most of it is done.

@rafaelgomesxyz rafaelgomesxyz added the enhancement New feature or request label Jul 17, 2020
@rafaelgomesxyz rafaelgomesxyz marked this pull request as ready for review July 18, 2020 14:57
@rafaelgomesxyz
Copy link
Member Author

Alright, think I'm done here.

@rafaelgomesxyz
Copy link
Member Author

rafaelgomesxyz commented Jul 18, 2020

The only bug I'm experiencing is that sometimes the popup doesn't load, which appears to be the same bug that affects traktflix (tegon/traktflix#100). But after about 3 times of clicking the button, it loads. I'm trying to see what's causing this.

Edit: Actually, doesn't look like it's that issue, but I was sure this also happened in traktflix.

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 get an error when toggling: Show browser notifications. (Failed to save option.)

src/_locales/pt_BR/messages.json Show resolved Hide resolved
@MrMamen
Copy link
Member

MrMamen commented Jul 18, 2020

I get an error when toggling: Show browser notifications. (Failed to save option.)

Same with Netflix. Not able to disable it.

@rafaelgomesxyz
Copy link
Member Author

I get an error when toggling: Show browser notifications. (Failed to save option.)

For notifications, it's because I forgot to add the notifications permission to the manifest, but I don't know why you can't disable Netflix. I'll test with a clean profile.

@MrMamen
Copy link
Member

MrMamen commented Jul 18, 2020

I get an error when toggling: Show browser notifications. (Failed to save option.)

For notifications, it's because I forgot to add the notifications permission to the manifest, but I don't know why you can't disable Netflix. I'll test with a clean profile.

Probably related but I'm not able to scrobble from Netflix either. I did try to clean all cache & data, but same problem.

@rafaelgomesxyz
Copy link
Member Author

rafaelgomesxyz commented Jul 19, 2020

Probably related but I'm not able to scrobble from Netflix either. I did try to clean all cache & data, but same problem.

Ok, I can reproduce it on Chromium.

Edit: This is a bizarre error. It looks like the extension still thinks that Netflix is a required permission. I tried completely wiping the extension from my Chromium folder and it still thinks that.

@rafaelgomesxyz
Copy link
Member Author

Ok, I fixed the scrobble bug.

As for not being able to disable Netflix, I think this is because we're loading the extension as unpacked, so it considers all permissions as required. I don't think this should happen with an actual extension coming from the store.

@rafaelgomesxyz rafaelgomesxyz merged commit 36cc05f into master Jul 19, 2020
@rafaelgomesxyz rafaelgomesxyz deleted the merge-scrobbler branch July 19, 2020 23:52
@MrMamen
Copy link
Member

MrMamen commented Jul 20, 2020

Ok, I fixed the scrobble bug.

As for not being able to disable Netflix, I think this is because we're loading the extension as unpacked, so it considers all permissions as required. I don't think this should happen with an actual extension coming from the store.

But why are the others toggable?

@rafaelgomesxyz
Copy link
Member Author

Ok, I fixed the scrobble bug.
As for not being able to disable Netflix, I think this is because we're loading the extension as unpacked, so it considers all permissions as required. I don't think this should happen with an actual extension coming from the store.

But why are the others toggable?

They're not toggable for me. Probably because I erased the extension and added it again.

@MrMamen
Copy link
Member

MrMamen commented Aug 9, 2020

Ok, I fixed the scrobble bug.
As for not being able to disable Netflix, I think this is because we're loading the extension as unpacked, so it considers all permissions as required. I don't think this should happen with an actual extension coming from the store.

But why are the others toggable?

They're not toggable for me. Probably because I erased the extension and added it again.

Just for the record. The services with scrobbling are not toggable for me, but the ones with just history sync are.

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

Successfully merging this pull request may close these issues.

Merge with scrobbler or third repo with common code?
2 participants