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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Soundcloud browser #1990

Merged
merged 48 commits into from Jul 24, 2016

Conversation

Projects
None yet
2 participants
@declension
Member

declension commented Jul 19, 2016

See #1828 for background.

This collection of, err, 40 34 (+ merge) commits is what hopefully now represents a minimum viable (and compliant, as far as my checks have gone) feature for Soundcloud, plus a bunch of "free extras" I did along the way (see checklists in #1828).

The technical commentary is generally in the various detailed commit messages (so please don't squash).

Seems working from here now, but probably contains a few bugs / inefficiencies / gotchas still so it would be good to validate this on a few different systems. Features / improvements can be added in later PRs, as this one has got far too big really... 馃槈

declension added some commits Feb 21, 2016

Add basic Soundcloud browser.
 * Uses HTTP helpers from `soundcloud` package (required, for now)
 * Authenticates via a callback URI that is now handled in-browser
 * Basic searching added, only supports 50 tracks but builds up in-memory
   library / cache
 * Best-efforts conversions of Soundcloud's API fields to QL tags including artwork
 * Will play high-quality original (download) link where available
 * Some parsing tests added
Soundcloud: added cover art source plugin
...called `artwork-url-cover` as it searches the `artwork_url` tag, which is (currently)
only generally populated by the experimental Soundcloud Browser
Soundcloud: fix library re-init, increase track limit
  * Limit 50 -> 100; not much performance hit and well, double the results...
  * Tidy / namespace model / view constants
  * Remove redundant "no cat" (non) filter entry
  * Make STAR relevant to all non-text headers, seems less arbitrary
Soundcloud: split & rewrite API using libsoup
  * Split browser into a package structure
  * API / Library now lives separately (though still here...)
  * Ditched `soundcloud` package, now uses "native" `gi.Soup` as Cover art etc
  * Some tweaking of the common `http` utils
  * Use signals to communicate events _from_ the library
  * Slow browser update down (having allowed this in `searchbar`)
  * Tweak column headers to allow `~people`, etc
  * Tidy browser init code
@lazka

This comment has been minimized.

Member

lazka commented on 4a335e0 Mar 3, 2016

I guess you need gir1.2-soup-2.4

This comment has been minimized.

Member

lazka replied Mar 3, 2016

> locate Soup-2.4
/usr/lib/girepository-1.0/Soup-2.4.typelib
/usr/share/gir-1.0/Soup-2.4.gir
> dpkg -S /usr/lib/girepository-1.0/Soup-2.4.typelib
gir1.2-soup-2.4: /usr/lib/girepository-1.0/Soup-2.4.typelib

This comment has been minimized.

Member

declension replied Mar 3, 2016

Thanks @lazka
I guess the only reason this was working all along (the album art / cover source plugins use the same version) is that they can fail safely (and are not tested anyway AFAICT).

Should I add that package to Travis? Does anything else need updating?

This comment has been minimized.

Member

lazka replied Mar 7, 2016

Should I add that package to Travis?

Sounds good.

Does anything else need updating?

no idea

This comment has been minimized.

Member

declension replied Mar 7, 2016

Added it in a4e3d75, and build went green.

Bit worrying that those cover plugins (and http.py) must have been broken in CI all along, but I also can't think of any so when I get some time I might put a few in.

We don't have a requirements page any more do we (that's what I was thinking of really)?

This comment has been minimized.

Member

lazka replied Mar 7, 2016

We don't have a requirements page any more do we (that's what I was thinking of really)?

Do you mean this? https://quodlibet.readthedocs.org/en/latest/packaging.html

This comment has been minimized.

Member

lazka replied Mar 7, 2016

I've added libsoup as a hard dep to the PPAs/repos

This comment has been minimized.

Member

declension replied Mar 7, 2016

Thanks!

declension added some commits Mar 3, 2016

Soundcloud: update auth URL to new hosted page
 * ...this does the same redirect as before in the end,
 but keeps the auth token visible to the user, with help.
 * TODO: Currently there _is_ no way of using this code manually though
 other than editing the config file...
@lazka

This comment has been minimized.

Member

lazka commented on 4a335e0 Mar 3, 2016

I guess you need gir1.2-soup-2.4

This comment has been minimized.

Member

lazka replied Mar 3, 2016

> locate Soup-2.4
/usr/lib/girepository-1.0/Soup-2.4.typelib
/usr/share/gir-1.0/Soup-2.4.gir
> dpkg -S /usr/lib/girepository-1.0/Soup-2.4.typelib
gir1.2-soup-2.4: /usr/lib/girepository-1.0/Soup-2.4.typelib

