Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix URIUtils::IsDVD function logic #83

Closed
wants to merge 2 commits into from

3 participants

Harry Muscle jmarshallnz Ian Firns
Harry Muscle

The logic in the URIUtils::IsDVD function is slightly off, so I've put together this pull request to fix it.

The original logic in that function checked to see if the file name was NOT "X:" or "X:\" (where X could be any letter) and return false if it's not. However, this prevents any of the other checks later in the function from ever being reached. The correct logic should be to check if the file name IS "X:" or "X:\" and if it is return true, otherwise continue running the rest of the function.

Thanks,
Harry

Ian Firns
Collaborator

Are you sure your logic is right here?

Firstly, De Morgan would require that && to be a ||. As it stands the latter is always true if the former is.

Secondly, as a 'nix dev, and having not used Windows for some time, would drives such as "C:" be considered a DVD? Your proposal suggests that any valid Windows drive is a DVD. Perhaps look at IsOnDVD and adapt as required.

The new logic will never be met (Mid(1) just drops the first character from the string).

The old logic is "more correct" in that it was designed to detect "D:\" or "D:" and fail on "D:\foo\bar" or "/foo/bar". Ofcourse, that's not ideal under anything non-win32 - and it specifically will fail on dvd:// yet the next check is supposed to handle it.

I suspect the best thing is to drop the first test completely, but someone needs to verify on all platforms.

Sorry guys for the confusion, I forgot to change && to ||. Hopefully with that change my pull will make more sense :)

This pull request has nothing to do with my other patches, I just simply came across this and noticed that it's messed up. As it stands currently, this function under Windows will almost always return false. The section that checks for "dvd://" will never be reached. I simply wanted to bring this to all the devs attention. Feel free to fix it in other ways if you'd like. I simply wanted to put together a patch that makes the least amount of code modifications while still fixing the function.

Thanks,
Harry

Harry Muscle

I noticed JMarshall fixed this a day or two ago, so I'll close this pull request.

Thanks,
Harry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 27, 2011
  1. Harry Muscle
  2. Harry Muscle

    Change AND to OR

    HarryMuscle authored
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 3 deletions.
  1. +3 −3 xbmc/utils/URIUtils.cpp
6 xbmc/utils/URIUtils.cpp
View
@@ -471,9 +471,9 @@ bool URIUtils::IsHD(const CStdString& strFileName)
bool URIUtils::IsDVD(const CStdString& strFile)
{
#if defined(_WIN32)
- if(strFile.Mid(1) != ":\\"
- && strFile.Mid(1) != ":")
- return false;
+ if(strFile.Mid(1) == ":\\"
+ || strFile.Mid(1) == ":")
+ return true;
if((GetDriveType(strFile.c_str()) == DRIVE_CDROM) || strFile.Left(6).Equals("dvd://"))
return true;
Something went wrong with that request. Please try again.