GNOME Bugzilla – Bug 723368
GTK+ print module for Google Cloud Print
Last modified: 2014-02-28 13:14:25 UTC
I've made a simple print module to support printing to Google Cloud Print. This is a web service provided by Google that allows people to share their printers. https://www.google.com/cloudprint/learn/ In addition to being able to print to printers shared on Google Cloud Print, there is an equivalent of "Print to file" in the form of "Save to Google Drive". Here's a screenshot of the print dialog gedit shows: http://cyberelk.net/tim/wp-content/uploads/2014/01/cloudprint.png The cloudprint module uses gnome-online-accounts to obtain the OAuth 2.0 access token for the Google account, so I needed to add another "scope" in the google goa backend. Currently it can discover available printers, get simple details about them such as display name and status, and submit jobs without any special options. I plan to add options-setting based on the capabilities advertised by the printer. Posted on gtk-devel: https://mail.gnome.org/archives/gtk-devel-list/2014-January/msg00021.html
Created attachment 267734 [details] [review] cloudprint patch
Review of attachment 267734 [details] [review]: I applied the patch, and built gtk (without updating gnome-online-accounts) - it built fine, but when I click on the printing example in gtk3-demo, it hangs in a dbus call to goa. We should probably do this async, and be defensive against goa not having the cloudprint support. ::: modules/printbackends/Makefile.am @@ -4,1 @@ I think this needs to be conditional, like cups: if HAVE_CLOUDPRINT SUBDIRS += cloudprint endif ::: modules/printbackends/cloudprint/gtkprintbackendcloudprint.c @@ +1,1 @@ +/* GTK - The GIMP Toolkit We avoid this expansion of the acronym nowadays - shouldn't put it in new source files. @@ +2,3 @@ + * gtkprintbackendcloudprint.c: Google Cloud Print implementation of + * GtkPrintBackend + * Copyright (C) 2007, 2012, 2014, Red Hat, Inc. And this is probably just 2014 ?
Created attachment 268070 [details] [review] improved cloudprint patch Thanks for the comments. Here is a new version of the patch with no sync calls, no GTK acronym expansion, and a fixed Makefile.am. It also removes some incorrect g_object_weak_unref() calls, and fills in the 'cancellable' member of a call where it had mistakenly been left as NULL.
Oh, I should mention: it also uses goa_account_get_printers_disabled() from bug #723370, so it will need that version of gnome-online-accounts to build against.
Review of attachment 268070 [details] [review]: Hi, thank you very much for the work on this. I tried it and it works well for me. But I would prefer to use DBus calls instead of calls to GOA API so we don't have a dependency on GOA (I'll attach a sample of getting of the access token for you). Bellow are some comments regarding the code. ::: modules/printbackends/cloudprint/gtkprintbackendcloudprint.c @@ +308,3 @@ + gboolean success = FALSE; + + gdk_threads_enter (); This is deprecated. @@ +319,3 @@ + } + + json_parser = json_parser_new (); Could you move this whole part (up to the "if (!success){}") to a function which would return the root in the case of success? It is repeated 3 times. @@ +374,3 @@ + g_object_unref (ps->job); + + if (error) I would prefer to have these pointer checks like "if (error != NULL)" in newly written code. @@ +380,3 @@ + g_free (ps); + + gdk_threads_leave (); This is deprecated too. @@ +492,3 @@ + _PrintStreamData *ps = (_PrintStreamData *) user_data; + + error = NULL; We could make this part of the error's declaration. @@ +857,3 @@ + GtkPrinter *printer; + JsonObject *jprinter = json_array_get_object_element (jprinters, i); + char *name = g_strdup (json_object_get_string_member (jprinter, Would it be possible to use "const char *" without the g_strdup() ? @@ +912,3 @@ + + done: + gtk_print_backend_set_list_done (GTK_PRINT_BACKEND (backend)); I'm just curious, have you tested this with more than 1 google account? @@ +1016,3 @@ + + done: + g_signal_emit_by_name (printer, "details-acquired", FALSE); I think that we could remove both labels and move the g_signal_emit_by_name() to the body of the if(). ::: modules/printbackends/cloudprint/gtkprintbackendcloudprint.h @@ +1,1 @@ +/* GTK - The GIMP Toolkit Here is one more acronym expansion to remove.
Created attachment 268813 [details] the promised sample
(In reply to comment #5) > Review of attachment 268070 [details] [review]: > But I would prefer to use DBus calls instead of calls to GOA API so we don't > have a dependency on GOA (I'll attach a sample of getting of the access token > for you). This might be a good idea because, as I mentioned on the GOA patch review, GOA itself depends on GTK+.
(In reply to comment #6) > Created an attachment (id=268813) [details] > the promised sample I think you can copy the XML file that has the GOA DBus interface definition and use gdbus-codegen to generate most of what libgoa.so offers. That is how libgoa.so itself is built. The only thing that is not autogenerated is GoaClient. However most of that is a wrapper around GDBusObjectManager, and the code to obtain a GDBusObjectManager is also autogenerated. I think that would make things a lot simpler.
Created attachment 269596 [details] [review] improved cloudprint patch I'm working on this. Here is a new patch. * Removed deprecated gdk_threads_enter/leave() calls. * Factored out some json parsing code. * Explicit pointer comparisons. * Initialise error as part of declaration. * No need to g_strdup(name), and more robust JSON handling. * Tidy-ups in cloudprint_printer_request_details(). * Header file fix. * Use GDBusObjectManager to interact with gnome-online-accounts. Still left to do: * Multiple account handling. Currently, only one configured Google account will work. I'll attach a new patch when the multiple account handling is ready.
Created attachment 269797 [details] [review] Change API calls to DBus calls (In reply to comment #9) > Created an attachment (id=269596) [details] [review] > improved cloudprint patch > > I'm working on this. > > Here is a new patch. > * Removed deprecated gdk_threads_enter/leave() calls. > * Factored out some json parsing code. > * Explicit pointer comparisons. > * Initialise error as part of declaration. > * No need to g_strdup(name), and more robust JSON handling. > * Tidy-ups in cloudprint_printer_request_details(). > * Header file fix. > * Use GDBusObjectManager to interact with gnome-online-accounts. > > Still left to do: > * Multiple account handling. Currently, only one configured Google account will > work. > > I'll attach a new patch when the multiple account handling is ready. Hi Tim, thank you for the patch. But I would like to go the way I've proposed before. Using the DBus calls instead of generating of the API. Maintaining a XML file in gtk+ which comes from another package doesn't seem right to me. We use just 2 method from it and 1 method from DBus ObjectManager so the only inconvenience is the getting of accounts info from the returned GVariant. I've prepared a patch which changes it this way (see the attachment). I have just one another note. Could you add comments for translators above the translated strings?
Created attachment 269813 [details] [review] Change API calls to DBus calls (In reply to comment #10) > Created an attachment (id=269797) [details] [review] > Change API calls to DBus calls > > But I would like to go the way I've proposed before. Using the DBus calls > instead of generating of the API. Maintaining a XML file in gtk+ which comes > from another package doesn't seem right to me. We use just 2 method from it and > 1 method from DBus ObjectManager so the only inconvenience is the getting of > accounts info from the returned GVariant. > I've prepared a patch which changes it this way (see the attachment). > > I have just one another note. Could you add comments for translators above the > translated strings? This is an update to the change of APIs. It just changes the way we handle the situation when the GOA methods are not available. I would propose to not show the warning because it would be shown every time to all users which don't have installed GOA.
Created attachment 269927 [details] [review] Handle multiple accounts Thanks. Here's a new patch which should also handle multiple Google accounts.
Created attachment 269932 [details] [review] Handle multiple accounts and send base64-encoded New patch due to a bug I discovered in the previous patch. The job content needs to be submitted in base64 format or else the /submit response will cause an assertion failure in json-glib.
Review of attachment 269932 [details] [review]: Hi Tim, thank you for the modifications. I have just few comments. Otherwise it is good for me. Thank you Marek ::: modules/printbackends/cloudprint/gtkcloudprintaccount.c @@ +51,3 @@ + + /* Context for async calls (only one async call at a time). */ + GTask *task; I don't see any usage of this member. @@ +286,3 @@ + if (output == NULL) + { + g_object_unref (source); You can unref this right after the finish() call. @@ +293,3 @@ + + g_variant_get (output, "(si)", + &access_token, Maybe we could store the token directly into "account->oauth2_access_token". @@ +304,3 @@ + FALSE); + + g_object_unref (source); No need for this if source is unreffed right after the finish() call. @@ +337,3 @@ + g_object_unref (task); + g_object_unref (call); + return; We can achieve the same thing if we remove the "g_object_unref (call)" and the "return". @@ +519,3 @@ + g_object_unref (call); + g_object_unref (task); + return; The "g_object_unref (call)" and the "return" can be removed too. ::: modules/printbackends/cloudprint/gtkprintbackendcloudprint.c @@ +400,3 @@ +{ + gchar buf[_STREAM_MAX_CHUNK_SIZE]; + gchar encoded[(_STREAM_MAX_CHUNK_SIZE / 3 + 1) * 4 + 4]; Could you place here a comment describing the calculation? @@ +727,3 @@ + { + if (!strcmp (status, "ONLINE")) + /* The printer status is online, i.e. it is ready to print. */ Please add "Translators: " to the beginning of the translators comments. @@ +883,3 @@ + else + { + if (error->code != G_IO_ERROR_CANCELLED) Please add "error->domain != G_IO_ERROR ||" to the if(), I forgot to place it there. @@ +931,3 @@ +cloudprint_printer_get_settings_from_options (GtkPrinter *printer, + GtkPrinterOptionSet *options, + GtkPrintSettings *settings) I'm nitpicking now but could you align the parameters :). @@ +939,3 @@ + GtkPrintJob *print_job, + GtkPrintSettings *settings, + GtkPageSetup *page_setup) Here too please. ::: modules/printbackends/cloudprint/gtkprintbackendcloudprint.h @@ +34,3 @@ + gchar *path; + gchar *presentation_identity; +} typedef TGOAAccount; I intended this structure as an internal one in gtkprintbackendcloudprint.c. Could you modify it so that you pass the "id", "path" and "presentation_identity" directly to the gtk_cloudprint_account_new() ? ::: modules/printbackends/cloudprint/gtkprintercloudprint.c @@ +141,3 @@ + G_PARAM_STATIC_NAME | + G_PARAM_STATIC_NICK | + G_PARAM_STATIC_BLURB | You can use G_PARAM_STATIC_STRINGS instead of those three G_PARAM_STATIC_* flags. @@ +153,3 @@ + G_PARAM_STATIC_NAME | + G_PARAM_STATIC_NICK | + G_PARAM_STATIC_BLURB | The same case as above.
Created attachment 270376 [details] [review] patch with latest improvements Thanks for the review. Here is a patch with those improvements incorporated. The base64 calculation was copied from the documentation for g_base64_encode_step(), although I realised that I was adding 4 extra bytes that are only needed when g_base64_encode_close() is used. I've added comments to those parts explaining the calculation.
(In reply to comment #13) > New patch due to a bug I discovered in the previous patch. The job content > needs to be submitted in base64 format or else the /submit response will cause > an assertion failure in json-glib. So something the server sends back to us can cause an assertion failure? That needs to be fixed, even if you change the code here so that a non-malicious server doesn't send that response.
(In reply to comment #16) > So something the server sends back to us can cause an assertion failure? That > needs to be fixed, even if you change the code here so that a non-malicious > server doesn't send that response. That's right. The particular case I encountered is when the server sends a string containing a Unicode high surrogate character followed by any character that is not a low surrogate. The assertion is in json-glib-0.16.2: json-glib/json-scanner.c: 1118 if (g_unichar_type (ucs) == G_UNICODE_SURROGATE) 1119 { 1120 /* read next surrogate */ 1121 if ('\\' == json_scanner_get_char (scanner, line_p, position_p) 1122 && 'u' == json_scanner_get_char (scanner, line_p, position_p)) 1123 { 1124 gunichar ucs_lo = json_scanner_get_unichar (scanner, line_p, position_p); >>1125 g_assert (g_unichar_type (ucs_lo) == G_UNICODE_SURROGATE); 1126 ucs = (((ucs & 0x3ff) << 10) | (ucs_lo & 0x3ff)) + 0x10000; 1127 } 1128 } 1129 1130 g_assert (g_unichar_validate (ucs)); 1131 gstring = g_string_append_unichar (gstring, ucs);
Review of attachment 270376 [details] [review]: Thank you for the changes. I just have two more things before I'll accept it. ::: modules/printbackends/cloudprint/gtkcloudprintaccount.c @@ +383,3 @@ + account)); + + g_dbus_connection_call (G_DBUS_CONNECTION (g_object_ref (source)), You don't need to ref the connection here if you ref it in the gtk_cloudprint_account_search() as mentioned below. @@ +416,3 @@ + account)); + + g_dbus_connection_call (dbus_connection, You have to ref this connection, otherwise you'll get some assertion fails when you cancel print dialog right after it is shown. I came across this during today's testing. ::: modules/printbackends/cloudprint/gtkprintbackendcloudprint.c @@ +412,3 @@ + /* Base64 converts 24 bits into 32 bits, so divide the number of + * bytes by 3 (rounding up) and multiply by 4 */ + gchar encoded[(_STREAM_MAX_CHUNK_SIZE / 3 + 1) * 4]; I think that you had it correct in the previous patch. Looking at the documentation of the g_base64_encode_step(), it seems to me that you should add the +4. Am I wrong?
Created attachment 270401 [details] [review] patch with latest improvements Oh, of course you're right. It does need the extra 4 bytes for the _step() call as well as for the _close() call. Thanks for spotting the late ref count bug. New patch attached.
Review of attachment 270401 [details] [review]: Thank you for the changes. We have to have freeze break approval now, so I've asked for it on release-team@gnome.org.
Comment on attachment 270401 [details] [review] patch with latest improvements I've pushed the new module to master. Thank you Tim for all the work you've put into this. Marek
(In reply to comment #17) > (In reply to comment #16) > > So something the server sends back to us can cause an assertion failure? That > > needs to be fixed, even if you change the code here so that a non-malicious > > server doesn't send that response. > > That's right. The particular case I encountered is when the server sends a > string containing a Unicode high surrogate character followed by any character > that is not a low surrogate. The assertion is in json-glib-0.16.2: Has this issue been reported against json-glib?
(In reply to comment #22) > Has this issue been reported against json-glib? I've filed it as bug #725397.