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.

Sign up for GitHub

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

Jump to bottom

Fix stream playlists as directory #14284

Merged
merged 2 commits into from
Aug 10, 2018
Merged

Fix stream playlists as directory #14284

merged 2 commits into from
Aug 10, 2018

Conversation

vrichter
Copy link
Contributor

@vrichter vrichter commented Aug 9, 2018

Description

Workaround for broken STRM playback from favorites after #14183

Motivation and Context

Patch #14183 introduced a regression, preventing playlists with streams from being played correctly.
This fixes the problem.,

How Has This Been Tested?

Ubuntu 18.04, self built master. Tested playback of local playlist and playlist with stream.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@vrichter vrichter mentioned this pull request Aug 9, 2018
Merged
12 tasks
@vrichter
Copy link
Contributor Author

vrichter commented Aug 9, 2018

This patch does not change much but requires the indentation of a block of code. That is why it looks as if much was changed. The first commit holds the changes in code. The second is only cosmetic.

As a drawback: This solution still tries to expand the playlists as a directory.
In the case of stream files this results in a not a directory error being logged.

Rechi
Rechi reviewed Aug 9, 2018
View reviewed changes
xbmc/interfaces/builtins/PlayerBuiltins.cpp Outdated
@@ -435,39 +435,43 @@ static int PlayMedia(const std::vector<std::string>& params)
std::string extensions = CServiceBroker::GetFileExtensionProvider().GetVideoExtensions() + "|" + CServiceBroker::GetFileExtensionProvider().GetMusicExtensions();
XFILE::CDirectory::GetDirectory(item.GetPath(), items, extensions, XFILE::DIR_FLAG_DEFAULTS);

bool containsMusic = false, containsVideo = false;
for (int i = 0; i < items.Size(); i++)
if (items.Size() != 0) // fall through on non expandable playlist

This comment was marked as spam.

Sign in to view

@Rechi Rechi requested a review from notspiff August 9, 2018 19:35
@notspiff
Copy link
Contributor

wanna take a stab at removing the historic error that strm files are playlists?

@notspiff
Copy link
Contributor

oh, and please squash the fix commit into the first one.

@vrichter
Copy link
Contributor Author

squashed and rebased.

@vrichter
Copy link
Contributor Author

wanna take a stab at removing the historic error that strm files are playlists?
I can look into it if i find the time but I cannot estimate the side effects of such a change.

Nonetheless, this fix is still needed because m3u playlists can be lists of streams too.

The strm problem should be a different pull-request.

@notspiff
Copy link
Contributor

Absolutely, i have already approved this..

@FernetMenta
Copy link
Contributor

wanna take a stab at removing the historic error that strm files are playlists?

strm files can be anything, including playlists, i.e. a strm file can contain a m3u URL

@vrichter vrichter mentioned this pull request Aug 10, 2018
Closed
13 tasks
@vrichter
Copy link
Contributor Author

As I could not find any definition of what a strm file may or may not contain, they sure can be what ever they want :-)
Feel free to reject or accept #14286 when you agree on the usage.

@notspiff notspiff merged commit c074f25 into xbmc:master Aug 10, 2018
@FernetMenta
Copy link
Contributor

you are right, the strm by its own can never be a playlist. sorry for the noise

@Rechi Rechi added Type: Fix non-breaking change which fixes an issue v18 Leia labels Aug 11, 2018
@Rechi Rechi added this to the Leia 18.0-beta1 milestone Aug 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rechi Rechi Rechi left review comments

@notspiff notspiff notspiff approved these changes

Assignees
No one assigned
Labels
Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Milestone
Leia 18.0-beta1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vrichter @notspiff @FernetMenta @Rechi