Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13006 closed Bug Report - General (Invalid)

filenames turned into QUrls should be proper file: URLs

Reported by: brian@… Owned by:
Priority: minor Milestone: unknown
Component: MythTV - General Version: 0.28.1
Severity: medium Keywords:
Cc: Ticket locked: no

Description

QUrl will mangle the left-hand side of a string with a : in it:

For example:

#include <QString>
#include <QUrl>
#include <iostream>

using namespace std;

main() {

        QUrl qurl = QString("This:");
        cout << qurl.toDisplayString().toLocal8Bit().constData() << "\n";
    
}

produces:

this:

Notice it lower-cases the first letter.

We should create proper file:filename URLs for filenames.

Change History (14)

comment:1 Changed 7 years ago by brian@…

[PR #131]https://github.com/MythTV/mythtv/pull/131 created over at GitHub? has been created for this.

comment:2 Changed 7 years ago by Peter Bennett

I would think that the part before the colon represents the protocol (e.g. http: ftp: etc.), and SHOULD be lowercased. I suggest this is not a bug in QT but a feature. Maybe URL is being used incorrectly somewhere, if this is causing a problem.

comment:3 in reply to:  2 Changed 7 years ago by brian@…

Replying to pbennett:

I would think that the part before the colon represents the protocol (e.g. http: ftp: etc.),

Indeed. The "scheme".

and SHOULD be lowercased.

Probably MAY.

I suggest this is not a bug in QT but a feature. Maybe URL is being used incorrectly somewhere, if this is causing a problem.

Indeed. That was why I suggested that if the data type is a URL (it is), it should be a properly formed URL, and making a file:filename URL does seem to work. The patch in PR is applied here and seems to be working so far.

comment:4 Changed 7 years ago by brian@…

Any chance of getting this landed? It really does fix up the immediate bug (a Qurl should be in URL format), regardless of the bigger issue of whether the data should be a Qurl or not.

If and until somebody makes a decision (and a change) on that, we should at least form Qurl data items properly.

comment:5 Changed 7 years ago by Peter Bennett

I am not familiar with the code here. Should the input string be a filename or should it be a URL? Maybe the caller should be formatting the string as file://something. If the caller is actually doing that in some cases, then changing the code to use QUrl::fromLocalFile may cause a problem.

comment:6 in reply to:  5 Changed 7 years ago by brian@…

Replying to pbennett:

I am not familiar with the code here.

Me either.

Should the input string be a filename or should it be a URL?

I don't know. But I think that's a different issue. As it is currently, it's a URL yet it's data is badly formed, not in URL format.

I think if somebody wants to look at the bigger picture and refactor all of that code one day, that is fine. But until then, the data should at least be in a valid format for the type that it is.

Maybe the caller should be formatting the string as file://something.

The caller doesn't know that internally the BE is storing that item in a URL data type. The caller is just given a list of items and chooses one. If the BE wants the caller to choose a URL it should give it a list of URLs and not plain names.

If the caller is actually doing that in some cases, then changing the code to use QUrl::fromLocalFile may cause a problem.

So how about the update I just made to the PR then to only make it a file:// URL if it's not already a URL with a scheme?

comment:7 Changed 7 years ago by Peter Bennett

It looks like you have renamed your recording files and now cannot access them from Kodi. Recording files have names similar to 2781_20170412152900.ts . If you have renamed your recording files and updated the database with the new names, that is not supported. If you want to access your recording files with friendly names, you can use mythlink.pl which creates symbolic links (https://www.mythtv.org/wiki/Mythlink.pl).

See background information here. https://github.com/MythTV/mythtv/commit/946f0397

If this is not what you are doing, please explain the use case where this is affecting you.

comment:8 Changed 7 years ago by brian@…

So, we'll just ignore and leave this very demonstrable bug in place simply because it's not affecting the official Myth FE (yet)? We'll just leave it as a time bomb waiting to break something in the future instead of taking an opportunity to fix it now, yes?

I've gone to the effort of finding the bug and providing a patch that fixes it. And I've even spent the time to revise the patch to address the only issue you found when you reviewed it. Why wouldn't we apply it now?

Why did you point out something you wanted fixed in the patch only to now say you don't want to apply it after I have spent the time addressing the issue you wanted fixed? If you had no intention of applying the patch, why did you waste my time like that?

On the mailing list people harp on and on about how this project accepts patches. I've supplied a patch and now you are not accepting it. Why not?

The symlink solution does not work because when a (i.e. cppmyth) FE accessing the symlinks deletes a recording it only deletes the symlink, not the actual recording.

comment:9 Changed 7 years ago by Karl Egly

I don't understand. What is broken that you are fixing by renaming recording files?

The Kodi PVR addon should properly display the metadata of the recording with the original file name.

By manually fiddling with the recording file names you are leaving the tested and supported scenarios, thus finding little issues all over the place. My short term suggestion is to just avoid the colon and some other special characters that are known to have issues on some operating systems. Or move the cleaned up recordings over to the video library and thus stay on the suggested and supported path that is known working.

comment:10 in reply to:  9 Changed 7 years ago by brian@…

Replying to dekarl:

I don't understand. What is broken that you are fixing by renaming recording files?

Does it really matter?

The bottom line here is that there is an identified bug and patch has been supplied to fix it.

And seemingly a strange reluctance to fix a clearly identified bug.

By manually fiddling with the recording file names you are leaving the tested and supported scenarios, thus finding little issues all over the place.

Except that I don't find any, other than this one. Other than this issue, renaming the files and updating the DB accordingly works fine.

My short term suggestion is to just avoid the colon and some other special characters that are known to have issues on some operating systems. Or move the cleaned up recordings over to the video library and thus stay on the suggested and supported path that is known working.

Sure. I could do either of those or any number of other things.

None of that will change the fact that there is a bug lurking, waiting to surface for somebody else some day.

I'm honestly quite puzzled why there is so much resistance to fix a bug when all of the hard work of finding the bug and create a fix for it has been done. I'm puzzled why I am preached at that the MythTV project "accepts patches" and when I take the time to write one up and submit it, there is no desire to land it.

comment:11 Changed 7 years ago by Karl Egly

Resolution: Invalid
Status: newclosed

Sorry, but with that attitude there is no point in keeping that ticket open and trying to help you.

comment:12 Changed 7 years ago by paulh

I've not looked at your patch or any of the affected code but I think the danger is fixing this problem for one Kodi user could break things for ALL MythTV users so it's only right there is some caution here.

comment:13 Changed 7 years ago by Roger Siddons

Because it's the wrong fix. It alters the interface for file transfers that may cause problems for others in the future.

Myth has too many ugly tweaks already. This is one area that is well-defined, so we should preserve that.

Your issue isn't the colon; it's that the file isn't being specified as a local file. Assuming it is a local file may work for you. Many of us keep private patches for our own. But we only commit patches that benefit everyone and don't break things.

Consider:

#include <QUrl>
#include <QDebug>
int main() {
        qDebug() << QUrl("This:").path();        // ""
        qDebug() << QUrl("/This:").path();       // "/This:"
        qDebug() << QUrl("This").path();         // "This"
}

Although the error message uses toString() the relevant code extracts the path().

The internal interface is defined at https://www.mythtv.org/wiki/ANN_(Myth_Protocol)

Note that the prefix "/" denotes it's a local file and Myth handles it correctly.

I suggest you reopen the issue with Kodi PVR.

The filename returned by the services API ("This:") won't have a prefix because the services don't require it. However Kodi PVR is then using the internal Myth protocol for playback, which does. They should be adding a prefix "/".

You are the first to notice their bug because you are probably the first Kodi PVR user to use colons in filenames.

Version 0, edited 7 years ago by Roger Siddons (next)

comment:14 in reply to:  13 Changed 7 years ago by Peter Bennett

Replying to rsiddons: I fixed the URL in your post.

Note: See TracTickets for help on using tickets.