GNOME Bugzilla – Bug 671072
Light up the OLEDs
Last modified: 2013-07-15 16:51:13 UTC
On Intuos4 and the likes. See: http://braindump.kargulus.de/?p=432 http://www.davidrevoy.com/index.php?article70/
Maintainer change
For something newer: https://github.com/PrzemoF/i4oled There's various things I don't like with this code though: - it uses a single program, which needs to be run as root to access the device sysfs entries, and it does text and image processing. - this device run as root looks like it could be used to load arbitrary files (say I passed /root/myfile.png) accessible by root. - the PNG could could probably be read using gdk-pixbuf on the gnome-settings-daemon side. - the GPL v3 is not compatible with the current gnome-settings-daemon LICENSE (GPLv2 or later) - There's a bunch of hard-coded values and length (1024 seems particularly popular) I would: - Split the code that writes to the device into a separate helper (see gsd-backlight-helper in plugins/power for an example) that can only be passed raw image data (I'd encode the image data as Base64, glib has code to encode/decode this) - Do all the image processing on the user session, inside the g-s-d plugin
I have a patchset for gsd/gcc implementing OLED handling ready - I just need to test them on a normal system (as opposed to jhbuild environment). They might need a little bit of cleaning as well (mostly variable naming & some cast warnings). My comments to above: - run as root required because of sysfs permissions - won't be a problem when included in gsd/gcc. - feel free to apply GPL v2 to i4oled code if you wish - the 1024 hardcoded value is related to size of the OLED screen (64x32x4bit = 1024 bytes) and should be really replaced by a #define OLED_BUFFER_SIZE 1024 I'll post my patches later today - they might be a good starting point. Demo of the patchset: http://firszt.eu/wacom-icons/wacom_oleds_in_action.webm My long term goal: - editable labels, - icons instead of labels is user decides so. Examples: http://firszt.eu/wacom - full bluetooth support (1-bit images as opposed to 4-bit over USB), I'm not familiar with bugzilla policy - I hope it's OK to include links to external blog/gallery (there are no ads)
Created attachment 240974 [details] [review] gsd patch - to be applied together with relevant gnome-control-center patch This patch is not ready for use, but it should work. It needs cleaning - spacing, variable names, not used variables, comments, not used code.
Created attachment 240975 [details] [review] gnome-control-center patch - to be applied together with relevant gsd patch This patch is not ready for use, but it should work. It needs cleaning - spacing, variable names, not used variables, comments, not used code.
Review of attachment 240974 [details] [review]: Additional comments made in the bug report still apply. Most of the code here should be in gnome-settings-daemon, running as a normal user. ::: data/org.gnome.settings-daemon.peripherals.wacom.gschema.xml.in.in @@ +52,3 @@ <_description>EDID information of monitor to map tablet to. Must be in the format [vendor, product, serial]. ["","",""] disables mapping.</_description> </key> + <key name="oled-label-update" type="s"> Why have 2 settings for the oled-label? ::: plugins/wacom/gsd-wacom-device.c @@ +260,3 @@ int idx, + int status_led, + int has_oled) Use a gboolean. @@ +1288,3 @@ id = g_strdup_printf ("%s%c", button_str_id, i); + if (libwacom_get_button_flag(wacom_device, i) & WACOM_BUTTON_OLED) + has_oled = GSD_WACOM_HAS_OLED; TRUE. ::: plugins/wacom/gsd-wacom-device.h @@ +115,2 @@ #define GSD_WACOM_NO_LED -1 +#define GSD_WACOM_HAS_OLED 1 That's not needed. ::: plugins/wacom/gsd-wacom-oled-helper.c @@ +41,3 @@ + +#define LABEL_SIZE 30 /*Maximum length of text for OLED icn*/ +#define IMAGE_SIZE 1024 /*Size of buffer for storing OLED image*/ That shouldn't be a constant. @@ +87,3 @@ +static void oled_split_text(gchar *label, char* line1, char* line2) +{ + wchar_t wlabel[LABEL_SIZE+1] = L""; GLib has UTF-8 support, there's no need to use those barebones libc features.
Review of attachment 240975 [details] [review]: ::: panels/wacom/cc-wacom-page.c @@ +612,3 @@ g_settings_set_enum (button->settings, ACTION_TYPE_KEY, GSD_WACOM_ACTION_TYPE_NONE); g_settings_set_string (button->settings, CUSTOM_ACTION_KEY, ""); + g_settings_set_string (button->settings, KEY_OLED_LABEL, "\"\""); That's most bizarre, why do you set the string to contain 2 quotes? ::: panels/wacom/gsd-wacom-device.h @@ +115,2 @@ #define GSD_WACOM_NO_LED -1 +#define GSD_WACOM_NO_OLED -1 This file should be a copy/paste of the gnome-settings-daemon one, committed separately (see the makefile for the target to update and commit it)
Review of attachment 240974 [details] [review]: Thanks for the review! ::: data/org.gnome.settings-daemon.peripherals.wacom.gschema.xml.in.in @@ +52,3 @@ <_description>EDID information of monitor to map tablet to. Must be in the format [vendor, product, serial]. ["","",""] disables mapping.</_description> </key> + <key name="oled-label-update" type="s"> A couple factors here: 1. The update is rather slow 2. I didn't know how to trigger gsd event for individual buttons 3. Triggering event for whole device was easy, with all required code already in place. So, oled-label-update triggers update on one button, the other setting stores labels for all buttons. ::: plugins/wacom/gsd-wacom-device.c @@ +260,3 @@ int idx, + int status_led, + int has_oled) OK - agreed. ::: plugins/wacom/gsd-wacom-device.h @@ +115,2 @@ #define GSD_WACOM_NO_LED -1 +#define GSD_WACOM_HAS_OLED 1 OK ::: plugins/wacom/gsd-wacom-oled-helper.c @@ +41,3 @@ + +#define LABEL_SIZE 30 /*Maximum length of text for OLED icn*/ +#define IMAGE_SIZE 1024 /*Size of buffer for storing OLED image*/ I have mixed feelings about it. Kernel module won't accept anything that is not 1024 bytes, so I don't see a reason of using a variable. Unless you mean something different?
Review of attachment 240975 [details] [review]: Thnaks again for the review! ::: panels/wacom/cc-wacom-page.c @@ +612,3 @@ g_settings_set_enum (button->settings, ACTION_TYPE_KEY, GSD_WACOM_ACTION_TYPE_NONE); g_settings_set_string (button->settings, CUSTOM_ACTION_KEY, ""); + g_settings_set_string (button->settings, KEY_OLED_LABEL, "\"\""); oled-helper has to get some kind of string to set label to empty image. Empty string (like "") doesn't work, because it gets hammered by pkexec call. I'm open to correction/a better solution on that. ::: panels/wacom/gsd-wacom-device.h @@ +115,2 @@ #define GSD_WACOM_NO_LED -1 +#define GSD_WACOM_NO_OLED -1 OK
(In reply to comment #6) > Review of attachment 240974 [details] [review]: > > Additional comments made in the bug report still apply. Most of the code here > should be in gnome-settings-daemon, running as a normal user. > > ::: data/org.gnome.settings-daemon.peripherals.wacom.gschema.xml.in.in > @@ +52,3 @@ > <_description>EDID information of monitor to map tablet to. Must be in > the format [vendor, product, serial]. ["","",""] disables > mapping.</_description> > </key> > + <key name="oled-label-update" type="s"> > > Why have 2 settings for the oled-label? A couple factors here: 1. The update is rather slow, so I didn't want to update all 8 oleds and.. 2. I didn't know how to trigger gsd event for individual buttons 3. Triggering event for whole device was easy, with all required code already in place. So, oled-label-update triggers update on one button, the other setting stores labels for all buttons. > > ::: plugins/wacom/gsd-wacom-device.c > @@ +260,3 @@ > int idx, > + int status_led, > + int has_oled) > > Use a gboolean. > > @@ +1288,3 @@ > id = g_strdup_printf ("%s%c", button_str_id, i); > + if (libwacom_get_button_flag(wacom_device, i) & WACOM_BUTTON_OLED) > + has_oled = GSD_WACOM_HAS_OLED; > > TRUE. OK > > ::: plugins/wacom/gsd-wacom-device.h > @@ +115,2 @@ > #define GSD_WACOM_NO_LED -1 > +#define GSD_WACOM_HAS_OLED 1 > > That's not needed. > > ::: plugins/wacom/gsd-wacom-oled-helper.c > @@ +41,3 @@ > + > +#define LABEL_SIZE 30 /*Maximum length of text for OLED icn*/ > +#define IMAGE_SIZE 1024 /*Size of buffer for storing OLED image*/ > > That shouldn't be a constant. I have mixed feelings about it. Kernel module won't accept anything that is not 1024 bytes, so I don't see a reason of using a variable. Unless you mean something different? > > @@ +87,3 @@ > +static void oled_split_text(gchar *label, char* line1, char* line2) > +{ > + wchar_t wlabel[LABEL_SIZE+1] = L""; > > GLib has UTF-8 support, there's no need to use those barebones libc features. The only reason why it looks like that is because it is copy&paste from i4oled code. P.S. I'm learning bugzilla, so I tried to answer to your comments in the code.. ended up messy :-)
(In reply to comment #7) > Review of attachment 240975 [details] [review]: > > ::: panels/wacom/cc-wacom-page.c > @@ +612,3 @@ > g_settings_set_enum (button->settings, ACTION_TYPE_KEY, > GSD_WACOM_ACTION_TYPE_NONE); > g_settings_set_string (button->settings, CUSTOM_ACTION_KEY, ""); > + g_settings_set_string (button->settings, KEY_OLED_LABEL, "\"\""); > > That's most bizarre, why do you set the string to contain 2 quotes? oled-helper has to get some kind of string to set label to empty image. Empty string (like "") doesn't work, because it gets hammered by pkexec call. I'm open to correction/a better solution on that. > > ::: panels/wacom/gsd-wacom-device.h > @@ +115,2 @@ > #define GSD_WACOM_NO_LED -1 > +#define GSD_WACOM_NO_OLED -1 > > This file should be a copy/paste of the gnome-settings-daemon one, committed > separately (see the makefile for the target to update and commit it) OK
(In reply to comment #10) <snip> > A couple factors here: > 1. The update is rather slow, so I didn't want to update all 8 oleds and.. What's slow? Writing out to the device file? The pkexec launching code should probably not be sync then :) > 2. I didn't know how to trigger gsd event for individual buttons In set_wacom_settings(), where reset_pad_buttons() is called, you'll iterate through the buttons for the device, check that the button has an OLED, and listen to the "changed:: oled-label" signal on the button's GSettings. > 3. Triggering event for whole device was easy, with all required code already > in place. > So, oled-label-update triggers update on one button, the other setting stores > labels for all buttons. That should be fixed by the above. > > ::: plugins/wacom/gsd-wacom-oled-helper.c > > @@ +41,3 @@ > > + > > +#define LABEL_SIZE 30 /*Maximum length of text for OLED icn*/ > > +#define IMAGE_SIZE 1024 /*Size of buffer for storing OLED image*/ > > > > That shouldn't be a constant. > > I have mixed feelings about it. Kernel module won't accept anything that is not > 1024 bytes, so I don't see a reason of using a variable. Unless you mean > something different? MAX_IMAGE_SIZE then. You can also remove all the checks for NULL returns from g_malloc(), it can never fail (or your application aborts). Will you be able to iterate the code soon? I'd like to get this in GNOME ASAP so it can get tested before 3.10.
(In reply to comment #12) > (In reply to comment #10) > <snip> > > A couple factors here: > > 1. The update is rather slow, so I didn't want to update all 8 oleds and.. > > What's slow? Writing out to the device file? The pkexec launching code should > probably not be sync then :) Time between key press and oled change is long enough to notice. It will be even slower over bluetooth. Anyway that's not important if I can go button-by-button. > > 2. I didn't know how to trigger gsd event for individual buttons > > In set_wacom_settings(), where reset_pad_buttons() is called, you'll iterate > through the buttons for the device, check that the button has an OLED, and > listen to the "changed:: oled-label" signal on the button's GSettings. OK, I'll try again. That's my first attempt to code something in gnome, so please be patient ;-) > > 3. Triggering event for whole device was easy, with all required code already > > in place. > > So, oled-label-update triggers update on one button, the other setting stores > > labels for all buttons. > > That should be fixed by the above. OK > > > ::: plugins/wacom/gsd-wacom-oled-helper.c > > > @@ +41,3 @@ > > > + > > > +#define LABEL_SIZE 30 /*Maximum length of text for OLED icn*/ > > > +#define IMAGE_SIZE 1024 /*Size of buffer for storing OLED image*/ > > > > > > That shouldn't be a constant. > > > > I have mixed feelings about it. Kernel module won't accept anything that is not > > 1024 bytes, so I don't see a reason of using a variable. Unless you mean > > something different? > > MAX_IMAGE_SIZE then. OK > You can also remove all the checks for NULL returns from g_malloc(), it can > never fail (or your application aborts). OK, kernel habits.. > Will you be able to iterate the code soon? I'd like to get this in GNOME ASAP > so it can get tested before 3.10. I'll try, but I might not be able to apply all you comments - i.e. switching to Glib for string handling doesn't sound easy for me. If you know that you can do it easily and you have enough spare time feel free to do it. I'll be working on it anyway, but I can't promise any deadline as I know nothing about gnome internals and I'm learning as I code. (I'm still struggling with ostree, so testing is not as easy as it should be)
Created attachment 242049 [details] [review] gcc patch - version 2
Created attachment 242050 [details] [review] gsd patch - version 2
Version 2. Migration to Glib string handling not yet included. Patches were tested only in jhbuild environment (root check in the helper + pkexec call were removed for testing).
Created attachment 242092 [details] [review] gsd patch - version 3 - glib migration completed
Created attachment 242094 [details] [review] Patch for testing only!! This patch switches off root checking and pkexec calls - it's for testing purposes only!
Testing procedure that I used: 1. Connect tablet 2. Change oled files to writeable i.e.: sudo chmod a+w /sys/bus/usb/drivers/wacom/3-1\:1.0//wacom_led/* 3. Apply gsd/gcc patches. 4. Apply patch id=242094 5. Build gsd/gcc using jhbuild and run using jhbuild gnome session
Created attachment 242166 [details] [review] gcc patch - version 3
Review of attachment 242092 [details] [review]: Nearly there! ::: configure.ac @@ +255,3 @@ else if test x$enable_gudev != xno; then + PKG_CHECK_MODULES(WACOM, [libwacom >= $LIBWACOM_REQUIRED_VERSION x11 xi xtst gudev-1.0 gnome-desktop-3.0 >= $GNOME_DESKTOP_REQUIRED_VERSION xorg-wacom librsvg-2.0 >= $LIBRSVG_REQUIRED_VERSION pango >= $PANGO_REQUIRED_VERSION]) You need to separate the checks (and libraries) that will be used by the helper and the daemon itself. The helper will run as root, so there's no need to link it against libwacom, x11, or gnome-desktop for example. ::: plugins/wacom/gsd-wacom-manager.c @@ +601,3 @@ } +static void oled_scramble_icon(guchar* image) 2 lines, and a space before the bracket. Could we move all the icon mangling to a separate file though? gsd-wacom-manager.c is already long enough :) @@ +612,3 @@ + for (y = 0; y < 16; y++) { + for (x = 0; x < 32; x++) { + l1 = (0x0F & (buf[31 - x + 64 * y])); No magic numbers, please move those to a define (16, 32, 64). @@ +623,3 @@ +} + +static void oled_surface_to_image(guchar* image, cairo_surface_t* surface) I'd like to have a GdkPixbuf to base64 function as well. We could use that to have specific keybindings have proper icons instead of text in the future. @@ +693,3 @@ + oled_split_text(label ,line1, line2); + + strcpy(buf, line1); strncpy(). Why not use buf = g_strdup_printf ("%s\n%s", line1, line2)? @@ +973,3 @@ grab_button (id, TRUE, manager->priv->screens); + + GList *buttons, *l; Declarations at the top of the function. ::: plugins/wacom/gsd-wacom-oled-helper.c @@ +51,3 @@ + } + + image = g_base64_decode(buffer, &length); Space here. And please check the retval. @@ +111,3 @@ + } + + g_type_init (); That's not needed since GLib 2.36.
Comment on attachment 242094 [details] [review] Patch for testing only!! Marking as reviewed, though I'd probably done a one-liner: s/pkexec/sudo/ for testing (rather shorter ;)
Review of attachment 242166 [details] [review]: We have a script to update gsd-wacom-device.h from gnome-settings-daemon to gnome-control-center, so no need to add it to the patch (and the changes to gsd-wacom-device.c are missing anyway ;) ::: panels/wacom/cc-wacom-page.c @@ +563,3 @@ g_settings_set_string (button->settings, CUSTOM_ACTION_KEY, str); + str = gtk_accelerator_get_label (keyval, mask); + g_settings_set_string (button->settings, OLED_LABEL, str); I don't think you want <Primary><Alt>F as the string when you could show: Ctrl+Alt+F This should use the user-visible string instead.
Created attachment 244246 [details] [review] gsd patch - version 4
(In reply to comment #21) > Review of attachment 242092 [details] [review]: > > Nearly there! Yes! :-) > ::: configure.ac > @@ +255,3 @@ > else > if test x$enable_gudev != xno; then > + PKG_CHECK_MODULES(WACOM, [libwacom >= $LIBWACOM_REQUIRED_VERSION x11 > xi xtst gudev-1.0 gnome-desktop-3.0 >= $GNOME_DESKTOP_REQUIRED_VERSION > xorg-wacom librsvg-2.0 >= $LIBRSVG_REQUIRED_VERSION pango >= > $PANGO_REQUIRED_VERSION]) > > You need to separate the checks (and libraries) that will be used by the helper > and the daemon itself. > > The helper will run as root, so there's no need to link it against libwacom, > x11, or gnome-desktop for example. Done! > ::: plugins/wacom/gsd-wacom-manager.c > @@ +601,3 @@ > } > > +static void oled_scramble_icon(guchar* image) > > 2 lines, and a space before the bracket. Done! > Could we move all the icon mangling to a separate file though? > gsd-wacom-manager.c is already long enough :) > > @@ +612,3 @@ > + for (y = 0; y < 16; y++) { > + for (x = 0; x < 32; x++) { > + l1 = (0x0F & (buf[31 - x + 64 * y])); > > No magic numbers, please move those to a define (16, 32, 64). Done! > @@ +623,3 @@ > +} > + > +static void oled_surface_to_image(guchar* image, cairo_surface_t* surface) > > I'd like to have a GdkPixbuf to base64 function as well. We could use that to > have specific keybindings have proper icons instead of text in the future. Do we need that function for now? i know that you're thinking ahead, but I just don't know if that should be part of that patchset. Storing icons as base64 strings sounds interesting (if that's what you mean). I'm building up library of icons, but that's for a separate bug report. > @@ +693,3 @@ > + oled_split_text(label ,line1, line2); > + > + strcpy(buf, line1); > > strncpy(). > > Why not use buf = g_strdup_printf ("%s\n%s", line1, line2)? Done! There is still room for improvement in string handling. (i.e. line1/2 definitions) > @@ +973,3 @@ > grab_button (id, TRUE, manager->priv->screens); > + > + GList *buttons, *l; > > Declarations at the top of the function. Done! > ::: plugins/wacom/gsd-wacom-oled-helper.c > @@ +51,3 @@ > + } > + > + image = g_base64_decode(buffer, &length); > > Space here. > And please check the retval. Done! > @@ +111,3 @@ > + } > + > + g_type_init (); > > That's not needed since GLib 2.36. Done! (reason: I have glib = 2.34.2) (In reply to comment #22) > (From update of attachment 242094 [details] [review]) > Marking as reviewed, though I'd probably done a one-liner: > s/pkexec/sudo/ > for testing (rather shorter ;) Thanks! I couldn't make it work, but that will be my homework.. (In reply to comment #23) > Review of attachment 242166 [details] [review]: > > We have a script to update gsd-wacom-device.h from gnome-settings-daemon to > gnome-control-center, so no need to add it to the patch (and the changes to > gsd-wacom-device.c are missing anyway ;) OK > ::: panels/wacom/cc-wacom-page.c > @@ +563,3 @@ > g_settings_set_string (button->settings, CUSTOM_ACTION_KEY, str); > + str = gtk_accelerator_get_label (keyval, mask); > + g_settings_set_string (button->settings, OLED_LABEL, str); > > I don't think you want > <Primary><Alt>F > as the string when you could show: > Ctrl+Alt+F > > This should use the user-visible string instead. Please double check as current version works fine (gtk_accelerator_get_label vs. gtk_accelerator_name).
Review of attachment 244246 [details] [review]: ::: configure.ac @@ +256,3 @@ if test x$enable_gudev != xno; then PKG_CHECK_MODULES(WACOM, [libwacom >= $LIBWACOM_REQUIRED_VERSION x11 xi xtst gudev-1.0 gnome-desktop-3.0 >= $GNOME_DESKTOP_REQUIRED_VERSION xorg-wacom librsvg-2.0 >= $LIBRSVG_REQUIRED_VERSION]) + PKG_CHECK_MODULES(WACOM_OLED, [libwacom >= $LIBWACOM_REQUIRED_VERSION pango >= $PANGO_REQUIRED_VERSION]) It doesn't use pango or libwacom, but uses gudev. ::: plugins/wacom/Makefile.am @@ +68,3 @@ +gsd_wacom_oled_helper_LDFLAGS = \ + $(BACKLIGHT_HELPER_LIBS) \ It doesn't need backlight_helper_libs @@ +76,3 @@ + +gsd_wacom_oled_helper_CFLAGS = \ + $(BACKLIGHT_HELPER_CFLAGS) \ Nor the cflags. ::: plugins/wacom/gsd-wacom-device.h @@ +120,3 @@ char *id; GSettings *settings; + GsdWacomDevice *device; You don't use this at all in gsd-wacom-device.c, it seems to only be a helper for gsd-wacom-manager.c You can use something like: g_object_set_data (button, "parent-device", device); and device = g_object_get_data (button); in gsd-wacom-manager.c with requiring a new struct member. ::: plugins/wacom/gsd-wacom-manager.c @@ +706,3 @@ + label = g_settings_get_string (button->settings, OLED_LABEL); + if (label) { + set_oled(device, button->id, label); space before bracket. @@ +722,3 @@ + + label = g_settings_get_string (settings, OLED_LABEL); + set_oled(button->device, button->id, label); Space before bracket. ::: plugins/wacom/gsd-wacom-oled.c @@ +84,3 @@ + int i; + + if (g_utf8_strlen(label, LABEL_SIZE) <= MAX_1ST_LINE_LEN) { Space. @@ +85,3 @@ + + if (g_utf8_strlen(label, LABEL_SIZE) <= MAX_1ST_LINE_LEN) { + g_utf8_strncpy(line1, label, MAX_1ST_LINE_LEN); Space. @@ +89,3 @@ + } + + token = g_strsplit_set(label, delimiters, -1); Etc. ::: plugins/wacom/gsd-wacom-oled.h @@ +25,3 @@ + +/* OLED parameters */ +#define OLED_WIDTH 64 Can you align the values together so it's more readable?
> > ::: panels/wacom/cc-wacom-page.c > > @@ +563,3 @@ > > g_settings_set_string (button->settings, CUSTOM_ACTION_KEY, str); > > + str = gtk_accelerator_get_label (keyval, mask); > > + g_settings_set_string (button->settings, OLED_LABEL, str); > > > > I don't think you want > > <Primary><Alt>F > > as the string when you could show: > > Ctrl+Alt+F > > > > This should use the user-visible string instead. > > Please double check as current version works fine (gtk_accelerator_get_label > vs. gtk_accelerator_name). Yes, you're right. I missed the assignment, which means you're leaking str before that assignment ;)
(In reply to comment #26) > Review of attachment 244246 [details] [review]: > > ::: configure.ac > @@ +256,3 @@ > if test x$enable_gudev != xno; then > PKG_CHECK_MODULES(WACOM, [libwacom >= $LIBWACOM_REQUIRED_VERSION x11 > xi xtst gudev-1.0 gnome-desktop-3.0 >= $GNOME_DESKTOP_REQUIRED_VERSION > xorg-wacom librsvg-2.0 >= $LIBRSVG_REQUIRED_VERSION]) > + PKG_CHECK_MODULES(WACOM_OLED, [libwacom >= $LIBWACOM_REQUIRED_VERSION > pango >= $PANGO_REQUIRED_VERSION]) > > It doesn't use pango or libwacom, but uses gudev. I did something strange here.. Next version should be better :-) > ::: plugins/wacom/Makefile.am > @@ +68,3 @@ > > +gsd_wacom_oled_helper_LDFLAGS = \ > + $(BACKLIGHT_HELPER_LIBS) \ > > It doesn't need backlight_helper_libs OK > @@ +76,3 @@ > + > +gsd_wacom_oled_helper_CFLAGS = \ > + $(BACKLIGHT_HELPER_CFLAGS) \ > > Nor the cflags. OK > ::: plugins/wacom/gsd-wacom-device.h > @@ +120,3 @@ > char *id; > GSettings *settings; > + GsdWacomDevice *device; > > You don't use this at all in gsd-wacom-device.c, it seems to only be a helper > for gsd-wacom-manager.c > You can use something like: > g_object_set_data (button, "parent-device", device); > and > device = g_object_get_data (button); > in gsd-wacom-manager.c with requiring a new struct member. Thanks for the lesson - I didn't know that! > ::: plugins/wacom/gsd-wacom-manager.c > @@ +706,3 @@ > + label = g_settings_get_string (button->settings, OLED_LABEL); > + if (label) { > + set_oled(device, button->id, label); > > space before bracket. OK > @@ +722,3 @@ > + > + label = g_settings_get_string (settings, OLED_LABEL); > + set_oled(button->device, button->id, label); > > Space before bracket. OK > ::: plugins/wacom/gsd-wacom-oled.c > @@ +84,3 @@ > + int i; > + > + if (g_utf8_strlen(label, LABEL_SIZE) <= MAX_1ST_LINE_LEN) { > > Space. OK > @@ +85,3 @@ > + > + if (g_utf8_strlen(label, LABEL_SIZE) <= MAX_1ST_LINE_LEN) { > + g_utf8_strncpy(line1, label, MAX_1ST_LINE_LEN); > > Space. I fixed those & more - it there a script like scripts/checkpatch.pl in kernel to check for that type of "errors"? > @@ +89,3 @@ > + } > + > + token = g_strsplit_set(label, delimiters, -1); > > Etc. OK > ::: plugins/wacom/gsd-wacom-oled.h > @@ +25,3 @@ > + > +/* OLED parameters */ > +#define OLED_WIDTH 64 > > Can you align the values together so it's more readable? Done! Thank you for being patient :-) Please double check the Makefile/configure part...
Created attachment 244473 [details] [review] gsd patch - version 5
Created attachment 244486 [details] [review] wacom: Add OLED handling for Intuos4
Created attachment 244487 [details] [review] wacom: Add basic OLED handling for Intuos4 This sets the OLED to the keyboard shortcut when a custom keyboard shortcut is used.
Attachment 244486 [details] pushed as a6ed61b - wacom: Add OLED handling for Intuos4 After a few style fixes, and re-adding the "pkexec" call for the helper
Attachment 244487 [details] pushed as 16c0a66 - wacom: Add basic OLED handling for Intuos4 After fixing the small memleak :)
Thanks Bastien!
Created attachment 246561 [details] [review] Quiet compiler warning about non initialized image That shouldn't make any difference, because image was freed anyway, but with this patch compiler doesn't complain about non initialized variable. I'm not opening a new bug, because the issue was introduced by patch 244486
Review of attachment 246561 [details] [review]: The commit subject is missing the "wacom: " prefix. Also note that you should never add patches to closed bugs. Closed bugs are ignored in all sorts of bugzilla reports and can fall through the cracks. git-bz file gnome-settings-daemon/wacom master will upload the last patch in a new bug.