Created attachment 211308 [details] [review]
Add optical media source

Created attachment 211397 [details] [review]
Add optical media source

Will show:
- Mounted VCDs and DVDs
- mounted ISO images of VCDs and DVDs

Works here in grilo-test-ui with a number of physical DVDs and DVD images.

Created attachment 211416 [details] [review]
Add optical media source

Will show:
- Mounted VCDs and DVDs
- mounted ISO images of VCDs and DVDs

Comment on attachment 211397 [details] [review]
Add optical media source

Removed the debug

Created attachment 211743 [details] [review]
Add optical media source

Will show:
- Mounted VCDs and DVDs
- mounted ISO images of VCDs and DVDs

Comment on attachment 211416 [details] [review]
Add optical media source

New version has cancellation support, and cleans up after itself (lingering GVolumeMonitor signals, as it's a singleton).

Created attachment 211861 [details] [review]
Refactor of Bastien's patch

This is a refactor over Bastien patch:

* Use GRL_WARNING instead of g_warning

* Removed Gtk dependency and variables, as they are not used in the code

* Fixed indentation

* Show each device as a separated source

The last change deserves an explanation. In Grilo, each plugin can create several sources. The idea is to map each source to a physical device source from where we get the content.

For instance, UPnP plugin creates a source for each server it finds. This is done dynamically, so when a new server comes up a new source is created, and when an UPnP server dissapears, the corresponding source is removed too.

The refactor brings this idea to optical-media: each disk is show as a separated source; inserting a new disk creates a new source, and removing a disk removes the source.

Also, to speed up things, when a new disk is detected, a GrlMedia for that disk is created and cached in the source. So when user browses the source, it gets that media.

(In reply to comment #8)
<snip>
> * Show each device as a separated source
> 
> The last change deserves an explanation. In Grilo, each plugin can create
> several sources. The idea is to map each source to a physical device source
> from where we get the content.
> 
> For instance, UPnP plugin creates a source for each server it finds. This is
> done dynamically, so when a new server comes up a new source is created, and
> when an UPnP server dissapears, the corresponding source is removed too.
> 
> The refactor brings this idea to optical-media: each disk is show as a
> separated source; inserting a new disk creates a new source, and removing a
> disk removes the source.
> 
> Also, to speed up things, when a new disk is detected, a GrlMedia for that disk
> is created and cached in the source. So when user browses the source, it gets
> that media.

This has the unfortunate effect of putting the DVDs a level of indirection away from the top-level, each in their own little source with a single media object.

From a UI stand-point, I'd want to have optical media in a single location. There's bound to be only one or 2 optical media listed. We'd want to see them in the same location and hide the whole source if there are no optical media. Right now, I'd need to keep track of every source, and each item inside each source. This doesn't make writing the UI any easier, and I can't quite see the benefits.

The rest of the changes are fine by me.

(In reply to comment #9)
> This has the unfortunate effect of putting the DVDs a level of indirection away
> from the top-level, each in their own little source with a single media object.
> 
> From a UI stand-point, I'd want to have optical media in a single location.
> There's bound to be only one or 2 optical media listed. We'd want to see them
> in the same location and hide the whole source if there are no optical media.
> Right now, I'd need to keep track of every source, and each item inside each
> source. This doesn't make writing the UI any easier, and I can't quite see the
> benefits.


Yes, that's true. The main point to propose it, besides following the "one device, one source" approach, is the idea of adding support for CDAudio in future. Even if Totem does not support them, I find it very useful for other applications. 

And in this sense, a CDAudio is not just one media, but a set of CDDA tracks.  So I put two CDAudios or a DVD and a CDAudio, I do not want to mix them in the same place, but have them separated.

But also I understand that maybe having just one source with one media (like in DVD) is too much. A mixed approach could be used instead: group all DVDs in one source (as you are doing in your patch), and for the case of CDAudio putting them in a separated source.

What do you think? Note that CDAudio case is not actually supported in the plugin, but I would like to know the approach to follow in future.

On the other hand, and thinking in your approach, something I don't like personally very much is showing a source with no content. I wonder what's the best approach: showing always the Optical Media source, even if there is no DVD inserted, and thus no content on the source, or rather, showing the source only where there is at least one DVD inserted. Note that by "showing" I mean registering the source when the DVD is inserted, and unregistering it when it is removed.

The way I see it being implemented in applications is (depending on the type of source), each item within the single source is a top-level item, or we show each source as a top-level item.

For the first case, it's easy to imagine an "optical media" section in Videos' results row:
https://live.gnome.org/Design/Apps/Videos#Tentative_Design
each item being a video disc, and the section being hidden when no discs are present.

For the second case, applications would show separate sources (like Rhythmbox' sidebar) and do the grouping themselves.

I would go with:
- grouping all video discs in one top-level source
- putting each audio CD in a separate source (or as a container within the same top-level source as the video discs)

(In reply to comment #12)
> I would go with:
> - grouping all video discs in one top-level source
> - putting each audio CD in a separate source (or as a container within the same
> top-level source as the video discs)

Seems that we have an agreement.

I'm applying some of the changes I did over the last version of your patch, and I'll push it master.

Also, I'll backport it 0.1.x branch, so people using that version will be pleased of having this wonderful plugin.

Pushed in master, backported to 0.1.x branch.


commit 9b3f9cd031e270a9a370ddabc46cb02d8329f144
Author: Bastien Nocera <hadess@hadess.net>
Date:   Thu Apr 12 14:53:17 2012 +0000

    optical-media: Add optical media source
    
    Will show:
     - Mounted VCDs and DVDs
     - Mounted ISO images of VCDs and DVD
    
    https://bugzilla.gnome.org/show_bug.cgi?id=672929
    
    Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

 configure.ac                                  |   38 ++
 src/media/Makefile.am                         |    6 +-
 src/media/optical-media/Makefile.am           |   41 ++
 src/media/optical-media/grl-optical-media.c   |  571 +++++++++++++++++++++++++
 src/media/optical-media/grl-optical-media.h   |   76 ++++
 src/media/optical-media/grl-optical-media.xml |   10 +
 6 files changed, 741 insertions(+), 1 deletions(-)