FIX: Add missing "Play with" to playlist viewers #1689

Merged
merged 1 commit into from Nov 2, 2012

Conversation

Projects
None yet
3 participants
@koying
Contributor

koying commented Oct 28, 2012

Extracted from #1679

+ break;
+
+ VECPLAYERCORES vecCores;
+ if (item->IsVideoDb())

This comment has been minimized.

@elupus

elupus Oct 28, 2012

Member

Why is this needed. It's not done everywhere. If it's needed it feels like it would be something the playercorefactory should take care of.

@elupus

elupus Oct 28, 2012

Member

Why is this needed. It's not done everywhere. If it's needed it feels like it would be something the playercorefactory should take care of.

This comment has been minimized.

@koying

koying Oct 28, 2012

Contributor

It's a copy-paste from GUIWindowVideoBase.cpp:1203, i.e. the original "Play using" code.
I suppose it is remove both or none, but I wouldn't have arguments for removing it from the original code.

@koying

koying Oct 28, 2012

Contributor

It's a copy-paste from GUIWindowVideoBase.cpp:1203, i.e. the original "Play using" code.
I suppose it is remove both or none, but I wouldn't have arguments for removing it from the original code.

This comment has been minimized.

@elupus

elupus Oct 28, 2012

Member

Ok. then we leave it as is now and remove them in one sweep.

@elupus

elupus Oct 28, 2012

Member

Ok. then we leave it as is now and remove them in one sweep.

@elupus

View changes

xbmc/video/windows/GUIWindowVideoPlaylist.cpp
@@ -390,10 +390,17 @@ void CGUIWindowVideoPlaylist::GetContextButtons(int itemNumber, CContextButtons
}
else
- {
- if (itemNumber > -1)
+ {

This comment has been minimized.

@elupus

elupus Oct 28, 2012

Member

trailing whitespace added, please remove.

@elupus

elupus Oct 28, 2012

Member

trailing whitespace added, please remove.

This comment has been minimized.

@koying

koying Oct 28, 2012

Contributor

ack

@koying

koying Oct 28, 2012

Contributor

ack

@elupus

View changes

xbmc/video/windows/GUIWindowVideoPlaylist.cpp
- {
- if (itemNumber > -1)
+ {
+ if (itemNumber >= 0 && itemNumber < m_vecItems->Size())

This comment has been minimized.

@elupus

elupus Oct 28, 2012

Member

It would seem we previously assumed itemNumber was valid here. You can do the same, so this diff is not needed.

@elupus

elupus Oct 28, 2012

Member

It would seem we previously assumed itemNumber was valid here. You can do the same, so this diff is not needed.

This comment has been minimized.

@koying

koying Oct 28, 2012

Contributor

ack

@koying

koying Oct 28, 2012

Contributor

ack

@elupus

View changes

xbmc/video/windows/GUIWindowVideoPlaylist.cpp
+ if (vecCores.size() >= 1)
+ buttons.Add(CONTEXT_BUTTON_PLAY_WITH, 15213); // Play With...
+
+ if (CFavourites::IsFavourite(item.get(), GetID()))
buttons.Add(CONTEXT_BUTTON_ADD_FAVOURITE, 14077); // Remove Favourite

This comment has been minimized.

@elupus

elupus Oct 28, 2012

Member

This need the IsVideoDb() check too then.

@elupus

elupus Oct 28, 2012

Member

This need the IsVideoDb() check too then.

This comment has been minimized.

@koying

koying Oct 28, 2012

Contributor

Indeed. In the original code, the File Item is constructed only with the filename in that case ?!
I'll just copy-paste to keep the filiation.

@koying

koying Oct 28, 2012

Contributor

Indeed. In the original code, the File Item is constructed only with the filename in that case ?!
I'll just copy-paste to keep the filiation.

This comment has been minimized.

@elupus

elupus Oct 28, 2012

Member

Weird that it differs this way. but if it's the way it's done in the movie settings we should do the same. but it should probably be fixed up.

@elupus

elupus Oct 28, 2012

Member

Weird that it differs this way. but if it's the way it's done in the movie settings we should do the same. but it should probably be fixed up.

This comment has been minimized.

@jmarshallnz

jmarshallnz Oct 28, 2012

Member

Playlist window will contain only non-videodb items perhaps, as they're resolved prior to adding to the playlist?

@jmarshallnz

jmarshallnz Oct 28, 2012

Member

Playlist window will contain only non-videodb items perhaps, as they're resolved prior to adding to the playlist?

This comment has been minimized.

@elupus

elupus Oct 28, 2012

Member

Well the originating code was from the original video base window. ie not playlists. So it's strange.

@elupus

elupus Oct 28, 2012

Member

Well the originating code was from the original video base window. ie not playlists. So it's strange.

@elupus

View changes

xbmc/video/windows/GUIWindowVideoPlaylist.cpp
+ }
+ else
+ CPlayerCoreFactory::GetPlayers(*item, vecCores);
+ if (vecCores.size() >= 1)

This comment has been minimized.

@elupus

elupus Oct 29, 2012

Member

should be > 1 not >= 1. Change the music one too even if it's wrong in the place you copied it from.

@elupus

elupus Oct 29, 2012

Member

should be > 1 not >= 1. Change the music one too even if it's wrong in the place you copied it from.

elupus added a commit that referenced this pull request Nov 2, 2012

Merge pull request #1689 from koying/playlistplayusing
FIX: Add missing "Play with" to playlist viewers

@elupus elupus merged commit 0f6fb94 into xbmc:master Nov 2, 2012

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