This comment has been minimized.

Member

declension replied Mar 3, 2016

Thanks @lazka
I guess the only reason this was working all along (the album art / cover source plugins use the same version) is that they can fail safely (and are not tested anyway AFAICT).

Should I add that package to Travis? Does anything else need updating?

This comment has been minimized.

Member

lazka replied Mar 7, 2016

Should I add that package to Travis?

Sounds good.

Does anything else need updating?

no idea

This comment has been minimized.

Member

declension replied Mar 7, 2016

Added it in a4e3d75, and build went green.

Bit worrying that those cover plugins (and http.py) must have been broken in CI all along, but I also can't think of any so when I get some time I might put a few in.

We don't have a requirements page any more do we (that's what I was thinking of really)?

This comment has been minimized.

Member

lazka replied Mar 7, 2016

We don't have a requirements page any more do we (that's what I was thinking of really)?

Do you mean this? https://quodlibet.readthedocs.org/en/latest/packaging.html

This comment has been minimized.

Member

lazka replied Mar 7, 2016

I've added libsoup as a hard dep to the PPAs/repos

This comment has been minimized.

Member

declension replied Mar 7, 2016

Thanks!

declension added some commits Mar 13, 2016

Soundcloud: tidy API & add rating support
 * Tidy API up, extracting generic(ish) REST layer to a base class
 * Add `PUT` and `DELETE` support for HTTP calls
 * Make more methods private
 * Extend `RemoteFile` to new `SoundcloudFile` which takes its own API instance (ugh)
   thus allowing basic two-way integration with SC.
 * Currently this only supports ~#rating:
        > DEFAULT => add to favourites
        < DEFAULT or no rating => remove from favourites
...and fix PyFlakes violations
..one day I'll learn. At least I've added auto-trailing newline...
Soundcloud: Log out & UI improvements:
 * Use branded icons. Currently from disk. TODO: not packaged at all
 * Allow logging out
 * Move the connect button, make it image
 * ...and make it stateful according to API status (logged in or not)
 * Tidy existing UI code a little
Soundcloud: support track comments and more
 * Hook into currently playing song
   (bit too heavy on network to do each query result async,
   though could still be an option for top `n` results perhaps)
 * New `comments-received` signal  emitted by api
 * Make SC song key actually the track_id, not the full URL (with token...)
 * Fix bug with missing description
 * Reduce debug logging
Soundcloud: Try parsing queries for search terms
 * Not specifically SC at all
 * Introspect the query tree to pull searchable text terms from Nodes
 * As the API probably uses search term weighting, the important difference between Inter and Union is less pronounced - we can hopefully fire *all* terms at the API and hope for the best.
 * TODO: prove / improve this behaviour though
 * Some dirty, best-efforts regex -> plaintext munging
 * As such, no non-trivial regexes are supported, but then
   the API doesn't either (in fact none do AFAIK) so there was little future for these.
 * TODO: support for limited tags sets, stripping of regex special chars (better)
Soundcloud: Improve QL query parsing, and expose as a map
 * Involves exposing internals of Query nodes a bit
 * Return terms as a map so that we could then send relevant fields
   to the API (e.g. length limits), not just everything in the
   free-text search
 * Add some tests to prove this
Merge branch 'master' into soundcloud-browser
# Conflicts:
#	quodlibet/quodlibet/qltk/searchbar.py
Soundcloud: queries now validated and translated
 * Extract this query code out
 * maps cleverly QL -> SC
 * Throws when not supported, but caught at QL level
 * Still filters using QL after ingestion of aPI results, so good quality matches
  * Update tests

declension added some commits Jun 27, 2016

Soundcloud: fix unnecessary rating traffic
 * These were being sent regardless of current status, so cache the SC favourite info, and only write when necessary
 * Begrudgingly use American English for `favou?rit[es?|ing]` :)
 * Also make `track_id` a mandatory field for `SoundcloudFile` as things go quite wrong without it.
 * Add some tests
