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

fix plugin context menus #9595

Merged
merged 4 commits into from Apr 14, 2016

Conversation

Projects
None yet
9 participants
@tamland
Copy link
Member

commented Apr 10, 2016

Fixes the ordering of context items from plugins which got messed up in recent refactoring.

I have also removed the broken 'replaceItems' option as I don't why plugins should be allowed to destroy menus coming from core and other addons, or what this would be useful for.

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 10, 2016

Not sure about the removal of that parameter, need to think about that first.

For listitems added via WindowXML.addItem() we definitely need some way to disable / replace the core/addon context menu entries though because for those listitems, script calls to self shouldnt be done async. Using WindowXML.onAction() makes much more sense here from a script author´s perspective.

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2016

@phil65 No idea what you're talking about tbh.. This is about the addContextMenuItems method which allows plugins replace the context menu in their own directory...

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 10, 2016

The issue I mentioned is not really related to this PR, but related to the contextmenu rework in general. At the moment the core context menu gets displayed for listitems added to WindowXML instances (via WindowXML.addItem() ). That should get disabled because it leads to some issues. (the one i mentioned, and also that the core context menu can trigger some container refresh which breaks the WindowXML.)
Would be cool if you could look into that.
If you need further details on how to test etc, drop me a message on slack.

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2016

So, ok to remove then?

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

I will take a look at the existing plugins tomorrow to see how big the fallout would be (this is definitely a far bigger API breaker than removing redundant params for .setInfo() ;) ) and comment then.

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2016

Except that API compatibility is kept. It doesn't break anything, it merely ignores the replace request..

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

...exactly the same as the change i did. It didnt break anything, setInfo() ignores not - used values. ;-)

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

lets agree to disagree and get back on topic shall we

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

Sure sure, just wanted to make sure that we use the same yardsticks. Both changes make Kodi simply ignore the request, there´s no real difference.

I git-grepped some of our plugin branches, my impression is that we should keep that replaceItems option for now. Quite a handful of plugins use it, and I could also imagine some edge-cases where it would make sense to have a custom context menu. @ronie, what´s your opinion?

@ronie

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

the 'replaceItems' option was added years ago when xbmc did show a lot of non suitable context menu items for plugins.
a lot has changed since then and afaik the only context menu items kodi currently adds in plugin containers are 'add to favourites' and 'play from here'.

i can't think of a usecase why those need to be removed by a plugin, so imo 'replaceItems' has served it's purpose and is no longer needed.

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

^ that was the actual question here; not if it's used, but if there are any real use cases for it. From what I can see in the addons in the repo, it's all api misuse/abuse and doesn't serve any purpose.

Also, @phil65: the difference is that removing an info label setter will cause addons using that label to no longer set it, and it turn no longer show it in the gui. Here, since there are no real reason to request them hidden, there is altering of any functionality provided by the plugin.

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

lets agree to disagree

Well, the question is more whether we want some grace period for those API-abusing plug-ins to fix their shit. Since I never was opposed to some breakage, feel free to press the green button. ;)

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2016

As I said, API is backwards compatible. No need for any grace period or to 'fix' anything. They will just work.
So jenkins build this please

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

I guess you are not aware what plug-in authors are doin nowadays :)
Stuff like this here: https://github.com/liberty-developer/plugin.video.prime_instant/blob/master/default.py#L1087-L1091
will lead to double entries for example.
EDIT: wrong link, sorry. anyways, I think you´ll get the point. Some add-ons emulate the core entries, so they will appear twice after this change.
EDIT2: correct link: https://github.com/Sandmann79/xbmc/blob/master/plugin.video.amazon/resources/lib/common.py#L156

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2016

How? None of those menus exist in core. Actually, this pr fixes that plugin as it breaks core functionality for no reason.

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

see updated link, that should be quite obvious ;) Anyways, no need to discuss this further. Merge it, no complaints here.

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2016

New link pass replaceItems=False. Already a duplicate lol.

Fin.

@tamland tamland merged commit 49ddc95 into xbmc:master Apr 14, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Merged build finished.
Details

@tamland tamland deleted the tamland:fix_plugin_context_menu branch Apr 14, 2016

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

You´re a smart guy, I am very confident that you understood the message. ;)

@ronie

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

i noticed some regression...

movies / tv shows / etc... libraries
the 'manage..' option is missing

videos > files
a lot of context menu options are missing

before:
before

after:
after

@Razzeee Razzeee added this to the Krypton 17.0-alpha1 milestone Apr 15, 2016

@Razzeee Razzeee added the v17 Krypton label Apr 15, 2016

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

@ronie 83f959f Pay attention guys:p

@spoyser

This comment has been minimized.

Copy link

commented on f6ea8bf Apr 19, 2016

Surely this is going to be added back at some point? Otherwise there is going to be some be some very cluttered menus appearing soon complete with functionality that doesn't actually work :(

This comment has been minimized.

Copy link
Member

replied Apr 19, 2016

Why would we add something back when it's removed on purpose.

This comment has been minimized.

Copy link

replied Apr 19, 2016

The title suggests It was removed because it got broke, surely it would be better to fix it? Removing it is simply a step backwards,

But then what would I know.

This comment has been minimized.

Copy link

replied Apr 19, 2016

Can someone give an indication as to why the decision to remove this very useful functionality was made?

This comment has been minimized.

Copy link
Member

replied Apr 19, 2016

Surely this is going to be added back at some point? Otherwise there is going to be some be some very cluttered menus appearing soon complete with functionality that doesn't actually work :(

could you please elaborate / provide some examples?
main reason for removal was that we couldn't think of a valid use case for this functionality.
(see my first comment on the PR).

@spoyser

This comment has been minimized.

Copy link

commented Apr 19, 2016

capture

The final 4 have need added due to the replaceItems being ignored.

A simple example I know.

I guess the biggest issue is that it also adds context menu addon extensions. I'm currently adding functionality to SF to allow various options too be added to the standard menu (athough broke in Krypton - see here http://trac.kodi.tv/ticket/16697) and these will certainly not be required whilst in the SF addon. I guess I'll need to add code to disable them when in SF.

Just seems pointless removing it.

@ronie

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

if kodi adds the 'Play' and 'Marked as watched' options on non-playable items, it's something that needs to be fixed in core imo.

'Add to favourites' doesn't seem to be a problem, you could simply drop the duplicate one your addon adds.

the 'Super Favourites Menu' one is added by a context menu addon i guess?
visibility of those can be controlled through the <visible> tag.

@spoyser

This comment has been minimized.

Copy link

commented Apr 19, 2016

The play and mark as watched are correctly added in the example, although it doesn't feel right them being at the bottom.

Add to favourites - yes I can remove the one I add, again the position isn't great.

And yes the visibility of the 'Super Favourites menu' can be controlled (as it already is to be switched on via a configurable setting), I guess I can somehow detect the title of the current container?

A lot of effort, for little gain.

My gripe is that useful functionality, ie the ability for an addon to fully customise it's own context menu has now been removed - and I cannot see any legitimate reason for this, if the functionality had never been there and I was asking for it to be added fair enough - but removing it???

AND what about other context menu addons? There could be any number of them in the future - at least have an option to disable them.

AND also, for Super Favourites the "built-in" Add to favourites will not work for all items, there is a method in Super Favourites for handling the adding to Kodi favourites as some items need modifying to work correctly (addToXBMC in default.py if you fancy taking a look). This is more or less a showstopper IMHO.

@spoyser

This comment has been minimized.

Copy link

commented Apr 20, 2016

Also, if I start Super Favourites from the Music menu (as a side note please can that be renamed Audio someday), I get this context menu for this item:

ActivateWindow(10502,"addons://sources/audio/",return)

music

Queue item, Play and Make default are all incorrect.

Secondly Super Favourites allows you to have the displayed items numerically prefixed, ie

01 | Item A
02 | Item B
03 | Item C
04 | Item D

Before being added to Kodi favourites, these require the prefix to be removed. Again the "built-in" Add to favourites doesn't do this. Again a showstopper IMHO.

Shall I carry on?

@spoyser

This comment has been minimized.

Copy link

commented Apr 20, 2016

menu

This is the menu you get on a Super Folder item within SF (when SF is started from the music menu), the Play, Mark as watched, Queue item, Play using... and Make default are all erroneous and make no sense.

Is that enough use cases yet?

@Montellese

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

IMO it is a bug in Kodi that these context menu items are added where they shouldn't (which needs to be fixed).

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

