After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 331508 - Initial radio import doesn't work with recent totem-plparser
Initial radio import doesn't work with recent totem-plparser
Status: RESOLVED FIXED
Product: totem-pl-parser
Classification: Core
Component: General
Old
Other Linux
: Normal normal
: ---
Assigned To: General Totem maintainer(s)
General Totem maintainer(s)
: 333137 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-02-17 05:47 UTC by James "Doc" Livingston
Modified: 2008-06-03 17:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for totem (4.56 KB, patch)
2006-02-21 06:59 UTC, James "Doc" Livingston
needs-work Details | Review
rhythmbox changes (1.33 KB, patch)
2006-02-21 07:33 UTC, James "Doc" Livingston
committed Details | Review
fixed patch for totem (4.89 KB, patch)
2006-02-27 01:21 UTC, James "Doc" Livingston
accepted-commit_now Details | Review
patch with test-parser option (5.83 KB, patch)
2006-03-02 14:20 UTC, James "Doc" Livingston
committed Details | Review
additional totem patch (1.04 KB, patch)
2006-03-03 00:39 UTC, James "Doc" Livingston
committed Details | Review

Description James "Doc" Livingston 2006-02-17 05:47:22 UTC
Totem-plparser recently started recursively processing URIs, so that it works with more complex playlist files.

RB uses totem-plparser to load it's inital iradio station list, and this change break that usage. We either need a way to tell totem-plparser to not to do recursive processing, or load the initial iradio station in a different way.
Comment 1 Bastien Nocera 2006-02-17 12:48:37 UTC
We need a way to change the "recurse level" before pushing the URI, and make the .pls and other code use it before spitting out the URL.
Should I reassign to Totem?
Comment 2 James "Doc" Livingston 2006-02-17 13:54:29 UTC
Whether we want to reassign to totem, or leave this here is up to you.

Adding a "recurse" property (default true) to the parser object, or something would probably work okay.
Comment 3 James "Doc" Livingston 2006-02-21 06:59:21 UTC
Created attachment 59821 [details] [review]
patch for totem

This patch makes the necessary changes to totem-plparser so that Rhythmbox's initial station loading works again. I don't have any URLs of streams that require fancy recursive processing, but it should work.

As well as adding a "recurse" property (default on), I've fixed an issue with PLS parsing where it would reset the recurse-level to 0 and fallback to false.
Comment 4 James "Doc" Livingston 2006-02-21 07:33:32 UTC
Created attachment 59822 [details] [review]
rhythmbox changes

The necessary changes to RB
Comment 5 Bastien Nocera 2006-02-21 07:53:17 UTC
Thanks James for that, I'll review the code soon, and go through the release-team for that one.
Bug #318749 contains one of those streams that need further parsing.
Comment 6 Bastien Nocera 2006-02-26 12:45:06 UTC
Comment on attachment 59821 [details] [review]
patch for totem

>diff -u -u -r1.91 totem-pl-parser.c
>--- src/plparse/totem-pl-parser.c	6 Feb 2006 20:42:35 -0000	1.91
>+++ src/plparse/totem-pl-parser.c	21 Feb 2006 06:45:46 -0000
>@@ -54,6 +54,15 @@
<snip>
>+							       "Whether or not to process URLs furthur", 

Should be "further".

<snip>
>-			if (totem_pl_parser_parse (parser, file, FALSE) != TOTEM_PL_PARSER_RESULT_SUCCESS) {
>+			if (totem_pl_parser_parse_internal (parser, file) != TOTEM_PL_PARSER_RESULT_SUCCESS) {
> 				totem_pl_parser_add_one_url_ext (parser, file, title, genre);

The "fallback" is different from the recurse. It means that if the parsing is ignored, or fails, the url itself is added to the playlist, otherwise, it would be ignored.

<snip>
>-
>+		

Please mind the gratuitous spacing changes.
Comment 7 James "Doc" Livingston 2006-02-27 01:21:06 UTC
Created attachment 60187 [details] [review]
fixed patch for totem

I've fixed the issues you mentioned.

I'm still not clear on why turning fallback off for "child" PLS files makes any sense. If it is needed for something, is there any reason why the same isn't done for other formats, like asx?
Comment 8 Bastien Nocera 2006-03-02 13:23:43 UTC
asx, and the likes need fallback as they could be playlists or data files. If we can't parse the playlist, we assume it's a media file and add it to the playlist.

As for the patch, please also add a "-n/--no-recurse" option to test-parser so we can test without the recurse.
Comment 9 James "Doc" Livingston 2006-03-02 14:20:30 UTC
Created attachment 60492 [details] [review]
patch with test-parser option

I've added --no-recurse and -n as options to the test parser to set recurive processing to false.
Comment 10 James "Doc" Livingston 2006-03-02 22:41:56 UTC
*** Bug 333137 has been marked as a duplicate of this bug. ***
Comment 11 James "Doc" Livingston 2006-03-02 23:43:17 UTC
Patch committed to cvs. I'll fix up the RB one now.
Comment 12 James "Doc" Livingston 2006-03-03 00:39:16 UTC
Created attachment 60525 [details] [review]
additional totem patch

I've notices a few minor issue that can potentially cause problems, patch attached.

We shouldn't reset fallback if we are not recursing, because (depending on the playlist) it may not always emit the entries if it's FALSE.

@@ -983,7 +983,8 @@
 		}
 
 		fallback = parser->priv->fallback;
-		parser->priv->fallback = FALSE;
+		if (parser->priv->recurse)
+			parser->priv->fallback = FALSE;
 
 		if (strstr (file, "://") != NULL || file[0] == G_DIR_SEPARATOR) {
 			if (totem_pl_parser_parse_internal (parser, file) != TOTEM_PL_PARSER_RESULT_SUCCESS) {


Currently, each entry still has it's mime-type looked up, even though we don't use it. If the mime-type cannot be determines by the file name, it's data is look at, which won't work if it's on the 'net and the user has no 'net connection. This makes it not get the mime-type for things other than the initial playlist when we are not recursing.

@@ -1738,6 +1739,10 @@
 			|| g_str_has_prefix (url, "rtsp") != FALSE) {
 		totem_pl_parser_add_one_url (parser, url, NULL);
 		return TOTEM_PL_PARSER_RESULT_SUCCESS;
+	}
+
+	if (!parser->priv->recurse && parser->priv->recurse_level > 0) {
+		return TOTEM_PL_PARSER_RESULT_UNHANDLED;
 	}
 
 	mimetype = gnome_vfs_mime_type_from_name (url);
Comment 13 Bastien Nocera 2006-03-05 14:39:58 UTC
Looks good. Thanks for working on that.
Comment 14 James "Doc" Livingston 2006-03-10 11:28:11 UTC
Committed to cvs.
Comment 15 Philip Withnall 2008-06-03 17:39:26 UTC
Mass-move from totem to totem-pl-parser. You can remove all messages by searching for this comment.