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

Unify context menus #9257

Merged
merged 11 commits into from
Mar 17, 2016
Merged

Unify context menus #9257

merged 11 commits into from
Mar 17, 2016

Conversation

tamland
Copy link
Member

@tamland tamland commented Mar 3, 2016

This is a start at fixing current context menu code and move it to a global item based system (which will also support arbitrary nesting of menus). We already have this for addons, so this PR simply hooks up this system to allow non-addon menus to be added, and adds Interoperability with the old system.

Also adds context menu support to directory providers and ports a few menus as examples.

Next: port all current context menus to IContextMenuItem implementations and drop the old ones. Volunteers?:)

@tamland tamland added Type: Feature non-breaking change which adds functionality Type: Improvement non-breaking change which improves existing functionality WIP PR that is still being worked on labels Mar 3, 2016
@Tolriq
Copy link
Contributor

Tolriq commented Mar 3, 2016

@tamland do you think that after this PR it will be possible to extend that so it can be accessed from JSON RPC ?

@razzeee
Copy link
Member

razzeee commented Mar 3, 2016

Awesome, very good step into the right direction. I really hated all those add(menuItem) scattered everywhere.

@BigNoid
Copy link
Member

BigNoid commented Mar 4, 2016

Awesome indeed, this adds a wide range of possibilities.

@razzeee
Copy link
Member

razzeee commented Mar 4, 2016

@tamland
please pick razzeee@6112c08

and consider razzeee@222ae61

@razzeee
Copy link
Member

razzeee commented Mar 4, 2016

Pick 1d4a76f for the remaining musics info stuff

image
The bottom left menu part is missing for some reason, but might be skin related? @phil65

@phil65
Copy link
Contributor

phil65 commented Mar 4, 2016

yes, skin related.
@tamland latest estuary master already contains homescreen preparations so that the infoscreens mostly look fine.
Thx for this! Really awesome work.

@Montellese
Copy link
Member

Nice work (haven't looked at all the changes).

Personally I'd prefer the CTvShowInfo implementation example to be in its own class and part of the xbmc/video directory because that's where it belongs.

I'm also not a huge fan of having a huge list of "interface implementations" hardcoded in the manager initialization. Every implementation should be injected into the manager from some place where it belongs. But that's not easy to do (which is why I haven't solved it yet either in my media importing stuff).

@da-anda
Copy link
Member

da-anda commented Mar 5, 2016

one thing to keep an eye on is that if users modify an item in the info views (like slecting a new poster), that the underlying window needs to be notified to update the info of this item as well or it will show the old artwork/name f.e.

@tamland
Copy link
Member Author

tamland commented Mar 5, 2016

@Montellese If it wasn't clear, the implementations here was just proof of concept. I don't intend to have them all in one huge header file! The final connection of implementation the manager however is by design done in a central place atm. This is to keep it simple and straight forward, and it's required to have complete control of ordering here.

@razzeee I've take the liberty of refactoring the info menus because that turned into way too much boilerplate too fast:) As @Montellese said we should try to find appropriate places to keep the implementations. Keep them coming!

@razzeee
Copy link
Member

razzeee commented Mar 5, 2016

@tamland
did you miss 1d4a76f ? or should I refactor it accordingt to the new way?

@Montellese
Copy link
Member

@tamland: Ah I didn't realize that order matters. Maybe this should be documented in a comment somewhere near the definition or initialization of m_items.

@razzeee
Copy link
Member

razzeee commented Mar 8, 2016

@tamland
Pick razzeee@ad5df93

We will also need a xcode and vs sync when we're done with everything.

@tamland tamland force-pushed the unify_context_menu branch 4 times, most recently from 6928b5a to f35e665 Compare March 13, 2016 12:11
@tamland
Copy link
Member Author

tamland commented Mar 13, 2016

I've added a few more menus. I think I'll stop now, otherwise I'll never get done:)

Still looking for xcode. Anyone? @anaconda?

@razzeee
Copy link
Member

razzeee commented Mar 13, 2016

Should I look at vs sync again if you say your done for now?

@tamland
Copy link
Member Author

tamland commented Mar 13, 2016

Shouldn't be necessary. I've done it manually

@anaconda
Copy link
Contributor

@tamland Last 3 commits here add the new files to Xcode:
https://github.com/anaconda/xbmc/commits/9257-xcode

@tamland
Copy link
Member Author

tamland commented Mar 16, 2016

Thanks @anaconda!
jenkins build this please

@tamland
Copy link
Member Author

tamland commented Mar 16, 2016

jenkins build this please

tamland added a commit that referenced this pull request Mar 17, 2016
@tamland tamland merged commit c35b26d into xbmc:master Mar 17, 2016
@razzeee razzeee added this to the Krypton 17.0-alpha1 milestone Mar 17, 2016
@razzeee razzeee removed the WIP PR that is still being worked on label Mar 17, 2016
@NedScott
Copy link
Contributor

Is this all "under the hood" or does it need any changes to the wiki? (granted, the wiki isn't entirely up-to-date on context menu options as-is)

@tamland
Copy link
Member Author

tamland commented Mar 18, 2016

Mostly 'under the hood', yes, but there's a few smaller adjustments to the some of the current menus. There will likely be more changes before Krypton though, so I would wait with updating the wiki until it 'settle'.

@tamland tamland deleted the unify_context_menu branch March 18, 2016 08:46
@NedScott
Copy link
Contributor

That sounds good. Those will probably be picked up as we start to take new screenshots for everything, due to the new skin :)

tamland added a commit that referenced this pull request Mar 19, 2016
if (pItem && pItem->GetVideoInfoTag()->m_playCount > 0)
return OnContextButton(m_viewControl.GetSelectedItem(),CONTEXT_BUTTON_MARK_UNWATCHED);

if (pItem && pItem->HasAddonInfo())

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality Type: Improvement non-breaking change which improves existing functionality v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants