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

Add new folders in the main menu: "Newest", "Most viewed" and "Last chance" #55

Merged
merged 3 commits into from Jan 13, 2019

Conversation

goggle
Copy link
Contributor

@goggle goggle commented Nov 1, 2018

This PR adds three new folders to the main menu of the plugin: "Newest" (which contains a list of the most recent ARTE videos), "Most viewed" (which contains a list of the most viewed videos) and "Last chance" (which contains a list of videos that will soon be offline). So this PR allows the user of the plugin to browse through the ARTE videos in a more natural way.

Caveat: The API request does not return any deeper information about the videos, so there won't be any video description available for the videos in these three folders.

@known-as-bmf
Copy link
Owner

Maybe it's worth looking into this endpoint as well:

http://www.arte.tv/hbbtvv2/services/web/index.php/EMAC/teasers/home/v2/fr

There is a lot more information about the videos. The downside is that the lists are limited to 20 items.

{
    "teasers": {
        "highlights": [...],
        "playlists": [...],
        "creative": [...],
        "mostRecent": [...],
        "mostViewed": [...],
        "lastChance": [...]
    }
}

@goggle
Copy link
Contributor Author

goggle commented Nov 2, 2018

Hi @known-as-bmf,

Thanks for the suggestion. I have modified the code to use this suggested API request. I don't think that limit of 20 items is a problem. The lists should just be convenient for the user of this plugin to scroll through and see, if there is an interesting video to watch.

There is one problem though: The "mostRecent" section in the json contains only "Silex and the City" videos, but https://www.arte.tv/fr/videos/plus-recentes/ contains completely different videos. Maybe this is just a temporary flaw in the ARTE API. But if this is permanent, we cannot use this method.

What do you think?

@goggle
Copy link
Contributor Author

goggle commented Nov 13, 2018

The problem with the most recent videos seems to be resolved. What do you think about this PR?

@known-as-bmf
Copy link
Owner

I think the "most recent" videos actually are "most recently uploaded file in the paltform" rather than "last aired program"... Is it really useful to browse this items ?

@goggle
Copy link
Contributor Author

goggle commented Nov 13, 2018

I'm not certain about this, but it's indeed possible. When I tested it before, the listed videos (language of the plugin was set to de) were the same as listed under https://www.arte.tv/de/videos/neueste-videos/.

For me, it would be very useful to be able to browse the most recent videos. When I open the plugin, I usually want to see what's new and not have to click through all these topic menus. I guess many other users would like this too. See #52 for a somehow related proposal.

Copy link
Owner

@known-as-bmf known-as-bmf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, here is some feedback.

Apart from these minor things, this is very nice and I will merge it :)

@@ -30,6 +31,11 @@ def magazines(lang):
return _load_json(url).get('magazines', {})


def home_categories(name, lang):
url = _endpoints['home'].format(category_code='home', lang=lang)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

category_code not needed

or you could refactor the _endpoints dict to merge similar paths (category & home) like so:

_endpoints = {
    'categories': '/EMAC/teasers/{type}/v2/{lang}',
    'category': '/EMAC/teasers/category/v2/{category_code}/{lang}',
    'subcategory': '/OPA/v3/videos/subcategory/{sub_category_code}/page/1/limit/100/{lang}',
    'magazines': '/OPA/v3/magazines/{lang}',
    'collection': '/OPA/v3/videos/collection/{kind}/{collection_id}/{lang}',
    'streams': '/OPA/v3/streams/{program_id}/{kind}/{lang}',
    'daily': '/OPA/v3/programs/{date}/{lang}'
}

and api functions like

def categories(lang):
    url = _endpoints['categories'].format(type='categories', lang=lang)
    return _load_json(url).get('categories', {})


def home_category(name, lang):
    url = _endpoints['categories'].format(type='home', lang=lang)
    return _load_json(url).get('teasers', {}).get(name, [])

categories = [mapper.map_categories_item(
item) for item in api.categories(lang)]
categories = []
categories.append(mapper.create_newest_item())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can directly init category with the first menu items instead of using append

category = [
  mapper.create_newest_item(),
  mapper.create_most_viewed_item(),
  mapper.create_last_chance_item()
]

@goggle
Copy link
Contributor Author

goggle commented Jan 13, 2019

@known-as-bmf Thanks for the review. I've implemented the requested code review changes. Let me know if I should squash the commits or if there is anything else to do.

@known-as-bmf known-as-bmf merged commit b9a259a into known-as-bmf:master Jan 13, 2019
@known-as-bmf
Copy link
Owner

I squashed via the github pull-request merge :)

Thanks for the contribution !!

@goggle
Copy link
Contributor Author

goggle commented Jan 13, 2019

@known-as-bmf Thanks for merging this! Do you plan to make a new minor release soon, so that we can enjoy this feature?

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.

None yet

2 participants