GNOME Bugzilla – Bug 710380
Fn Keys Wont Step up and down brightness
Last modified: 2013-10-24 20:24:34 UTC
On my system: Samsung RV511 laptop The FN Keys (FN+UP and FN+DOWN) for brightness will show the OSD but not change the brightness. The slider in the power panel will change the power.
Created attachment 257510 [details] [review] Retry settings brightness This keeps trying to increase or decrease the brightness until it actually succeeds
Review of attachment 257510 [details] [review]: That's absolutely out of the question. XRandR should queue, and cache the values to be used. The driver for your graphics card needs fixing.
Created attachment 257557 [details] [review] mutter patch
Created attachment 257558 [details] [review] gnome-desktop patch
Created attachment 257559 [details] [review] gnome-settings-daemon patch
Uhm, the patches look good, but I'd expose the minimum backlight step in percentage rather than the number of steps (which is in some unspecified HW-dependent unit, while everything else is %)
Created attachment 257564 [details] [review] mutter patch Expose minimum step as a percentage. We dont need to add 1 ... 0-4 is 5 different levels but they would be 0,25,50,75,100
Created attachment 257565 [details] [review] gnome-desktop patch
Created attachment 257566 [details] [review] gnome-settings-daemon patch Hopefully I wont have to recompile mutter, gsd and gnome-desktop again for a while.
Review of attachment 257564 [details] [review]: Looks good.
Review of attachment 257565 [details] [review]: Yes
Review of attachment 257566 [details] [review]: Just a style nit. ::: plugins/power/gpm-common.c @@ +1276,3 @@ if (now < 0) return percentage_value; + step = MAX (gnome_rr_output_get_min_backlight_step(output), BRIGHTNESS_STEP_AMOUNT (max - min + 1)); Space before (
Created attachment 257570 [details] [review] Gnome-Settings_Daemon patch Style fixes
I have the same issue with the RV520. Thanks for fixing this Asad!
Review of attachment 257570 [details] [review]: ::: plugins/power/gpm-common.c @@ +1276,3 @@ if (now < 0) return percentage_value; + step = MAX (gnome_rr_output_get_min_backlight_step(output), BRIGHTNESS_STEP_AMOUNT (max - min + 1)); Space before the bracket. @@ +1335,3 @@ if (now < 0) return percentage_value; + step = MAX (gnome_rr_output_get_min_backlight_step(output), BRIGHTNESS_STEP_AMOUNT (max - min + 1)); Ditto.
Created attachment 257732 [details] [review] mutter patch Add some error handling ... so it doesn't divide by zero ... oops
Created attachment 257733 [details] [review] gnome-settings-daemon patch Some more style fixes ... How wrong can I make 2 lines of code
Review of attachment 257733 [details] [review]: Looks good :)
Review of attachment 257732 [details] [review]: ::: src/core/monitor.c @@ +808,3 @@ g_variant_new_int32 (output->backlight)); + g_variant_builder_add (&properties, "{sv}", "min-backlight-step", + g_variant_new_int32 ( (output->backlight_max - output->backlight_min) ? it should read: g_variant_new_int32 ((output->backlight_max - output->backlight_min) ? there's no need for the second space, it's a space after function names, before the bracket.
Created attachment 257747 [details] [review] mutter patch
And all committed, many thanks for the patches and iterating over them.
FWIW, I've had this problem on my laptop for a few weeks, just managed to find this bug report - many thanks for the fixes, folks. hadess, can this fix go to 3.10 ? I'm in the middle of doing scratch builds to check that the patches apply and the fix works, but it'd be a pain to keep doing my own side builds each time any of those three is bumped...thanks!
Confirmed the fix, with all three patched components rebuilt, on my 2010 Vaio Z (VPCZ1). This is a moderately popular laptop (it has its own mailing list!), so backporting the fix would be appreciated by a few folks, I think.