@spoyser If you haven't notices there have a lot of refactoring of the context menu lately to make it global and unified. That means broken code and useless "features" goes. All you are doing is post images of random context menu claiming it's 'incorrect' and 'making no sense', not presenting any use cases. If some context menu is shown where it shouldn't, that should be fixed. Not pile on more workarounds.

@spoyser

This comment has been minimized.

Copy link

commented Apr 20, 2016

@tamland - apologies for bringing bugs to your attention - would you rather I didn't bother.

They are not random images, they are examples as requested by @ronie

"could you please elaborate / provide some examples?"

Use Case - The builtin "Add to favourites" may not work under certain circumstance.

Example 1
In the Super Favourites addon (in the official org repo) the "built-in" Add to favourites does not work correctly for many items; there is a method in Super Favourites for handling the adding to Kodi favourites as some items need modifying to work correctly (addToXBMC in default.py if you fancy taking a look).

Example 2
In the Super Favourites addon (in the official org repo) the user has the option to display items numerically prefixed, ie

01 | Item A
02 | Item B
03 | Item C
04 | Item D

Before being added to Kodi favourites, these require the prefix to be removed. The "built-in" Add to favourites doesn't do this.

Obviously, these 2 use cases both relate to the Super Favourites, but I'm sure there will be others with similar problems that I'm not familiar with.

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

Menus like play and queue are showing because you are incorrectly setting playable attributes on non-playable items.

What's broken here is the utter mess that is super favourites. If you actually expect anything in that addon to work from revision to revision, I cant help you... Instead of complaining that one of your many hacks and workarounds broke, you should try finding proper solutions to you problems.

"Add to favourites" being available for things it won't work for, I can agree with, but this is global issue with that feature affecting all plugins, not related to this pr.

@spoyser

This comment has been minimized.

Copy link

commented Apr 20, 2016

WOW - JUST FREAKING WOW

And you wonder why the majority of users have very low opinions of Team Kodi. I'll bookmark this page as a reference for next time a holier than thou Kodi dev posts in the forum claiming how friendly and helpful you all are.

I'm really not sure what your beef is with SF, maybe it should be removed from the official repo if it is such a mess then - can you point to one hack or work-around?

Anyhow, before you start throwing accusations around simply because I'm trying to help, I have checked and I can confirm that I am setting the parameters correctly

A non-playable folder:
14:03:54 T:4368 DEBUG: *********** SF DEBUG OUTPUT ***************
14:03:54 T:4368 DEBUG: label Non-Playable Super Folder
14:03:54 T:4368 DEBUG: isFolder parameter True
14:03:54 T:4368 DEBUG: isPlayable parameter False

Gives this menu:
non-playable

And a playable folder:
14:05:19 T:5072 DEBUG: *********** SF DEBUG OUTPUT ***************
14:05:19 T:5072 DEBUG: label Playable Super Folder
14:05:19 T:5072 DEBUG: isFolder parameter False
14:05:19 T:5072 DEBUG: isPlayable parameter True

Gives this menu:
playable

They are obviously different so must be being treated differently in code (Play compared to Play using...) but the first one is incorrectly showing the Play item (etc)

"Instead of complaining that one of your many hacks and workarounds broke, you should try finding proper solutions to you problems." - I am, that's why I'm posting here.

Anyway, it is obvious from your replies that your not bothered about this issue and really you are just interested in arguing, so I'm signing-off from this thread,

I won't expect an apology for your false accusation - that would clearly be far too much to ask.

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

/me has seen this coming, but only got bashed for mentioning it. :)

@wjturner1978

This comment has been minimized.

Copy link

commented Apr 20, 2016

jeez..... someone has been drinking too much of their own coolaid... sf is one of the best coded plugins kodi has keep up the good work spoyser

@tamland

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

spoyser you're taking this way too personal. It's not:\

/me notes to self: just leave half-broken things to slowly die on their own next time. not worth the hassle..

@DixieDean

This comment has been minimized.

Copy link

commented Apr 20, 2016

tamland, you actually think some of your comments above are OK?
As someone looking in and reading from the outside, they are down right insulting.

You need to wind your neck in and show some respect. In my humble opinion.
Your arrogance is astounding.

@xbmc xbmc locked and limited conversation to collaborators Apr 20, 2016

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

guys, this is no place for stuff like that. I think both sides have their arguments, but that would be a discussion for the forum... Also remember that this is still pre-alpha, no need to go through the roof.

@phil65

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.