Merge branch 'master' into soundcloud-browser
# Conflicts:
#	quodlibet/quodlibet/util/http.py
#	quodlibet/quodlibet/util/songwrapper.py
Log any AudioFileErrors in songwrapper
...UI messages are nice, but harder to investigate
Soundcloud: implement favo(u)rites + bugfixes
 * Favourites now added as an explicit "filter". Clicking this disables the query entry - seemed the easiest UI to get going here
 * API query added for getting favourited tracks
 * Fix updated API columns: `~#favoritings_count` is now what `likes_count` was, AFAICT.
 * Catch query parsing failures both at start-up and elsewhere
 * Add concept of "untranslatable but OK" tags (currently just `~#rating`) which mean whilst they won't affect the outgoing API query, they will still be parsed by QL on its own library items.
 * Add favourite icon, use for "filters" pane
@lazka

This comment has been minimized.

Member

lazka commented Jul 20, 2016

Two blockers imo, but if that's get handled after merge, fine with me.

  • I don't think we can ship the icons
  • Does the login still depend on a custom URI scheme? We need something that works on OSX/Windows.
@declension

This comment has been minimized.

Member

declension commented Jul 20, 2016

Thanks

I don't think we can ship the icons

Ah, yes that. What's the specific concern - GPL? Conversely, SC seems to be quite keen on always displaying the icons ("If you are using SoundCloud's data in your app or site, we ask that you place these logos where the data is being used/shown" etc). Might need to look at that web-caching idea you had then...

Does the login still depend on a custom URI scheme? We need something that works on OSX/Windows.

It still uses it where available (it's definitely a UX win), but I added a third state for the button (logging in) whereby you can enter the code manually copied from the redirect page (even pastes it from the clipboard by default). This should allow all users then, but I think the OS X registration might be doable too

@lazka

This comment has been minimized.

Member

lazka commented Jul 21, 2016

I don't think we can ship the icons

Ah, yes that. What's the specific concern - GPL? Conversely, SC seems to be quite keen on always displaying the icons ("If you are using SoundCloud's data in your app or site, we ask that you place these logos where the data is being used/shown" etc). Might need to look at that web-caching idea you had then...

A Gtk.Image subclass which does an async http request shouldn't be too hard. I can look into that if you want.

Does the login still depend on a custom URI scheme? We need something that works on OSX/Windows.

It still uses it where available (it's definitely a UX win), but I added a third state for the button (logging in) whereby you can enter the code manually copied from the redirect page (even pastes it from the clipboard by default). This should allow all users then, but I think the OS X registration might be doable too

Yeah, I've used the manual code input. But once you log in it redirects you to a browser error page, and you have to figure out that part of the URL has to be copied to QL. No UX win in that case.

@declension

This comment has been minimized.

Member

declension commented Jul 21, 2016

Redirect problem resolved in quodlibet/quodlibet.github.io@f4892bd

Merge branch 'master' into soundcloud-browser
# Conflicts:
#	quodlibet/quodlibet/cli.py
@declension

This comment has been minimized.

Member

declension commented Jul 22, 2016

A Gtk.Image subclass which does an async http request shouldn't be too hard. I can look into that if you want.

@lazka that would be great, if you have time

declension added some commits Jul 22, 2016

Fix: URI was being used in Soundcloud code
...and compiled class still existed locally :(
@lazka

This comment has been minimized.

Member

lazka commented Jul 23, 2016

here is a basic implementation: https://gist.github.com/lazka/87b59993b3cf01ed420d78dda03e1eac

Soundcloud: Remove all local brand images:
 * Add `WebImage` (thanks to @lazka), wire in (with helper function)
 * Remove all local brand images and references
 * Add button tooltip back as it's more likely it will be useful now
@lazka

This comment has been minimized.

Member

lazka commented Jul 24, 2016

Nice, one more thing: As the images are defined globally the http requests happen on every QL start, even if another browser is selected.

Either move them locally, or use our @cached_func decorator.
Or we could only start loading them when the mapped in screen, but I'd prefer the first solution as I'd like to avoid creating Gtk.Widget instances at import time.

declension added some commits Jul 24, 2016

Soundcloud: Restore likes_count as.. it's working again.
 * ...most of the time, anyway. Grr.
 * `favoritings_count` is now zero for all requests I've tried today, but it seems safer just to keep these all.
 * Extract these a bit better
 * Move constants into the class, for tidiness.
Merge branch 'master' into soundcloud-browser
# Conflicts:
#	quodlibet/quodlibet/query/_match.py
@lazka

This comment has been minimized.

Member

lazka commented Jul 24, 2016

Sorry for the conflicts.

Feel free to merge

@declension

This comment has been minimized.

Member

declension commented Jul 24, 2016

No worries - merging then :)

@declension declension merged commit d9ff4aa into master Jul 24, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@declension declension deleted the soundcloud-browser branch Jul 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment