GNOME Bugzilla – Bug 123957
[PATCH] multidisksrc should emit its new-file signal even if it failed to open it
Last modified: 2005-11-22 21:27:47 UTC
multidisksrc emits a new-file signal when it starts playing a new file. The main purpose of this signal seems to be to let the user of multidisksrc be able to free the file name and the memory used by the list if he wants to. Consequently, this signal should be emitted even if multidisksrc fails to open the current file. Here's a patch which does that
Created attachment 20510 [details] [review] reworked new-file signal emission
Shouldn't the plugin free such memory itself? Plugins are responsible for their own memory management...
Actually it's doing something very weird atm: the "locations" property is a user-provided pointer to a GSList containing gchar*, and multidisksrc doesn't copy this gslist, it only stores a pointer to it (my guess is that it was done to allow the user of multidisksrc to add files to the locations list without having to stop the element).
That's ugly. Feel free to implement something different.
After discussing on IRC with Ronald, I made the "locations" property read-only, and added "add-locations" and "remove-locations" write-only properties which get a GSList* of filenames, and then copy it in an internal GSList* I'll attach the corresponding patch tomorrow morning
Sounds good. However, it makes more sense from an API standpoint to have "add-location" that takes a gchar *.
That's what was suggested at first, but I think a gobject "notify" signal would be emitted each time you add a song to the src, which would be suboptimal if the user of the src wants to queue 100 or 1000 songs imo.
I don't think that 100 or 1000 songs are a target application for multidisksrc. Or rather, it _shouldn't_ be. An element that can handle 100-1000 files easily should have a much more powerful API. (btw, we'd eventually like to make multidisksrc a subclass of filesrc. If you don't want to do that yourself, that's cool, but it would be a good thing to keep in mind while working on it.)
You can use both properties. So "locations" is aread-only and returns the internal list. "add-location" and "remove-location" adds and removes a single file (gchar *), and "add-locations" and "remove-locations" removes a GSList * of files. Oh, and this filesrc-as-parent-of-multidisksrc is a really cool idea, I favour that, too.
I'll attach what I have done so far in case there are some "do that, don't do that comments". I'll add the "add-location" and "remove-location" properties. I don't have the slightest idea how I could make multidisksrc be a subclass of filesrc (and don't have much time to learn how to do it), so I'll let that to someone else :) What I can do though is rename multidisksrc to multifilesrc as it is suggested in another bug.
Created attachment 20530 [details] [review] patch adding add-locations and remove-locations properties
For the record, there is more brokeness in the current code dealing with the new_file signal: the .h files say the signal handler will receive a gchar * corresponding to the new file name, while the code passes a GSList* when it emits the signal.
Subclassing of filesrc allows to use filesrc's file_open() and file_close() routines, which do all read()/mmap() bladiebla. They can also do the signalling, which would mean filesrc/multifilesrc use exactly the same signals. This should make multifilesrc's code a lot simpler, too. And for the record, the other multidisksrc bug is #122200. Thanks for the work, evne though you didn't subclass it. ;).
Created attachment 20532 [details] [review] same patch as previously with more properties added (compiles but not tested ;)
ok, so what is multidisksrc ? look like the grandpa of multifilesrc if yes, is this patch applicable/needed to multifilesrc or should this be closed ?
doesn't exist anymore... DIE DIE DIE !