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 possibility to set ListItems' context_menu by list of dicts #110

Open
dersphere opened this issue Apr 25, 2013 · 3 comments
Open

add possibility to set ListItems' context_menu by list of dicts #110

dersphere opened this issue Apr 25, 2013 · 3 comments

Comments

@dersphere
Copy link
Contributor

... instead of "list of two-element-length-tuples containing label and action" ;)

This would improve readability and would make it easier for beginners. Also this is a good place to use the new wrappers _run and _view

Draft:

items = [{
    'label': movie['title'],
    'info': {
        'year': '2013',
    },
    'context_menu': [{
        'label': 'Add to favorites',
        'action': _run(plugin.url_for(
            endpoint='add_to_fav',
            movie_id=movie['id'],
        )),
    }]
    'replace_context_menu': True,
    'is_playable': True,
    'thumbnail': 'http://foo.bar',
    'path': plugin.url_for(
        endpoint='play',
        movie_id=movie['id'],
    )
}]

Tell me what you think :)

@ulion
Copy link
Contributor

ulion commented Apr 27, 2013

since there's only the fixed 2 items in that dict, tuple will do well and type less words, what's benefit from this?
for the _run/_view, just use it in tuple, no much reason to use dict imo.

@jbeluch
Copy link
Owner

jbeluch commented Apr 28, 2013

@ulion hit the nail on the head. The reason it is lists of tuples is because each item is always a label and an action, so typing out dict keys is wordier, especially if you have multiple context menu items. However, @dersphere I agree with you that the dict version if more informative to beginners...

@dersphere
Copy link
Contributor Author

Sure it is wordier, but it's explicit :)
It could also be implemented to accept both (backward compatibility should be kept anyway).

But it was just an idea so feel free to close if you don't like :)

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

No branches or pull requests

3 participants