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

fixed: NFO/tag loading for http/https sources was broken (fixes #15481) #15492

Merged
merged 1 commit into from Feb 15, 2019

Conversation

@arnova
Copy link
Member

commented Feb 11, 2019

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

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
@arnova arnova referenced this pull request Feb 11, 2019
0 of 6 tasks complete
@arnova arnova requested a review from notspiff Feb 11, 2019
// don't try to read tags for streams
if (item.IsInternetStream())
return nullptr;

This comment has been minimized.

Copy link
@fritsch

fritsch Feb 12, 2019

Member

@notspiff you directly programmed it like this. Removing it without discussion sounds not right.

This comment has been minimized.

Copy link
@arnova

arnova Feb 12, 2019

Author Member

Alternatively we could move the NFO-reading up so it's before the IsInternetStream()-check, although I don't see why any of the stuff below it could be a problem for http/https.

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 13, 2019

Contributor

in principle there is no problem loading for http(s). but some servers will not like the Exists() and some for some streaming protocols it makes little sense. so i added it to lessen chance of issues and log spam. i don't see a big issue removing it as such, but that was the logic.

This comment has been minimized.

Copy link
@arnova

arnova Feb 13, 2019

Author Member

Yes, I already suspected that. Since we've ran into this issue before I think we should see whether we can find another way to flag/distinguish between actual "streams" and "files" served from http/https but that's beyond the scope of this PR.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Feb 13, 2019

Member

Also be aware of shoutcasts (internet radio). I don't know if they would get to this part of code, but I know that they are often over looked and we had issues with determining it was a shoutcast outside the player (the player knows but does not publish the fact)

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 14, 2019

Contributor

If somebody tries to scan a shoutcast station into their video library they deserve it all.

@arnova arnova added this to the Leia 18.1-final milestone Feb 12, 2019
@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

So ,this is good to go?

@notspiff

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Do it.

@arnova

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

build error unrelated

@arnova arnova merged commit 112d310 into xbmc:master Feb 15, 2019
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@DaVukovic DaVukovic referenced this pull request Feb 18, 2019
0 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.