GNOME Bugzilla – Bug 709667
Rotate OLEDs when the tablet is in left-handed mode
Last modified: 2013-11-20 17:14:24 UTC
Otherwise the labels are upside-down.
Created attachment 260245 [details] [review] wacom: rotate OLED labels on left-handed setups If the tablet is set to left-handed mode, invert the labels so they look natural with the tablet physically rotated. CW and CCW modes are also taken into account, so text is always displayed from top to bottom, even though rotated 90 degrees.
Created attachment 260246 [details] [review] wacom: separate LED/OLED setup to a separate function There could be situations where calling this without updating button mappings is desired.
Created attachment 260247 [details] [review] wacom: Update pad (O)LEDS when rotation gsetting changes This is so labels are updated accordingly as soon as the setting is changed.
Created attachment 260248 [details] [review] wacom: separate LED/OLED setup to a separate function There could be situations where calling this without updating button mappings is desired.
It is worth pointing out that I don't have a tablet with OLEDs at hand, although I double checked that the cairo_surface_t was being created and updated as expected
Review of attachment 260245 [details] [review]: Looks good otherwise. ::: plugins/wacom/gsd-wacom-oled.c @@ +159,3 @@ surface = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, OLED_WIDTH, OLED_HEIGHT); cr = cairo_create (surface); + apply_device_rotation (cr, rotation); Given that it's 2 lines of code, I'd rather you merged oled_render_text() into here. @@ +282,3 @@ if (g_str_has_prefix (label, MAGIC_BASE64)) buffer = g_strdup (label + MAGIC_BASE64_LEN); + else { If you add braces on one branch, please add it to the other as well.
Review of attachment 260247 [details] [review]: You have gvc changes in the patch. ::: plugins/wacom/gsd-wacom-manager.c @@ +924,3 @@ + set_rotation (device, g_settings_get_enum (settings, key)); + else + update_pad_leds (device); Where's update_pad_leds()?
Review of attachment 260248 [details] [review]: Looks good.
(In reply to comment #7) > Review of attachment 260247 [details] [review]: > > You have gvc changes in the patch. Oh, I'll make sure that doesn't get in. > > ::: plugins/wacom/gsd-wacom-manager.c > @@ +924,3 @@ > + set_rotation (device, g_settings_get_enum (settings, key)); > + else > + update_pad_leds (device); > > Where's update_pad_leds()? It's in attachment 260248 [details] [review], the order in the bz attachment list got scrambled as I updated and marked the former patch obsolete soon after
(In reply to comment #6) > Review of attachment 260245 [details] [review]: > > ::: plugins/wacom/gsd-wacom-oled.c > @@ +159,3 @@ > surface = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, OLED_WIDTH, > OLED_HEIGHT); > cr = cairo_create (surface); > + apply_device_rotation (cr, rotation); > > Given that it's 2 lines of code, I'd rather you merged oled_render_text() into > here. > I'm not sure what you mean here, should I fold apply_device_rotation() into oled_render_text(), or do you mean I should merge oled_render_text() into oled_encode_image()?
apply_device_rotation() into oled_render_text().
Created attachment 260338 [details] [review] wacom: rotate OLED labels on left-handed setups If the tablet is set to left-handed mode, invert the labels so they look natural with the tablet physically rotated. CW and CCW modes are also taken into account, so text is always displayed from top to bottom, even though rotated 90 degrees.
Created attachment 260339 [details] [review] wacom: Update pad (O)LEDS when rotation gsetting changes This is so labels are updated accordingly as soon as the setting is changed.
Review of attachment 260338 [details] [review]: Looks good otherwise. ::: plugins/wacom/gsd-wacom-oled.c @@ +272,1 @@ if (g_str_has_prefix (label, MAGIC_BASE64)) You're still missing the braces here, as you add some to the other branch of the conditional.
Review of attachment 260339 [details] [review]: Sure.
(In reply to comment #14) > You're still missing the braces here, as you add some to the other branch of > the conditional. Right, I forgot that change... Both patches are now pushed with that change applied.