danielfm / smudge

Control the Spotify app from within Emacs.
https://asciinema.org/a/218654
GNU General Public License v3.0
317 stars 47 forks source link

MELPA updates #66

Closed jkdufair closed 3 years ago

jkdufair commented 3 years ago

Closes #44

@danielfm I think this should address all the requirements laid out in https://github.com/melpa/melpa/blob/master/CONTRIBUTING.org.

I've tested creating a recipe from my branch and it is working correctly when installed locally. If/when you accept this PR, I can make the PR to melpa. If you want to test the recipe, it's pretty straightforward:

(spotify-client :fetcher github :repo "jkdufair/spotify.el" :branch "feature/melpa" :files ("*.el"))

We will submit to melpa with your canonical repo, of course. Which may warrant a repo rename also?

There's a lot here. All renames. Every defun in every file is prefixed by spotify-client-filename. I renamed spotify-playlist-search to spotify-playlist and spotify-track-search to spotify-track for brevity and to account for the fact those modules manage more than searching, even though they implement a search mode.

Lmk if that sounds ok and/or if you see any other issues.

Thanks for considering!

jkdufair commented 3 years ago

Just occurred to me I need to update the readme with the new name and use-package instructions. Putting this into draft.

bnjmnt4n commented 3 years ago

I believe you might also need to specify the dependencies (oauth2, request, simple-httpd AFAIK) with Package-Requires headers.

bnjmnt4n commented 3 years ago

@jkdufair I've made a PR to your fork with the Package-Requires headers. With those changes, I'm able to install directly from my fork with dependencies built using straight.el (other package managers should probably work, although I haven't tested them).

jkdufair commented 3 years ago

@danielfm Well I think this is ready to go! Smudge is growing on me 😂. I fixed some concurrency issues as well with the initial fetch of the oauth2 token. I guess we'll want to rename the repo?

danielfm commented 3 years ago

@jkdufair Done!

jkdufair commented 3 years ago

@danielfm Fabulous! Let me know how it's working for you. Pretty solid here. I did discover that the initial fetch to get the active device isn't terribly effective. Even though spotify indicates there is a device active in the app itself, the API does not indicate it is active until it is actually playing music. Not much we can do about that, sadly. Otherwise I believe we are good to go!

danielfm commented 3 years ago

I'm hitting this error:

Debugger entered--Lisp error: (void-function browse-url-with-browser-kind)
  (browse-url-with-browser-kind 'external (concat auth-url (if (string-match-p "?" auth-url) "&" "?") "client_id=" (url-hexify-string client-id) "&response_type=code" "&redirect_$
  (let ((is-already-running (smudge-api-start-httpd)) (oauth-code nil)) (defalias 'httpd/smudge-api-callback #'(lambda (proc G0 G1 G2 &rest G3) (let ((--proc-- proc)) (let ((temp$
  smudge-api-oauth2-request-authorization("https://accounts.spotify.com/authorize" "xxx" "playlist-read-private playlist-read-collaborative ..." nil $
  (oauth2-request-access token-url client-id client-secret (smudge-api-oauth2-request-authorization auth-url client-id scope state redirect-uri) redirect-uri)
...

This is my init file:

(require 'smudge)

;; Settings
(setq smudge-oauth2-client-secret "xxx")
(setq smudge-oauth2-client-id "xxx")

(define-key smudge-mode-map (kbd "M-p") 'smudge-command-map)

(global-smudge-remote-mode)

Emacs version is 27.1.

Not sure if I'm doing something wrong.

jkdufair commented 3 years ago

Well, apparently that function is an Emacs 28 function. Bummer. It's so nice. Lemme try to find something else that will not try to launch eww to do oauth2 stuff.

jkdufair commented 3 years ago

@danielfm Ok, I found the emacs 27 function that should work. LMK how it goes.

danielfm commented 3 years ago

Testing now. Seems solid. Great work @jkdufair!

Will submit a more thorough review later.

danielfm commented 3 years ago

The screenshot (smudge-api/callback) is still different from the code (smudge-api-callback)

jkdufair commented 3 years ago

Oh FFS. Fixed. And resized. Thanks @danielfm

danielfm commented 3 years ago

Awesome!

jkdufair commented 3 years ago

Fabulous. I'll get a PR submitted to MELPA!

jkdufair commented 3 years ago

Looks like we are in @danielfm! Woo! Mind if I do a write up for Reddit?

danielfm commented 3 years ago

Go ahead 😄