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.

Sign up for GitHub

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

Jump to bottom

Added filter to limit the search results to stations or shows #34

Closed
wants to merge 11 commits into from
Closed

Added filter to limit the search results to stations or shows #34

wants to merge 11 commits into from

Conversation

foxfabi
Copy link
Contributor

@foxfabi foxfabi commented Dec 28, 2018

Hello, this is my proposition for #1
Let me know what you think about it.

@kingosticks
Copy link
Owner

Thanks for taking a look. Do all requests support filtering?

kingosticks
kingosticks reviewed Jan 2, 2019
View reviewed changes
mopidy_tunein/__init__.py Outdated Show resolved Hide resolved
mopidy_tunein/tunein.py Show resolved Hide resolved
kingosticks
kingosticks reviewed Jan 2, 2019
View reviewed changes
mopidy_tunein/tunein.py Outdated Show resolved Hide resolved
@kingosticks
Copy link
Owner

It would also be quite nice if we could somehow extend mopidy_to_tunein_query() to handle this so users can set the filter dynamically rather than having to hardcode it in their config. It'd be a bit ugly but we could shoehorn this into the comment, or maybe even the genre field (makes the most sense of any of them). I'm not totally sold on this idea, feel free to shoot it down.

@foxfabi
Copy link
Contributor Author

foxfabi commented Jan 3, 2019

Thanks for taking a look. Do all requests support filtering?

Yes, i can also limit a browse request only to shows or stations.

http://opml.radiotime.com/Browse.ashx?id=c57941
http://opml.radiotime.com/Browse.ashx?id=c57941&filter=p
http://opml.radiotime.com/Browse.ashx?id=c57941&filter=s

The 'filter' parameter affect the behavior for search and browse, but I have only considered him for the search.

@foxfabi
Adapted filter type naming to existing strings so that existing ones …
e8d386e 
…are used.
@foxfabi
Revert "Adapted filter type naming to existing strings so that existi…
60224f1 
…ng ones are used."

Make global names defined in translator.py available in tunein.py is known as a "cyclical dependency" and is a terrible idea.
We should put global names that need to be available to both modules in a third module.

This reverts commit e8d386e.
@foxfabi
Adapted filter type naming to existing strings so that existing ones …
9faf000 
…are used.
@foxfabi
Creating the filter string once #34 #discussion_r244737759
655a426
@foxfabi
Improved syntax code (coding standards check with flake8)
3fae47b
@foxfabi
Copy link
Contributor Author

foxfabi commented Jan 3, 2019

did you notice?
i'm still learning git and python 😄

kingosticks
kingosticks reviewed Jan 3, 2019
View reviewed changes
mopidy_tunein/ext.conf Outdated Show resolved Hide resolved
kingosticks
kingosticks reviewed Jan 3, 2019
View reviewed changes
mopidy_tunein/tunein.py Outdated Show resolved Hide resolved
kingosticks
kingosticks reviewed Jan 3, 2019
View reviewed changes
mopidy_tunein/tunein.py Outdated Show resolved Hide resolved
kingosticks
kingosticks reviewed Jan 3, 2019
View reviewed changes
mopidy_tunein/tunein.py Outdated Show resolved Hide resolved
@kingosticks
Copy link
Owner

kingosticks commented Jan 18, 2019 •
edited
Loading

Having finally gotten around to trying this out I see this filtering applied to everything breaks the results in some cases. For example:

  • Browse -> By Location -> Europe -> United Kingdom -> Liverpool

Returns plenty of results from TuneIn (http://opml.radiotime.com/Browse.ashx?render=json&id=r102038&filter=s) but Mopidy-Tunein doesn't show anything. Contrast that with London (http://opml.radiotime.com/Browse.ashx?render=json&id=r102038&filter=s) which does work and you can see the difference in the results format and why _flatten() goes wrong . The TuneIn API is total garbage.

@foxfabi
Copy link
Contributor Author

foxfabi commented Jan 18, 2019 •
edited
Loading

The pull request did not change the browser function or _flatten.

I can reproduce the described issue without applying this filtering to browse...
Mopidy-Tunein with Iris frontend: Browse> Music> 70's (works)
Mopidy-Tunein on Iris frontend: Browse> Music> 80's (empty result)
even though http://opml.radiotime.com/Browse.ashx?id=c100000781 delivers a result

Why there is no open issue about this?
What is your suggestion regarding further action?

@kingosticks
Copy link
Owner

Correct, you didn't change flatten, that's not what I said. But the way it's implemented it adds the filter param to every request. So, yes, it did change the browse results. I'll have a go at fixing it tomorrow.

@foxfabi
Removed additional newline in the group of Stdlib imports. Added cond…
dc01aed 
…ition to _tunein to check if search variant is used, if so we add the filter.
@foxfabi
Copy link
Contributor Author

foxfabi commented Jan 22, 2019

With the last commit now both Music> 80's and Liverpool show the corresponding results.

@kingosticks
Copy link
Owner

If we only want to apply the filter for searching, it should just be added to the args in search().

I'll have another round of testing this weekend just to make sure there are no search queries that end up with the same issue. If memory serves the search results are always in the same flat format so it should be OK. Having this filter feature available for browsing would be nice to get working but we can sort that out separately. I like this feature, I personally don't want the podcasts in my results - thanks!

@kingosticks
Copy link
Owner

Merged in 514ef19 with some adjustments. Thanks.

@kingosticks kingosticks closed this Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kingosticks kingosticks kingosticks left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@foxfabi @kingosticks