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

Playlists Browser: enable searching of contents #1593

Merged
merged 3 commits into from Apr 30, 2015
Merged

Conversation

@declension
Copy link
Member

@declension declension commented Apr 23, 2015

@lazka - as discussed really. Please have a look, we can either then merge or update first.

  • Add searchbar as per other browsers, mostly.
  • Query is normal QL syntax
  • Searches persist across playlists selection
  • Text-compatible filtering supported too (TODO: ~playlists tag should select playlist)

Closes #1588.

 * Add searchbar as per other browsers, mostly.
 * Query is normal QL syntax
 * Searches persist across playlists selection
 * Text-compatible filtering supported too (TODO: ~playlists tag should select playlist)

Closes #1588.
@lazka

This comment has been minimized.

Copy link
Member

@lazka lazka commented on quodlibet/quodlibet/browsers/playlists/main.py in 9bf9a12 Apr 23, 2015

Switching to the browser makes the queue show. I guess you have to show only your added widgets, or show_all() before adding the songpane.

@lazka

This comment has been minimized.

Copy link
Member

@lazka lazka commented on quodlibet/quodlibet/browsers/playlists/main.py in 9bf9a12 Apr 23, 2015

Does this check if the passed song is also in the active playlist?

@declension
Copy link
Member Author

@declension declension commented Apr 25, 2015

@lazka - Thanks.

Weird, I can't replicate that UI behaviour; the queue stays shut for me, and if I don't show the songpane, I don't get a songpane at all it seems.

As for the active_filter()... I see. Is it in fact possible to remove it here entirely, as newly added songs will never be auto-added to a playlist (well, not without a whole lot new work)? That said, songs removed from the library should get removed from the songlist I guess, and it seems safer to implement it as a filter on both active playlist and text filter... so maybe I'll do that.

@lazka
Copy link
Member

@lazka lazka commented Apr 26, 2015

I mean if you hide the queue completely, not just close the expander.

for active_filter(), removals are still handled in the song list directly atm, so you can probably just remove it for now.

 * As per review comments
 * `active_filter` now checks song is in currently selected playlist too
 * Add tests to capture current intended (probably) behaviour of this
@declension
Copy link
Member Author

@declension declension commented Apr 26, 2015

Oh, right, I forgot you could even do that (again...)!

I've fixed that behaviour in the packing (AFAICT), and as for the active_filter, made it consistent with the others for now.

Perhaps we can think about cleaning that interface up at some point? IMO it feels a bit inconsistent with the rest of the Browser interface, or something.

@declension
Copy link
Member Author

@declension declension commented Apr 30, 2015

@lazka - are you happy for me to merge the updated version?

lazka added a commit that referenced this pull request Apr 30, 2015
Playlists Browser: enable searching of contents
@lazka lazka merged commit 80c42cd into master Apr 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants