Offline Media Stub Files (Feature Request)
#31
(2017-01-06, 15:12)da-anda Wrote: @dml3334 have a look for a file containing "Woerd88-patch-2-af8297b" on http://mirrors.kodi.tv/test-builds/win32/ in like 1h
Hi.

I have just tried the file KodiSetup-20161230-af8297b-patch-2 in a laptop with DVD drive, because I won't have a PC without DVD drive until Sunday.

This way, a new windows appears with the following information:

Please insert disc
Please insert the following disc:
<title_inside_the_disc_stub_file>
<message_inside_the_disc_stub_file>

Then, if you press ESC, a new window appears saying:

Playback failed
One or more items failed to play. Check the log for more information about this message.

I think that this message, due to the

return PLAYBACK_FAIL;

is a little ugly... If you try to abort, it is not necessarily a failure [emoji6]

What do you think?

I'll be posted. Thanks for having done the build!
Reply
#32
(2017-01-06, 17:37)dml3334 Wrote:
(2017-01-06, 15:12)da-anda Wrote: @dml3334 have a look for a file containing "Woerd88-patch-2-af8297b" on http://mirrors.kodi.tv/test-builds/win32/ in like 1h
Hi.

I have just tried the file KodiSetup-20161230-af8297b-patch-2 in a laptop with DVD drive, because I won't have a PC without DVD drive until Sunday.

This way, a new windows appears with the following information:

Please insert disc
Please insert the following disc:
<title_inside_the_disc_stub_file>
<message_inside_the_disc_stub_file>

Then, if you press ESC, a new window appears saying:

Playback failed
One or more items failed to play. Check the log for more information about this message.

I think that this message, due to the

return PLAYBACK_FAIL;

is a little ugly... If you try to abort, it is not necessarily a failure [emoji6]

What do you think?

I'll be posted. Thanks for having done the build!
Hi.

I have just tested it in the laptop. I removed physically the DVD tray.

It works!! [emoji106]

The same windows as stated before, with the DVD tray put, appear.

Can this patch be proposed for Kypton RC3? It would be very nice to have this issue solved at last asap in an official version.

Thanks again, mates!
Reply
#33
@dml3334
Thanks for testing

@da-anda
Thanks for the build, i hope i can make a local build for myself next time.

Is the comment from dml3334 about the return value the same issue you thought of when you reviewed the code?
I was also a bit hesitating about the precompile define HAS_DVD_DRIVE, because now systems where the platform doesn't support optical disc also don't show stub files.
But i didn't want to break too much on my first try Big Grin
Reply
#34
my comment was about the define which IMO shouldn't be needed anymore in this case. Haven't checked the return value stuff though. Didn't have time to look at the entire code section and only looked at the changed lines on the github diff. But I'm probably not the right person to review anyways, as I'm not really a core dev.
Reply
#35
Hi!

I have some good news.

I installed everything in my computer to be able to compile Win32 Kodi.

I made some changes in Application.cpp, so it would work for all configurations:

- With DVD Drive: As usual. There would be a DialogPlayEject to be able to load the correct DVD, so the movie would be played afterwards:

Image

- Without DVD Drive: Since it is impossible to eject the DVD tray and load a DVD, the DialogPlayEject would not be applicable. Instead, a DialogOK would appear with the title and message set in the .disc file. For the moment, I have reused a "Media Info" string, but I am opened to put whatever we think is more coherent: info, stub info, stub details...

Image

What do you think?

When we all agree, I can submit the pull request:

Code:
if (item.IsDiscStub())
  {
#ifdef HAS_DVD_DRIVE
    // Display the Play Eject dialog if there is any optical disc drive
    if (g_mediaManager.HasOpticalDrive())
    {
      if (CGUIDialogPlayEject::ShowAndGetInput(item))
        // PlayDiscAskResume takes path to disc. No parameter means default DVD drive.
        // Can't do better as CGUIDialogPlayEject calls CMediaManager::IsDiscInDrive, which assumes default DVD drive anyway
        return MEDIA_DETECT::CAutorun::PlayDiscAskResume() ? PLAYBACK_OK : PLAYBACK_FAIL;
    }
    else
#endif
    {    
      // Since there is no DVD Drive, display a CGUIDialogOK instead
      // Figure out Lines 0 and 1 of the dialog
      std::string strLine1, strLine2;
      CXBMCTinyXML discStubXML;
      if (discStubXML.LoadFile(item.GetPath()))
      {
        TiXmlElement * pRootElement = discStubXML.RootElement();
        if (!pRootElement || strcmpi(pRootElement->Value(), "discstub") != 0)
          CLog::Log(LOGERROR, "Error loading %s, no <discstub> node", item.GetPath().c_str());
        else
        {
          XMLUtils::GetString(pRootElement, "title", strLine1);
          XMLUtils::GetString(pRootElement, "message", strLine2);
        }
      }

      // Use the label for Line 1 if not defined
      if (strLine1.empty())
        strLine1 = item.GetLabel();
  
      CGUIDialogOK::ShowAndGetInput(CVariant{544}, CVariant{std::move(strLine1)}, CVariant{std::move(strLine2)}, false);
    }

    return PLAYBACK_OK;
  }
Reply
#36
Nobody is interested in this anymore?
Reply
#37
from a quick glance it makes sense. Please update your pull request by doing a "force push" to the github branch you created the PR with and it will autoupdate
Reply
#38
Ok.

Do I update Woerd88 pull request with this new code or do I create a new one?

What about the "Media Info" string in the window dialog of the case without DVD? The thing is that if I reuse the "Media Info", no other change is needed, but if the string is changed, ours too. If I create a new one, any suggestion? And translations will be needed.

Thanks.
Reply
#39
sorry, hadn't realized the first PR was by Woerd88 and not by you. Well, you can't really take over another persons PR. So best is to create your own PR and mention in the description that it supersedes the other one (in which case we'll close the obsolete one).

As for the string - I'm not yet too sure about what exactly to show in case there is no drive. It might be good to at least give a little help string before showing the stub info for better WAF, especially when there is not extra info within the stub file.
Reply
#40
Hi.

In case of no DVD drive, I would still prefer the cleaner version I proposed, but what would you say about a dialog with the former header "No optical disc drive detected", fist line with "Stub info:" and second and third, the title and comment fields, if there are set in the stub file?

What do you think?
Reply
#41
as stub files can be used for anything (discs, VHS, whatever) I'd probably not mention "no optical disc drive detected" but something more like ""%s" is not available for instant playback. Please check your offline library for:\n\n%s" where the first %s would be the movie name and the second %s the stub info.
Reply
#42
Thanks for the suggestion.

I guess that that message is for the text displayed inside the dialog box. But, what would be an appropriate header?
Reply
#43
Sorry for my abscence. Go ahead with your new pr, looks like you have a better environment to test this
Reply
#44
Thanks! I'm opened to any suggestion. As I asked @da-anda, what should we put for the header of the dialog box?

Later, when the pull-request is done, I don't know how to make developers pick it and include it in the official branch...
Reply
#45
I'll take care that devs will review the PR. If it's accepted, it's just a matter of pressing 1 button on Github to merge it into mainline Wink
Reply

Logout Mark Read Team Forum Stats Members Help
Offline Media Stub Files (Feature Request)2