[patch] complete get and set lcd patch

[Richard Hughes]

On Wed, 2005-08-31 at 19:03 -0400, David Zeuthen wrote:
> On Wed, 2005-08-31 at 19:17 +0100, Richard Hughes wrote:
> > Here's the latest complete patch as requested. Please review.
> > 
> 
> Looks pretty good. Thinking about it we should call it LaptopPanel
> instead of LCDPanel.. since that is exactly what it is. LCDPanel might
> mean an external monitor connected via VGA or DVI.

Changed.

> Some other comments:
> 
> +static gboolean
> +lcdpanel_refresh (HalDevice *d, ACPIDevHandler *handler)
> +{
> 
> We need to test that the 'brightness' file.. or whatever it's called on
> various systems actually exist.. And bail out (refuse to add the hal
> device object) if it's missing. 
> 
> +	snprintf (path, sizeof (path), "%s/acpi/ibm", get_hal_proc_path ());
> +	if (g_file_test (path, G_FILE_TEST_EXISTS))
> +		acpi_synthesize_item (path, ACPI_TYPE_IBM_DISPLAY);
> 
> Maybe this should be "%s/acpi/ibm/brightness" instead? That would give
> the check we need. Right?

Yes, but then you need to scan the directory. I've done a proper fix
that you can check.

> +++ fdi/policy/10osvendor/16-lcdpanel-mgmt-policy.fdi	2005-08-31 18:31:15.000000000 +0100
> 
> Why 16? Why not just 10? :-)

Sure, have changed.

> +++ tools/hal-system-lcd-get-brightness	2005-08-31 19:00:35.000000000 +0100
> +++ tools/hal-system-lcd-set-brightness	2005-08-31 18:36:11.000000000 +0100
> 
> Both of these need to check that the brightness file is actually there
> and throw an exception otherwise (the .Unsupported exception is fine)

Sure, and I've checked writable too, as some ppl might not be running
HAL as root. Shell script is great.

> So, if you can change this and send the patch again I think we're
> getting there!

New patch attached. If we can commit soon, I'll promise to patch any
remaining issues against CVS :-)

Richard.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hal-laptop_panel.diff
Type: text/x-patch
Size: 18937 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/hal/attachments/20050901/b7912db8/hal-laptop_panel-0001.bin