GNOME Bugzilla – Bug 700499
Handle non-text OLED labels
Last modified: 2013-07-17 08:25:45 UTC
The wacom code should check whether the content of oled-label starts with "base64:" and not touch the value if already encoded into a supported image format for the OLED.
Created attachment 244566 [details] [review] Special handling for base64: strings
String used for testing: base64:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADw8PD/8AAP/VAAAA1f8AAEDlTR9N5VAAAP8AADDq/wAAtDweHlzkQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAPDw8PD/AAD/TNUA1Uz/AADsFQAAABXsAAD/AEDbBv8AAAAAAAAAFesAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/wAA/wA7+jsA/wAAzlEAAABBzgAA/2DNBQD/AAD/Dw8AAFHOAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA8PDw8P8AAP8AAAAAAP8AAARdw/DDXQUAAP++AwAA/wAAT8Lw8MVOBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAigAAAAAAAAAAAAAAAAAA8AAAAAAAAAAAAAAAAP8AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA2iQAAAAAAAAA8AAAAAAAAP8AAFDQ4NBQAGDAEAD/AADw8LAQAAAgwODAIADw0HDwAAAAAAAAAAAAAAAAAAAAAP8AAPDw8AAAAAAAAAAAAAD/AAAKMpHUjgAABFzR/wAAAAAY6gAA6hgBF+sAAAEZ/wAAAAAAAAAAAAAAAAAAAACtQgAAAAAAAAAAAAAAAAAADQAA6jwHIpAAENJdBf8AAAAAga4AAK6BAIC+AAAAAP8AAAAAAAAAAAAAAAAAAAAABKgAAAAAAAAADwAAAAAAAA8AAAUNDw4GAQ0GAAAPAAAPDwsBAAACDA8MAgAAAAAPAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==
Review of attachment 244566 [details] [review]: Hi Przemo, I don't have a way to test this (don't have a tablet with OLED) so I'll just share my thoughts on the code. Thank you for your patch! ::: plugins/wacom/gsd-wacom-oled.c @@ -189,2 +189,4 @@ } +static gboolean +gsd_label_is_base64_encoded (char *label) Use gchar and Glib types when possible. @@ -191,0 +191,7 @@ +static gboolean +gsd_label_is_base64_encoded (char *label) +{ ... 4 more ... Judging from what what g_strstr_len does and the size limitation you're applying, why just not return pos != NULL ? @@ -210,3 +227,4 @@ button_id_short = (int)button_id_1[6]; button_id_short = button_id_short - 'A' - 1; - buffer = oled_encode_image (label); + + if (gsd_label_is_base64_encoded (label)){ Nitpicking here: space between ) and { ::: plugins/wacom/gsd-wacom-oled.h @@ -31,2 +31,3 @@ #define MAX_IMAGE_SIZE 1024 /*Size of buffer for storing OLED image*/ #define MAX_1ST_LINE_LEN 10 /*Maximum number of characters in 1st line of OLED icon*/ +#define MAGIC_BASE64 "base64:" /*Label starting with base64: is treated as already encoded*/ I would include these defines in the .c file, unless we need to access this define from outside the source file? (in which case, it might need to add the namespace as a prefix to all those defines) @@ -31,2 +31,4 @@ #define MAX_IMAGE_SIZE 1024 /*Size of buffer for storing OLED image*/ #define MAX_1ST_LINE_LEN 10 /*Maximum number of characters in 1st line of OLED icon*/ +#define MAGIC_BASE64 "base64:" /*Label starting with base64: is treated as already encoded*/ +#define MAGIC_BASE64_LEN 7 /*Length of MAGIC_BASE64*/ I'd prefer not to have the size hardcoded but I understand it is quicker than checking the length all the time and simpler than caching it inside the private so I'm okay with it.
(In reply to comment #3) > ::: plugins/wacom/gsd-wacom-oled.c > @@ -189,2 +189,4 @@ > } > > +static gboolean > +gsd_label_is_base64_encoded (char *label) > > Use gchar and Glib types when possible. Don't use gchar actually. It's a useless typedef. Ditto gint. gint64, guint64, *those* are useful. > @@ -31,2 +31,4 @@ > #define MAX_IMAGE_SIZE 1024 /*Size of buffer for storing > OLED image*/ > #define MAX_1ST_LINE_LEN 10 /*Maximum number of characters in > 1st line of OLED icon*/ > +#define MAGIC_BASE64 "base64:" /*Label starting with base64: is > treated as already encoded*/ > +#define MAGIC_BASE64_LEN 7 /*Length of MAGIC_BASE64*/ > > I'd prefer not to have the size hardcoded but I understand it is quicker than > checking the length all the time and simpler than caching it inside the private > so I'm okay with it. #define MAGIC_BASE64_LEN strlen(MAGIC_BASE64) will get optimised at compile-time (as MAGIC_BASE64 is a constant).
(In reply to comment #3) > Review of attachment 244566 [details] [review]: > > Hi Przemo, > > I don't have a way to test this (don't have a tablet with OLED) so I'll just > share my thoughts on the code. > > Thank you for your patch! > > ::: plugins/wacom/gsd-wacom-oled.c > @@ -189,2 +189,4 @@ > } > > +static gboolean > +gsd_label_is_base64_encoded (char *label) > > Use gchar and Glib types when possible. OK > @@ -191,0 +191,7 @@ > +static gboolean > +gsd_label_is_base64_encoded (char *label) > +{ > ... 4 more ... OK > Judging from what what g_strstr_len does and the size limitation you're > applying, why just not return pos != NULL ? OK, I can use MAGIC_BASE64_LEN as size limitation and return pos != NULL > @@ -210,3 +227,4 @@ > button_id_short = (int)button_id_1[6]; > button_id_short = button_id_short - 'A' - 1; > - buffer = oled_encode_image (label); > + > + if (gsd_label_is_base64_encoded (label)){ > > Nitpicking here: space between ) and { OK :-) Where I can find coding style for gcc/gsd? > ::: plugins/wacom/gsd-wacom-oled.h > @@ -31,2 +31,3 @@ > #define MAX_IMAGE_SIZE 1024 /*Size of buffer for storing > OLED image*/ > #define MAX_1ST_LINE_LEN 10 /*Maximum number of characters in > 1st line of OLED icon*/ > +#define MAGIC_BASE64 "base64:" /*Label starting with base64: is > treated as already encoded*/ > > I would include these defines in the .c file, unless we need to access this > define from outside the source file? (in which case, it might need to add the > namespace as a prefix to all those defines) I'm not really sure where (or if) Bastien wants to use those, but I'll move them to .c file > @@ -31,2 +31,4 @@ > #define MAX_IMAGE_SIZE 1024 /*Size of buffer for storing > OLED image*/ > #define MAX_1ST_LINE_LEN 10 /*Maximum number of characters in > 1st line of OLED icon*/ > +#define MAGIC_BASE64 "base64:" /*Label starting with base64: is > treated as already encoded*/ > +#define MAGIC_BASE64_LEN 7 /*Length of MAGIC_BASE64*/ > > I'd prefer not to have the size hardcoded but I understand it is quicker than > checking the length all the time and simpler than caching it inside the private > so I'm okay with it. Size of image is hardcoded in kernel https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/input/tablet/wacom_sys.c?id=c7788792a5e7b0d5d7f96d0766b4cb6112d47d75#n916 size of 1st line - my pick, 11 would work as well, but I'm not sure about non-latin fonts size of magic string - as you said, my thinking was similiar.
(In reply to comment #4) > (In reply to comment #3) > > ::: plugins/wacom/gsd-wacom-oled.c > > @@ -189,2 +189,4 @@ > > } > > > > +static gboolean > > +gsd_label_is_base64_encoded (char *label) > > > > Use gchar and Glib types when possible. > > Don't use gchar actually. It's a useless typedef. Ditto gint. gint64, guint64, > *those* are useful. OK :-) who is making the last call? :-) > > @@ -31,2 +31,4 @@ > > #define MAX_IMAGE_SIZE 1024 /*Size of buffer for storing > > OLED image*/ > > #define MAX_1ST_LINE_LEN 10 /*Maximum number of characters in > > 1st line of OLED icon*/ > > +#define MAGIC_BASE64 "base64:" /*Label starting with base64: is > > treated as already encoded*/ > > +#define MAGIC_BASE64_LEN 7 /*Length of MAGIC_BASE64*/ > > > > I'd prefer not to have the size hardcoded but I understand it is quicker than > > checking the length all the time and simpler than caching it inside the private > > so I'm okay with it. > > #define MAGIC_BASE64_LEN strlen(MAGIC_BASE64) > will get optimised at compile-time (as MAGIC_BASE64 is a constant). make sense... thanks!
Created attachment 244925 [details] [review] Special handling for base64: strings
Created attachment 244928 [details] [review] GdkPixbuf to base64 string function GdkPixbuf to base64 string function, performs size check, but doesn't check color depth assuming 8-bit. Returns base64 string starting with magic "base64" string. Returned value can be used directly as button label and should render to OLED icon.
Review of attachment 244925 [details] [review]: ::: plugins/wacom/gsd-wacom-oled.c @@ +193,3 @@ +static char* +gsd_label_is_base64_encoded (char *label) I would expect a boolean to come out of this function, and I'm not even sure it's necessary. @@ +223,3 @@ button_id_short = button_id_short - 'A' - 1; + + if (gsd_label_is_base64_encoded (label)) { if (g_str_has_prefix (label, MAGIC_BASE64)) buffer = g_strdup (label + MAGIC_BASE64_LEN); else buffer = oled_encode_image (label);
Review of attachment 244928 [details] [review]: ::: plugins/wacom/gsd-wacom-oled.c @@ +187,3 @@ + gchar *base64; + + if (OLED_WIDTH != gdk_pixbuf_get_width (pixbuf)) { No need for braces for one-line blocks. @@ +212,3 @@ + + oled_scramble_icon (image); + base64 = g_strconcat(MAGIC_BASE64, g_base64_encode (image, MAX_IMAGE_SIZE), NULL); You're leaking the output of g_base64_encode here, you'll need to do that in 2 parts. ::: plugins/wacom/gsd-wacom-oled.h @@ +34,2 @@ void set_oled (GsdWacomDevice *device, char *button_id, char *label); +char* gsd_wacom_oled_gdkpixbuf_to_base64 (GdkPixbuf *pixbuf); char *gsd_wacom_... Space after the type name, and star next to the function name.
Created attachment 246560 [details] [review] GdkPixbuf to base64 string function
(In reply to comment #10) > Review of attachment 244928 [details] [review]: > > ::: plugins/wacom/gsd-wacom-oled.c > @@ +187,3 @@ > + gchar *base64; > + > + if (OLED_WIDTH != gdk_pixbuf_get_width (pixbuf)) { > > No need for braces for one-line blocks. OK > @@ +212,3 @@ > + > + oled_scramble_icon (image); > + base64 = g_strconcat(MAGIC_BASE64, g_base64_encode (image, > MAX_IMAGE_SIZE), NULL); > > You're leaking the output of g_base64_encode here, you'll need to do that in 2 > parts. Please check if the new version is OK > ::: plugins/wacom/gsd-wacom-oled.h > @@ +34,2 @@ > void set_oled (GsdWacomDevice *device, char *button_id, char *label); > +char* gsd_wacom_oled_gdkpixbuf_to_base64 (GdkPixbuf *pixbuf); > > char *gsd_wacom_... > Space after the type name, and star next to the function name. OK Thanks Bastien!
Review of attachment 246560 [details] [review]: Looks good
Created attachment 249324 [details] [review] Special handling for base64: strings v2
(In reply to comment #9) > Review of attachment 244925 [details] [review]: > > ::: plugins/wacom/gsd-wacom-oled.c > @@ +193,3 @@ > > +static char* > +gsd_label_is_base64_encoded (char *label) > > I would expect a boolean to come out of this function, and I'm not even sure > it's necessary. OK > @@ +223,3 @@ > button_id_short = button_id_short - 'A' - 1; > + > + if (gsd_label_is_base64_encoded (label)) { > > if (g_str_has_prefix (label, MAGIC_BASE64)) > buffer = g_strdup (label + MAGIC_BASE64_LEN); > else > buffer = oled_encode_image (label); I'm learning a lot here - thanks again!
Review of attachment 249324 [details] [review]: Looks good.
In the future, please make sure that you rebase your patches on master, your patch didn't apply anymore. Also don't forget the "wacom: " prefix to the commit subject line.