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

Adjustable Refresh Rate #309

Merged
merged 6 commits into from
Feb 17, 2024
Merged

Adjustable Refresh Rate #309

merged 6 commits into from
Feb 17, 2024

Conversation

Yayroos
Copy link
Contributor

@Yayroos Yayroos commented Feb 10, 2024

Add a setting in Advanced Settings that allows the polling rate of currently playing and liked to be set.

@Yayroos Yayroos marked this pull request as ready for review February 10, 2024 04:15
@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 10, 2024

#308 #177 #234 #201 - fix and/or workaround for all these (and any any others about the API Throttling/constant calls) Allows the frequency to be set in Advanced Settings to your preference (default is current behaviour, currently playing every second)

.eslintrc.json Outdated Show resolved Hide resolved
ACKNOWLEDGMENTS.md Outdated Show resolved Hide resolved
BUGS.md Outdated Show resolved Hide resolved
src/main/main.ts Outdated Show resolved Hide resolved
src/models/settings.ts Outdated Show resolved Hide resolved
src/renderer/app/cover/index.tsx Outdated Show resolved Hide resolved
src/renderer/windows/settings/advanced-settings.tsx Outdated Show resolved Hide resolved
Refresh Time (milliseconds)
<Slider
type="range"
min={1000}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
min={1000}
min={1}

src/renderer/windows/settings/advanced-settings.tsx Outdated Show resolved Hide resolved
src/renderer/windows/settings/advanced-settings.tsx Outdated Show resolved Hide resolved
@stamoun
Copy link
Collaborator

stamoun commented Feb 10, 2024

Many thanks for your contribution! 🙏

However, please do not include styling change in an MR, follow the current ones (e.g. use single quotes and not double quotes).

Styling change could be done in a separate MR through an automatic process (much easier to review). I should set these for the whole project so that there's not ambiguity...

@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 10, 2024

Will do - friend was helping me sort out the git stuff and applied the prettier formatting defaults so everything got rearranged.

@stamoun
Copy link
Collaborator

stamoun commented Feb 10, 2024

Will do - friend was helping me sort out the git stuff and applied the prettier formatting defaults so everything got rearranged.

No worries, I need a more exhaustive eslint config file (including prettier styling)

@Yayroos Yayroos marked this pull request as draft February 10, 2024 22:57
@Yayroos Yayroos marked this pull request as ready for review February 10, 2024 23:12
@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 10, 2024

Have reverted all those other files - this should just be the files i touched to actually add the feature

src/models/settings.ts Outdated Show resolved Hide resolved
src/renderer/app/cover/index.tsx Outdated Show resolved Hide resolved
src/renderer/app/cover/index.tsx Outdated Show resolved Hide resolved
@stamoun
Copy link
Collaborator

stamoun commented Feb 12, 2024

Good stuff, I'll let you merge it @Yayroos 🚀

@stamoun
Copy link
Collaborator

stamoun commented Feb 12, 2024

Forgot to add something but I'm sure it's fine...

When you modify the UI in any way, please post a screenshot of your modification (the extra setting in this case). You can still go ahead and merge this PR though.

@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 12, 2024

i'll grab a screenshot today!

thanks for your quick responses and patience.

@stamoun
Copy link
Collaborator

stamoun commented Feb 12, 2024

i'll grab a screenshot today!

thanks for your quick responses and patience.

No worries, it's fun to see contributions to Lofi :) Also, thanks for being understanding. I hope you learned a bit during this review.

@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 14, 2024

i will get to this - i got covid and sitting up for more than 5 minutes hurts

@stamoun
Copy link
Collaborator

stamoun commented Feb 14, 2024

i will get to this - i got covid and sitting up for more than 5 minutes hurts

No worries, I can squash and merge for you, but I want you to press that button. I don't know why but it's a good feeling to press that Squash and merge button :)

This code isn't going anywhere.

@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 14, 2024

i'll do it, maybe this afternoon if i can actually use my proper laptop. I'm keen to do it, i just can't think straight or sit up properly at the moment, so it'll have to wait until i shake the damn virus

@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 17, 2024

lofi new setting refresh time
screenshot of the new setting :)

@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 17, 2024

i dont seem to actually have access to merge it, at the very least i cant see any button indicating to do so

@stamoun
Copy link
Collaborator

stamoun commented Feb 17, 2024

Could you change the option label to something like API polling interval (Seconds)?

Don’t worry about merging it, I thought PR authors could merge once approved, seems like not.

I’ll merge it early next week.

@stamoun
Copy link
Collaborator

stamoun commented Feb 17, 2024

That was fast! Merging! 🚀

@Yayroos
Copy link
Contributor Author

Yayroos commented Feb 17, 2024

have changed the setting label. thanks for the patience. still absolutely rolled by covid at the moment.

@stamoun stamoun merged commit 67900ad into dvx:master Feb 17, 2024
@alient12 alient12 mentioned this pull request Feb 17, 2024
@ccxxv
Copy link

ccxxv commented Sep 13, 2024

lofi new setting refresh time screenshot of the new setting :)

How do I add this to mine? I don't seem to have this option.

@Yayroos
Copy link
Contributor Author

Yayroos commented Sep 13, 2024

How do I add this to mine? I don't seem to have this option.

doesn't look like anyone's generated a release with this in - I'll try in the next couple of days on my fork if i can figure it out, else you can build it from source if you can install all the stuff.

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.

3 participants