Context item addons #1654

Closed
wants to merge 26 commits into
from

Conversation

Projects
None yet
Contributor

Fice commented Oct 21, 2012

This PR added a new Addon Extension point called "xbmc.context",
The xml for such an Extension point looks like:

<extension point="xbmc.context" library="addon.py" visible="{VISIBILITY_EXPRESSION}">
      <label>Label</label>
      <label lang="de">Beschriftung</label>
 </extension>

where you can do the same in VISIBILITY_EXPRESSION as you would do in in skins via:

<visible></visible> 

Also added a new expression: addon.hasSetting() where you can query for addon settings.

Possibilities could be:

Contributor

Giftie commented Oct 21, 2012

I'm interested :)

If you're familiar with android's intents system, it's be nice to see something like that for this. For example, only add the context items if the list item url matches a provided regex, or if certain list item properties match given regex's

Contributor

bossanova808 commented Oct 22, 2012

Yeah I am interested too - all my ideas for other addons seem to be based of adding functionality to items like this. Maybe you should make a forum thread to discuss? Or have I missed one?

Contributor

Fice commented Oct 22, 2012

@bstrdsmkr No, I'm not familiar with the intents system. Could you given an example when it would be useful to let a regex over a path decide if it's visible? There is a lot of information that you wouldn't get by that, like any metadata of your media. I think it would be more useful to query them directly.

@bossanova808 Yeah, I'm in the same situation ;) You either need skin support (as if the skinners weren't busy enough ;) ) or they would be completely un-userfriendly to use... I haven't created a special Forum thread for that but I read a few feature suggestions that would then be doable as an add-on. Where should I open a thread? Feature Suggestions, Development or Python Add-on Development... depending on what you want to discuss, it could go in any of them.

Contributor

Giftie commented Nov 8, 2012

@Fice I'm really hoping this does get added to XBMC. You mentioned Cinema Experience which would really benefit from being able to be integrated into the context menu. Since you have a pull request already up, if you want discussion on it, you probably be best to open a thread in the Python Development thread.

tidalf referenced this pull request in tidalf/plugin.audio.qobuz Dec 23, 2012

Closed

context menu on local track can call qobuz addon... #26

Contributor

Fice commented Jan 18, 2013

did a rebase to master... still no answer from some dev if this can make it into mainline?

Member

alanwww1 commented Jan 18, 2013

Hi!

Thanks for the PR. It looks really interesting.
I am sure other devs will take time to review, but now it is time for us to get Frodo 12.0 out, so no we concentrate more on that. So a little later everyone will have more time.

For me, at first sight (as I am responsible for translation handling) my only question is that do we have to get translations to another place in the addon.xml file ? It is already hard for me to handle the description and stuff, keeping them in sync with translations up on Transifex.

Of course if it is the ONLY way to do, I will try to manage. Just I think the addon xml files might grow too big. Just like they already are with the 30+ languages we have for 3 translatable strings.

So if there is any other solution, where we could put these strings into the strings.po files, I'de much happier :-)

Thanks

Contributor

Fice commented Jan 19, 2013

Made some changes. The addon.xml should now look like:

<extension point="xbmc.context" library="addon.py" label="{LABEL}" visible="{VISIBILITY_EXPRESSION}">
 </extension>

where {LABEL} can either be a hardcoded string (if you don't need it to be translatable or for prototyping) or an integer that represents the id of the translatable string in the strings.po ;)

Disadvantage: When opening the context menu for the first time, we have to load, parse and extract the label string... When having only a few context item addons, i don't think that this would be a big of a deal.
Right now i also keep all the language strings in memory, maybe it makes sense to only keep the label in memeory and clear the rest?

@Fice : Do you know if team=XBMC is interested in this patch?

Contributor

Fice commented Feb 13, 2013

@bulkzooi Still waiting for a somewhat official answer, but in the official dev forum they said sth. like "if this gets added" (http://forum.xbmc.org/showthread.php?tid=154474)... so at least not a no ;) I had a great deal of positive feedback by python developers though.

Owner

MartijnKaijser commented Feb 13, 2013

It's definitely not a no however we first want context menu cleaned up and structured before adding more to it (i think).
Team discussion here: http://forum.xbmc.org/showthread.php?tid=154474

Contributor

Fice commented Feb 13, 2013

So, just removed the [WIP] ;)
You can now finally access the selected list item in your addon via "sys.item" not via the arguments passed. Also the problem with context item addons showing up in the middle of the list is also fixed ;)

