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

✨🐛 Add Scobble percent & Fix Last.fm login #1286

Conversation

jaredcat
Copy link
Contributor

@jaredcat jaredcat commented Feb 6, 2024

Screenshot 2024-02-08 at 10 11 21 AM

@jaredcat jaredcat force-pushed the feature/user-defined-scrobble-percent branch from 6aedb5f to 91b18d2 Compare February 6, 2024 08:13
@Alipoodle
Copy link
Member

Only skimmed though the PR currently.

But why the change to 5 minutes when the documentation for Last.fm specifically notes 4 minutes:

"And the track has been played for at least half its duration, or for 4 minutes (whichever occurs earlier.)"
Last.fm Document

@Alipoodle Alipoodle changed the title Fix: lastfm login ✨🐛 Add Scobble percent & Fix Last.fm login Feb 6, 2024
Copy link
Member

@Alipoodle Alipoodle left a comment

Choose a reason for hiding this comment

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

Leaving this as comments for now until I can test and verify the Authenicate thing.

The Ability to have the progress of the feature looks perfect and it's a lot simplier than I originally expected it to be, but some of the changes break L.fm's documentation / seem a step back.

src/main/integrations/last-fm/index.ts Outdated Show resolved Hide resolved
src/main/integrations/last-fm/index.ts Outdated Show resolved Hide resolved
src/main/integrations/last-fm/index.ts Outdated Show resolved Hide resolved
@jaredcat jaredcat force-pushed the feature/user-defined-scrobble-percent branch from db1f306 to d7fcccb Compare February 6, 2024 23:24
@jaredcat jaredcat force-pushed the feature/user-defined-scrobble-percent branch from dcf5d4f to c27f46d Compare February 7, 2024 01:09
Copy link
Member

@Alipoodle Alipoodle left a comment

Choose a reason for hiding this comment

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

Primarily changes for the settings to make the UI look more consistent with the rest of stuff.

I will get this downloaded + tested tomorrow to see about the session stuff and the feature works as expected.
But otherwise this is looking good.

src/renderer/windows/settings/Settings.vue Outdated Show resolved Hide resolved
src/renderer/windows/settings/Settings.vue Outdated Show resolved Hide resolved
src/renderer/windows/settings/Settings.vue Outdated Show resolved Hide resolved
src/main/integrations/last-fm/index.ts Show resolved Hide resolved
Copy link
Member

@Alipoodle Alipoodle left a comment

Choose a reason for hiding this comment

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

Final change for the .vscode/settings.json file. And otherwise this is looking good, and should make it for the v2.0.1 release.

.vscode/settings.json Outdated Show resolved Hide resolved
src/main/index.ts Show resolved Hide resolved
@jaredcat jaredcat force-pushed the feature/user-defined-scrobble-percent branch from 4337553 to 3f1f123 Compare February 15, 2024 23:13
Copy link
Member

@Alipoodle Alipoodle left a comment

Choose a reason for hiding this comment

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

All looking good :)

@NovusTheory NovusTheory merged commit 1e12337 into ytmdesktop:development Feb 18, 2024
6 checks passed
@jaredcat jaredcat deleted the feature/user-defined-scrobble-percent branch February 18, 2024 23:54
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.

None yet

3 participants