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

Like/Unlike Implementation #43, Minimize to Tray #86 and Fix for Native Title Bar #83 #88

Merged
merged 26 commits into from
Jan 23, 2021

Conversation

stamoun
Copy link
Collaborator

@stamoun stamoun commented Oct 8, 2020

This is the implementation of enhancements #43 and fixes for issues #80 and #83
The only caveat is that we now make 2 requests in listeningTo, one to fill currently_playing the other to determine if the track is liked or not. This good lead to more 429 errors, but it does the same amount (or less) of API requests as version 1.5.0 does (I moved the keepAlive method to a 5sec interval only if the player is paused.

Lofi 1 5 4

Be aware that this PR exposes issue #87 more on Linux (workaround: use alt/super key to drag the window past the boundary).
Prior to this version bug issue #87 only happened when the user had always on top unchecked.

stamoun added 14 commits October 5, 2020 11:08
The workspace settings doesn't work, nor is .prettierrc.json... :|
- Added `show in taskbar` setting
- Apply new settings without reloading
- Only reset user-modifiable settings when resetting to factory defaults (i.e. do not remove the auth tokens)
- Fixed white background when closing the debug console
Use native titlebar buttons
This hides the min/max buttons when disabled
Focusable will always be true
This causes bug #87
Position the window in the middle of the screen on 1st time launch or when 'remember window position' is false
@stamoun stamoun marked this pull request as ready for review October 8, 2020 15:00
@stamoun stamoun changed the title Like/Unlike Implementation #43 Like/Unlike Implementation #43, Minimize to Tray #86 and Fix for Native Title Bar #83 Oct 8, 2020
@dvx
Copy link
Owner

dvx commented Oct 27, 2020

This is great! had time to look at it, and the only problem I'm seeing is that I sometimes have to re-auth again after I launch Lofi. For some reason, it's not properly remembering the past authorization token. I need to look at the code because I know the auth flow changed a bit, so I'll be able to get to the bottom of it. There's also a bit of an inconsistency with the control padding (more space left-to-right than top-to-bottom), but that's just a CSS tweak. Awesome job as always!

I love the alert icon at the bottom when requests are throttled, but I'm not sure if we should disable the controls visually -- I only say this because I was actually confused a few times when controls would randomly get greyed out (due to random Spotify throttling on the heartbeat requests).

@dvx dvx self-requested a review October 27, 2020 00:52
@dvx dvx self-assigned this Oct 27, 2020
@stamoun
Copy link
Collaborator Author

stamoun commented Oct 27, 2020

I sometimes have to re-auth again after I launch Lofi

I noticed that too, but it was somewhat rare. I think it occurs when an error (503?) happens in the backend while fetching/refreshing the access token. Nothing much we can do about it (once the refresh token is spent, it can't be reused).

...but I'm not sure if we should disable the controls visually

Yah, I hear ya, wasn't sure how to tell the user that the controls won't work since the API calls are being throttled. I'll just remove the disabled stuff and leave it as is for now. You'll decide where to take this next ;)

@dvx dvx changed the base branch from master to stamoun-like-43 January 23, 2021 21:10
@dvx
Copy link
Owner

dvx commented Jan 23, 2021

Finally had time to circle back and look at this. I was able to solve the re-auth problem by also adding user-read-email to AUTH_SCOPES. A few more quirks need to be fixed and don't wanna bother you with more review comments, so I'll merge this into its own branch, fiddle with it a bit, and merge into master.

Thanks again for all your hard work @stamoun!

@dvx dvx merged commit 7ffec64 into dvx:stamoun-like-43 Jan 23, 2021
@dvx dvx mentioned this pull request Jan 23, 2021
@stamoun stamoun deleted the like-43 branch September 7, 2021 19: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.

2 participants