Re: [Utopia] Fix gnome-volume-manager PTP camera detection

[Jeffrey Stedfast <fejj novell com>]

On Fri, 2005-07-29 at 21:22 +0200, Martin Pitt wrote:
> Hi Jeffrey!
> 
> Jeffrey Stedfast [2005-07-29 11:21 -0400]:
> > On Fri, 2005-07-29 at 17:08 +0200, Martin Pitt wrote:
> > > Hi!
> > > 
> > > gnome-volume-manager 1.3.2 broke PTP camera detection due to a logic
> > > flaw in gvm_udi_is_camera(). Even if a device had the "camera"
> > > capability, the subsequent check for libgphoto2 support made the
> > > function return FALSE.
> > 
> > I believe the logic is correct, but it sounds like it's failing for you
> > because your HAL isn't patched to ad the libgphoto2_support property.
> 
> Maybe, but it should also work with the upstream version. Will the
> libgphoto2_support go upstream as well?

I have no idea... I was told that this flag was the solution and so
implemented it (I've never been able to test this).

I just committed a second fix to the camera detection stuff to make it
check camera.access_method == "ptp" rather than checking
camera.libgphoto2_support since that's what my ptp camera reports
(altho, again, I'm unable to really test these changes because f-spot
immediately aborts for me... mono gcc4 issues?).

of course, the same camera in USB Mass-Storage mode reports
"libgphoto2", so perhaps I'll need to change that again... not sure.

e.g. is camera.access_method only reported as "ptp" if it's unsupported
by libgphoto2 (or at least no fdi file for the camera)? or is the access
method only "libgphoto2" for USB Mass-Storage?

If the latter, I might have to change some logic again... because we get
the "device added" event from HAL for the physical camera device before
the volume is added, but we don't actually want to do anything until we
get the volume added event for USB MAss-Storage camera devices (so that
we can both mount the camera volume and also pass the mount point to
f-spot or whatever)

>  I noticed that hal does not
> use gphoto's hotplug map any more, so I guess right now it only
> supports PTP cameras.
> 
> > So... I think the correct solution to this would be to make sure the
> > property exists before using the value returned by get_property_bool() -
> > this way, for people without the patch, it continues to work. 
> 
> Sounds good, thank you! :-)
> 
> > I might split the function in 2 such that the logic is clearer:
> > 
> > gvm_udi_is_camera()
> > 
> > gvm_udi_is_ptp_camera()
> 
> I think the function is simple enough already, but as you wish.

it turns out the case for USB Mass-Storage Cameras was wrong and
required a bit more work to detect properly. I needed to check the
physical device for the "camera" property, rather than the volume udi
(duh, should have been obvious).

I didn't catch this originally because the post-mount code was detecting
a DCIM directory and so things magically worked that way...

> 
> > > --- gnome-volume-manager-1.3.2-old/src/manager.c        2005-07-29 17:02:13.000000000 +0200
> > > +++ gnome-volume-manager-1.3.2/src/manager.c    2005-07-29 17:02:13.000000000 +0200
> > > @@ -614,15 +614,13 @@
> > >  static gboolean
> > >  gvm_udi_is_camera (const char *udi, gboolean check_libgphoto2)
> > >  {
> > > -       if (!libhal_device_query_capability (hal_ctx, udi, "camera", NULL))
> > > -               return FALSE;
> > > -
> > > -       if (check_libgphoto2 && !libhal_device_get_property_bool (hal_ctx, udi, "camera.libgphoto2_support", NULL))
> > > -               return FALSE;
> > > -
> > > -       dbg ("Camera detected: %s\n", udi);
> > > +       if (libhal_device_query_capability (hal_ctx, udi, "camera", NULL) ||
> > > +            (check_libgphoto2 && libhal_device_get_property_bool (hal_ctx, udi, "camera.libgphoto2_support", NULL))) {
> > > +                dbg ("Camera detected: %s\n", udi);
> > > +                return TRUE;
> > > +        }
> > 
> > the new logic is such that it will still return TRUE even if
> > check_libgphoto2 is TRUE and libgphoto2_support is FALSE (assuming the
> > camera property is on the device). This should not happen.
> > 
> > Might as well not even bother checking libgphoto2 :)
> 
> I don't understand---with my patch, hal will return FALSE if
> camera.libgphoto2_support exists and is false. So what do you mean?

if (TRUE || (TRUE && FALSE))

shortcut that logic :)

Jeff

-- 
Jeffrey Stedfast
Evolution Hacker - Novell, Inc.
fejj ximian com  - www.novell.com