Contributor

Fice commented Feb 13, 2013

already read it, I like the idea of it and unless you decide you want to go with nested contextmenus, I think it should be manageable to adapt ;)

Member

jmarshallnz commented Feb 13, 2013

@jimfcarroll your thoughts on the technique of getting the item back into python? The main thing I don't like with the way python add-ons (e.g. plugins) work is that transfer of information to the add-on is done only via the URL arguments. It would be nicer if we could set up a way to transfer more than strings, such as is done in this PR. This would also allow plugins to transfer the listitem object that the user clicked on (including any properties etc) rather than purely depending on the URL (though ofc, for repeatability from outside of browsing the directory hierarchy, we'd still want plugins to be able to generate listings purely based on the URL given).

Talking about subcontext menu's, there is also a special case: Play and then the next menu from start or here (see: http://trac.xbmc.org/ticket/5442)

Contributor

Fice commented Feb 14, 2013

kk, moved my ideas for further work on this PR here: http://forum.xbmc.org/showthread.php?

Owner

MartijnKaijser commented Feb 14, 2013

please discuss this on the forum so it won't spam everyone if it doesn't concern this PR directly

Member

da-anda commented Sep 2, 2013

what's the status of this? Context menu has been cleaned up a bit and this really seems to be a good feature (wondered why we didn't support it already). Can we pick up discussion on forum to get this baby in shape?
http://forum.xbmc.org/showthread.php?tid=144770

Contributor

Fice commented Oct 6, 2013

finally rebased, cleaned up and fixed some outstanding issues.

I also added a new "parent" attribute.
It can either be set to "core.manage" to add it to the new manage menu,
or it can be the ID of an addon implementing the new "xbmc.context.category" extension point in order to create custom context menus.

Here are all the test addons I wrote: Fice/xbmc@e9fca8f

Also updated the non OSX project files. Perhabs someone could ask jenkins?

Owner

MartijnKaijser commented Oct 6, 2013

if i read this correctly there's no way to localize the context menu entry?

Contributor

Fice commented Oct 6, 2013

There is:

<extension point="xbmc.context" library="addon.py" label="{LABEL}" visible="{VISIBILITY_EXPRESSION}">
 </extension>

where {LABEL} can either be a hardcoded string (if you don't need it to be translatable or for prototyping) or an integer that represents the id of the translatable string in the strings.po.

Owner

MartijnKaijser commented Oct 6, 2013

ok thx

jenkins build this please

Contributor

Fice commented Oct 6, 2013

grrr... boost/lexical_cast.hpp not found on windows.

Fixed by using atoi() instead

Member

jmarshallnz commented Oct 6, 2013

@Fice: Mind giving an overview of how you intend the parent/hierarchy stuff to work? It's not clear to me exactly what the desired relationship would be.

Contributor

Fice commented Oct 7, 2013

ACK to the rest. Almost done, but I have an appointment now, so will push later today.
@jmarshallnz: will draw you a diagram later today :)

Member

wsoltys commented Oct 7, 2013

grrr... boost/lexical_cast.hpp not found on windows.

its now there.

Contributor

Fice commented Oct 8, 2013

@jmarshallnz
everything done. haven't squashed yet, because that would remove the comments. just give the word and I'll do!

Regarding the tree structure:
The tree holds pointers of IContextItem where CContextItemAddons are leafs and CContextCategoryAddons are inodes. The CContextCategoryAddon derives from ContextMenuManager, because that one has a vector for the childs and has register/unregister functions. For the root I basically created a Singleton that makes the ContextMenuManager functions globally available. This Singleton is the root of our tree (called BaseContexMenuManager). This class also is responsible for initialisation of the whole tree.

Now I ran into some issues adding the 'manage' subtree into that base tree as well and decided there is no real benefit in doing so. Instead the CGUIDialogVideoInfo (responsible for the manage context menu) holds a seperate ContextMenuManager instance that contains the manage subtree.

@wsoltys
thanks. lexical_cast<> is my goto string conversion function. However in this case I don't need any of it's features so will stick with atoi().

Owner

Montellese commented Oct 8, 2013

strtol() someone? (before someone rips your head off for using atoi() ;-) )

Montellese was assigned Oct 17, 2013

jmarshallnz was assigned Oct 29, 2013

@topfs2 topfs2 assigned topfs2 and unassigned jmarshallnz Oct 12, 2014

@topfs2 topfs2 commented on an outdated diff Oct 14, 2014

xbmc/addons/IAddon.h
@@ -118,8 +120,6 @@ namespace ADDON
private:
friend class CAddonMgr;
virtual bool IsAddonLibrary() =0;
- virtual void Enable() =0;
- virtual void Disable() =0;
@topfs2

topfs2 Oct 14, 2014

Member

Is this really intended?

@topfs2 topfs2 commented on an outdated diff Oct 14, 2014

xbmc/dialogs/GUIDialogFavourites.cpp
@@ -129,7 +130,10 @@ void CGUIDialogFavourites::OnPopupMenu(int item)
choices.Add(3, 15015);
choices.Add(4, 118);
choices.Add(5, 20019);
-
+
+ CFileItemPtr itemPtr = m_favourites->Get(item);
+ BaseContextMenuManager::Get(). AppendVisibleContextItems(itemPtr, choices);
@topfs2

topfs2 Oct 14, 2014

Member

Remove the space between Get(). and AppendVisibleContextItems

@topfs2 topfs2 commented on the diff Oct 14, 2014

xbmc/interfaces/generic/LanguageInvokerThread.cpp
@@ -42,13 +43,14 @@ InvokerState CLanguageInvokerThread::GetState()
return m_invoker->GetState();
}
-bool CLanguageInvokerThread::execute(const std::string &script, const std::vector<std::string> &arguments)
+bool CLanguageInvokerThread::execute(const std::string &script, const std::vector<std::string> &arguments, const CFileItemPtr item /*= CFileItemPtr()*/)
@topfs2

topfs2 Oct 14, 2014

Member

Remove /*= CFileItemPtr() */ or make the parameter really optional

@topfs2

topfs2 Oct 14, 2014

Member

Oh sorry, nvm. Read the wrong cpp file when I wrote that. Disregard

Member

topfs2 commented Oct 14, 2014

To make sure if I understood the API properly (read through it quiet quickly), is it correct that an addon could specify this extension point in the addon.xml and then when called it will execute a script that addon provides?

topfs2 added the Helix label Oct 14, 2014

Contributor

Fice commented Oct 14, 2014

I use the same logic to call a context addon as other parts of kodi use to call scripts (except I pass an additional CFileItem). You can pretty much specify in 'library="python.py"' the same stuff you could specify in other addons.

I just rebased (most notably I moved some logic from the addon manager into the new OnPre/Post(Un)Install() hooks. Mostly c&p).
I also squashed the fixup commits and removed the white space issue.
Finally, I removed the commit that removes the enable/disable functions. (#5511).

@MartijnKaijser MartijnKaijser and 2 others commented on an outdated diff Oct 14, 2014

xbmc/addons/Addon.cpp
@@ -80,6 +80,8 @@ static const TypeMapping types[] =
{"xbmc.python.library", ADDON_SCRIPT_LIBRARY, 24081, "" },
{"xbmc.python.module", ADDON_SCRIPT_MODULE, 24082, "" },
{"xbmc.subtitle.module", ADDON_SUBTITLE_MODULE, 24012, "DefaultAddonSubtitles.png" },
+ {"xbmc.context.item", ADDON_CONTEXT_ITEM, 24025, "DefaultAddonContextItem.png" },
+ {"xbmc.context.category", ADDON_CONTEXT_CATEGORY, 0, "" },
@MartijnKaijser

MartijnKaijser Oct 14, 2014

Owner

while we are add it we should not keep continuing with using xbmc.foo extension. We need a more general one.

@MartijnKaijser

MartijnKaijser Oct 14, 2014

Owner

why shouldn't the category addon (assuming it's a separate addon) not be listed in repo?

@Fice

Fice Oct 14, 2014

Contributor

do you mean

#define ADDON_PREFIX "xbmc"
...
{ADDON_PREFIX "context.item", ...}

so 3rd party forks can easily change it, or renaming it to sth. generic {"sth.generic.context.item", ...}

Regarding Context Categories: I figured it doesn't make much sense to manually install a category if there are not going to be any items in it. If an item should be in a category it can add that category as a dependency (and you install that item directly). If we want the categories to show up in the addon manager, it might be nice to show them in the same hierarchy as in the actual context menu.
Going to Addon Manager -> Context Items: shows you all context items/categories without a parent. Clicking on a categorie will show you all items in that categorie. This seems more logical than having two differenct addon types that you browse seperately. That probably needs a little bit of additional work, but I could look into it if you want?

@MartijnKaijser

MartijnKaijser Oct 14, 2014

Owner

i mean something more generic for extensions as we are moving away from hardcoding XBMC/KODI anywhere (if possible or sane of course). Not sure what it should be though. t should certainly not depend on the appName as any fork will still use this framework. maybe "addon.context.menu" would be an option as it's clear enough and you only use such things within xbmc/kodi codebased apps. We should certainly not make the extension point depending on appName. (fork)

context categories: There have been some changes to our repo handling. I think consensus was that we should show them for doing the update/rollback option.

Guess it would be nice if you could have some some dummy addons available for testing as that would make it more clear. they don't have to be fully function though, just triggering a popup would be enough.
little sidenote, we discusses a possible redesign of our addon manager which i now made a public post for http://forum.xbmc.org/showthread.php?tid=206444 (no work has been done yet, just brainstorming) This is of course totally separate from this PR. just an fyi

@tamland

tamland Oct 21, 2014

Owner

@MartijnKaijser @Fice I think this better be reverted and left for a future change. The xbmc prefix is hardcoded e.g in CAddon::MeetsVersion. Might be more and have unforeseen consequences.

@MartijnKaijser

MartijnKaijser Oct 21, 2014

Owner

ah right. that sucks. Well this PR won't get in Helix anyway (imo). So guess a side PR could change CAddon::MeetsVersion to check for both.

@MartijnKaijser MartijnKaijser commented on an outdated diff Oct 14, 2014

xbmc/addons/Makefile
@@ -21,6 +21,8 @@ SRCS=Addon.cpp \
Service.cpp \
Skin.cpp \
Visualisation.cpp \
+ ContextItemAddon.cpp \
+ ContextCategoryAddon.cpp \
@MartijnKaijser

MartijnKaijser Oct 14, 2014

Owner

nitpick: all files list in alphabetical order. please place them according

@MartijnKaijser MartijnKaijser commented on an outdated diff Oct 14, 2014

language/English/strings.po
@@ -12074,7 +12074,11 @@ msgctxt "#24024"
msgid "Add-on disabled"
msgstr ""
-#empty strings from id 24025 to 24026
+msgctxt "#24025"
+msgid "Context Item"
@MartijnKaijser

MartijnKaijser Oct 14, 2014

Owner

add proper description that can be used by translator and in what file it is used (see .po for examples how to)

@MartijnKaijser MartijnKaijser commented on an outdated diff Oct 14, 2014

xbmc/Makefile.in
@@ -36,6 +36,7 @@ SRCS=Application.cpp \
XBDateTime.cpp \
xbmc.cpp \
XbmcContext.cpp \
+ GUIContextMenuManager.cpp \
@MartijnKaijser

MartijnKaijser Oct 14, 2014

Owner

please use alphabetical order

Owner

MartijnKaijser commented Oct 14, 2014

also our .xsd files need to be adapted for this

Contributor

Fice commented Oct 14, 2014

makefile and language file fixed.

.xsd files are the texture files used by skins, right? I only added the default image as a string, didn't actually create the image itself. @ronie Mind having a look into creating a default.png for context item addons?

Owner

MartijnKaijser commented Oct 14, 2014

https://github.com/xbmc/xbmc/tree/master/addons/xbmc.addon re: .xsd files. They define thye template for the .xml and we might be using them in future for checking xml structure. So we better keep them updated

Contributor

Fice commented Oct 14, 2014

renamed all kodi strings. Also added a few test addons (they all just pop up a notice, nothing special). have a look in the addon.xml for a short summary what this addon is supposed to test.

re: .xsd: those seem to only be for the repository/metadata extension point, so I have to create a new one, right? never dealt with .xsd files, will probably have to wait until tomorrow.

@tamland tamland commented on an outdated diff Oct 14, 2014

xbmc/interfaces/python/PythonInvoker.cpp
@@ -224,6 +230,17 @@ bool CPythonInvoker::execute(const std::string &script, const std::vector<std::s
if (m_argv != NULL)
PySys_SetArgv(m_argc, m_argv);
+ XBMCAddon::xbmcgui::ListItem* arg = new XBMCAddon::xbmcgui::ListItem(item);
+
+ m_item = PythonBindings::makePythonInstance(arg,
+ &PythonBindings::TyXBMCAddon_xbmcgui_ListItem_Type.pythonType,
+ true);
+ if (m_item != NULL)
+ {
+ if (0 != PySys_SetObject((char*)"item", m_item))
+ CLog::Log(LOGDEBUG, "CPythonInvoker(%d, %s): setSysParameter failed!", GetId(), m_sourceFile);
@tamland

tamland Oct 14, 2014

Owner

missing .c_str()

don't forget to rename the extension here as well to addon.foo

Member

ronie commented Oct 14, 2014

@ronie Mind having a look into creating a default.png for context item addons?

@universal recently recreated all the addon icons for Confluence (not yet merged, but pending),
perhaps he could also create a new one ?

Contributor

Fice commented Oct 14, 2014

Ofc I changed the strings AFTER I tested all addons ^^ addons are fixed and so is the c_str() (Nice catch)

Owner

MartijnKaijser commented Oct 14, 2014

i'll kick some build for live testing on all platforms

Contributor

Fice commented Oct 14, 2014

seems I borked the visual studio proj. files during rebase. hope it's fixed now

Owner

tamland commented Oct 15, 2014

and so is the c_str() (Nice catch)

Huh? I don't know what compiler you're using but gcc wont even compile it if you try to pass std sting..

@tamland tamland commented on an outdated diff Oct 15, 2014

xbmc/interfaces/python/PythonInvoker.cpp
@@ -224,6 +230,17 @@ bool CPythonInvoker::execute(const std::string &script, const std::vector<std::s
if (m_argv != NULL)
PySys_SetArgv(m_argc, m_argv);
+ XBMCAddon::xbmcgui::ListItem* arg = new XBMCAddon::xbmcgui::ListItem(item);
+
+ m_item = PythonBindings::makePythonInstance(arg,
+ &PythonBindings::TyXBMCAddon_xbmcgui_ListItem_Type.pythonType,
+ true);
+ if (m_item != NULL)
+ {
+ if (0 != PySys_SetObject((char*)"item", m_item))
@tamland

tamland Oct 15, 2014

Owner

I think this should be named "listitem" instead of the more generic "item" to make it clear we are talking about the gui list things called listitems here.

Owner

tamland commented Oct 15, 2014

Another thing: I don't know if you've though about this but the listitem you pass to python in mutable. Eg. calling setLabel on it will actually update the gui, temporarily. I don't know if it will have any other effects.

@tamland tamland commented on an outdated diff Oct 15, 2014

xbmc/video/windows/GUIWindowVideoNav.cpp
@@ -934,6 +934,8 @@ void CGUIWindowVideoNav::GetContextButtons(int itemNumber, CContextButtons &butt
buttons.Add(CONTEXT_BUTTON_PLUGIN_SETTINGS, 1045);
}
}
+
+ BaseContextMenuManager::Get().AppendVisibleContextItems(item, buttons);
@tamland

tamland Oct 15, 2014

Owner

Is this intentionally added here instead of one } up? It means context items will also be added to the parent item (".."), which normally don't have a context menu.

Contributor

Fice commented Oct 15, 2014

windows build finally fixed (although I don't know why that failed in the first place).

Python invoker now passes a copy to the scripts (no longer mutable). Also renamed sys.item to sys.listitem.
Context items are no longer available on "..." (at least in video windows, will have to check other windows tomorrow)

regarding .c_str(): on xcode (using gcc4.2) I only get warnings (wich are burried by all those format mismatch warnings). Perhabs you can get them as errors via some compiler flag?

Owner

MartijnKaijser commented Oct 15, 2014

when browsing addon manager it now shows "addon.context.category".

@MartijnKaijser MartijnKaijser added I* and removed Helix labels Oct 15, 2014

@tamland tamland commented on the diff Oct 16, 2014

xbmc/pictures/GUIWindowPictures.cpp
@@ -502,6 +502,8 @@ void CGUIWindowPictures::GetContextButtons(int itemNumber, CContextButtons &butt
CGUIMediaWindow::GetContextButtons(itemNumber, buttons);
if (item && !item->GetProperty("pluginreplacecontextitems").asBoolean())
buttons.Add(CONTEXT_BUTTON_SETTINGS, 5); // Settings
+
+ BaseContextMenuManager::Get().AppendVisibleContextItems(item, buttons);
@tamland

tamland Oct 16, 2014

Owner

Should probably think about where in all the GetContextButtons methods these calls are made. Each window is a bit different now. E.g. here, item can be null so the context menu can be opened on button and other gui elements.

@Montellese

Montellese Jan 28, 2015

Owner

Would be nice if we didn't have to manually add this to every GetContextButtons() implementation we have.

@tamland

tamland Jan 28, 2015

Owner

Yeah, it can be moved to CGUIMediaWindow, but then the addons will be added in the middle before the window specific ones instead of consistently last. I'm not sure what would be best..

@da-anda

da-anda Jan 28, 2015

Member

would some sort of "priority" parameter make sense? Some addons might add an entry for better usability that should be listet on top (like a delete button some forum users demand to be on first level and not below "manage")

@tamland

tamland Jan 30, 2015

Owner

If you mean setting default/user ordering, sure it could work. But that requires refactoring the whole context button lists. For now I suggest keeping things simple, so either just add them once in CGUIMediaWindow or add it in every window like now. (Or possibly a separate sub-menu for addons). PS: the "manage" menu is a completely separate menu for the video library so it doesn't transfer that easily over.

bstrdsmkr referenced this pull request in bstrdsmkr/1Channel Oct 27, 2014

Closed

select source in library #166

Contributor

Fice commented Nov 5, 2014

320ee53 changes the addon browser, so context items and context addons are merged in the same view, and they are shown in the correct hierarchy. That brings up one issue: right now a user can install a context category, but that's useless, because it will be empty and never show up unless he also installs a sub item (manually).
I think we have two options:
a) Disallow any actions like install/deinstall/enable/disable on context categories and only show them to visualise the hierarchy (probably easier)
b) If a user installs/deinstalls/enables/disables a category, we should also install/.../disable all the subitems. That's what I tend to. Last time I tried, I failed to setup a repository, will try again this weeked so I can work on this, unless someone prefers a) or has a differen idea.

eba1de1 adds .xsd definitions. I hope i got it right.

Also: #5657 rebrands all other extension points from xbmc.FOO to addons.FOO, so i will keep addon.context.item for now.

Member

phil65 commented Nov 6, 2014

First of all thank you for this PR, really a very nice feature.
Compiled and tested a bit and all seemed fine.
Two questions:

  • Is there a way to pass arguments to the python script? I did a quick test and it seemed as if did not work. (I put "addon.py test" into the xml library attribute)
  • What would be the best way to go if you want to add several context menu items for the same script? Creating some dummy add-ons which call the desired script (either by RunScript() or by JSON-RPC)? (I read that only one context menu extension point per add-on is possible)

Btw this also in some way "fixes" a regression:
In Gotham (and older xbmc versions) skinners were able to add custom context menu entries by putting button controls into a specific control group (id 996) in DialogContextMenu.xml. This still works in Helix, but it seems as if the visible conditions of the added button controls are not taken into account for the placement of the context menu bottom texture. In Helix this leads to a gap between the last button control and the bottom texture. (For exlanation: The context menu is automatically built from a top texture, then the button controls, then the bottom texture) In older XBMC versions the placement was correct, but since it is probably not intended that skinners start to put buttons into 996 at all this PR would help to get rid of that. :)

Contributor

Fice commented Nov 6, 2014

You want to specify parameters in the addon.xml that gets passed to the python.py? I haven't worked on anything in that regard. Is sth. like that possible for other extension points?

Several context items per addon is not possible (not really a concious choice, but the AddonMgr can one deal with one. so it would probably a bigger refactor). I think that this is actually a good think, it would avoid one addon cluttering up my context menu. So yeah several addons using RunScript() would be the way to go, I guess.

P.S. Nice to hear that this PR is actually a fix, let's hit that button ;)

Member

da-anda commented Nov 6, 2014

without knowing how this stuff works - would it be possible to simply pass the button list to the addon script and have the script do whatever it want's to? So like rearranging buttons, adding or even removing stuff? This would f.e. allow users to move functions that are now "hidden" in the manage button back to top level if they want to. Not that I would need this - just a general question for possible future changes.

Contributor

Fice commented Nov 6, 2014

there is no python involved in generating the context menu. The addons have a 'visible' and 'label attribute' in the addon.xml. So no rearranging or deleting possible. One idea I have in mind is eventually expose most of the context item logic via python (or json rpc or whatever) and make some current context items as addons (pre-installed). That way users can disbale/uninstall them, as they wish. (On top of the list is everything in the manage category)
A possible future feature could be an editor where you could be able to move context items up/down and perhabs even move them into a parent/child categorie. No idea how usable that would be though. Right now the context items are arranged by their installation order. Newest items are at the bottom.

Contributor

Fice commented Nov 6, 2014

the reason for not using python to generate the context menu, was responsiveness. I figured, calling several scripts to generate that list would be way too slow.

Member

da-anda commented Nov 6, 2014

nice. Thanks for the heads up about the implemenation. Your future plans sound good to me, but I don't think we'll need a rearrange feature etc. - once the contextmenu API is exposed to addons our python devs could write a manage/rearrange addon so it will be demanded by users - nothing we need in core IMO

Owner

tamland commented Jan 14, 2015

@Fice Are you up for a rebase of this PR? How about this, to scale the scope down a bit so this can actually get merged already!: drop the category addons for now. IMO the fact that empty dummy addons have to be created for categories is not ideal. Similarly, I imagine addons would commonly want to create a new category with multiple different items (for instance a trakt manager addon), which would result in a lot of addons. It means the context menu could possibly get crowded after some time, but at least installing them is optional.. Ideally it should be possible to define multiple items and categories in one addon. The parent and core.manage stuff can be kept I think, so people can item to that one category at least. You can also remove everything from AddonsDirectory.cpp. I will fix that in #5862. Until context item addons are added to the repo, it will be hidden anyway.

@Montellese Montellese commented on the diff Jan 28, 2015

xbmc/interfaces/python/PythonInvoker.cpp
@@ -224,6 +230,22 @@ bool CPythonInvoker::execute(const std::string &script, const std::vector<std::s
if (m_argv != NULL)
PySys_SetArgv(m_argc, m_argv);
+ if (item)
+ {
+ CFileItemPtr copiedItem = CFileItemPtr(new CFileItem(*item.get())); //use a copy of the item, so the python script cannot manipulate the item directly
+
+ XBMCAddon::xbmcgui::ListItem* arg = new XBMCAddon::xbmcgui::ListItem(copiedItem);
+ m_item = PythonBindings::makePythonInstance(arg,
+ &PythonBindings::TyXBMCAddon_xbmcgui_ListItem_Type.pythonType,
@Montellese

Montellese Jan 28, 2015

Owner

Do you really need the explicit API type here? AFAIK PythonBindings::makePythonInstance(arg, true) should be capable to determine this on its own as long as arg is an instance of a class derived from XBMCAddons::AddonClass.

@Montellese Montellese commented on the diff Jan 28, 2015

xbmc/interfaces/python/swig.h
@@ -1,3 +1,4 @@
+#pragma once
@Montellese

Montellese Jan 28, 2015

Owner

Either remove this again or remove the #pragma once that's already there below the license comment.

@Montellese Montellese referenced this pull request in tamland/xbmc Feb 17, 2015

@tamland Fice + tamland [ADDONS] Context Menu Addons System 807efb5
Owner

tamland commented Mar 5, 2015

Basic feature merged with #6316. Do continue work on the possible extensions mentioned here.

tamland closed this Mar 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment