GNOME Bugzilla – Bug 674264
Credentials from gnome-keyring is not used while printing
Last modified: 2015-01-19 09:34:37 UTC
This bug has been reported here: https://bugs.launchpad.net/ubuntu/+source/gtk+3.0/+bug/959451 Hi, For end-user desktops, we've inserted the printing credentials in the user's gnome-keyring to make printing authentication automated. This worked perfectly last year and still works today ... up to the moment when the /etc/cups/printers.conf is auto-modified with the following change : <nothing> or AuthInfoRequired none to AuthInfoRequired username,password The result is that the user gets a Window which asks the username and the password to print. Confirming the password prompt with empty user name and password will allow the job to be printed anyway thanks to the credentials in the keyring. The printers are installed system wide (authentication can not be done with the printer's URI) and we send printing jobs to a Windows printing server. Thanks,
Which exact GTK version is this about?
This is on a Ubuntu 10.04 : libgtk2.0-0 2.20.1-0ubuntu2.1 As said Lars Uebernickel (comment #11 on launchpad), this seems to be the same on the latests versions.
What exactly is "latest versions"? We don't care about GKT+ 2.20 anymore, latest 2.24.x and 3.4 are supported.
==> NEEDINFO as this needs to be tested with a supported version first.
I can confirm this issue using Ubuntu 12.04 (latest updates installed as of today) The user is not prompted for authentication at all. The user has to go to the print queue, right-click the job and select 'Authenticate'. The user is given a checkbox to "Remember Password" but it does nothing, the password is not saved in the keyring at all. Adding it manually is of no use because printing system ignores it. This was an annoyance prior to Ubuntu 12.04 because the user just had to enter their password every time. Now they cannot print unless they view the print queue and manually authenticate each job. This is horribly unintuitive. What specific information can I provide to help? I have 150 Ubuntu users to support, most of whom print to Windows shared printers requiring authentication (for accounting purposes) so I am highly motivated to help in any way I can to get this huge bug fixed.
I see above that this is marked as "needs to be tested with a supported version first". What versions are considered supported? As the previous poster indicated this is present in Ubuntu 12.04, it ship with 3.4.2. Is that supported? What additional information is needed for this report? I'm sure that myself or one of the other users that are affected by this bug would be more than happy to provide any necessary information needed to move this bug along.
Note that Ubuntu 12.04 ships gnome-keyring 3.2.x and gtk+ 3.4.x - might work together but definitely not well tested in upstream, as GNOME developers very often use the same testing/development versions (means: either both 3.2.x or both 3.4.x).
Reopening as I think the requested information has been provided.
I can confirm this also - Ubuntu 12.04.1 64-bit
we have same problem of #5. system-config-printer-applet check if any job need to be authenticated and prompt for password, but its not running by default on ubuntu 12.04. maybe it was replaced by indicator-printers-service, but this indicator doesn't auth jobs automaticaly. if i run system-config-printer-applet manualy, everything works. also, right click on job and authenticate doesn't save password. its because /usr/share/system-config-printer/jobviewer.py def on_job_authenticate_activate(self, menuitem): for jobid in self.jobids: // its not passing keyring parameter here self.display_auth_info_dialog (jobid) (ubuntu 12.04)
cups backend module is asking for user/pass as auth is requested by cups backend in gtk. gtk cups printing backend module has no facility to save/retrieve password. This works on fedora, as system-config-printer would capture auth-info from cups notification and pass the information.
Created attachment 274905 [details] [review] Patch to save / load credentials This is a patch that adds integration with gnome-keyring over the dbus secrets service interface. It first checks if a secrets service is available and if so uses this to look up credentials if not it still behaves in the same way it does currently. It stores / loads credentials from the user's default collection and prompts the user to unlock that collection if it is locked. The credentials are saved / searched in a format that will be compatible with system-config-printer if a patch to fix it's identification of printer secrets ( https://bugzilla.redhat.com/show_bug.cgi?id=1089029 ) gets included. I've tested / developed this patch with GTK 3.8.8 and GTK 2.24.22 but it basically applies without changes to both those versions and master. The attached patch is against current (2d9722e) master.
Review of attachment 274905 [details] [review]: Hi Andre, thank you very much for working on this. First of all I think that we should place the code for storing secrets inside of gtkprintbackend.c because this will allow us to use it for all backends in the future. I think that it should be part of the password dialog. Backend would just tell whether it allows to store the password + the used label. Another question is the format of the stored secrets. I'm not sure whether your proposal to system-config-printer will be accepted since current state allows users to store just one password and use it for all printers of a server. I'll wait for a decision of system-config-printer's upstream here. Also, all those _sync calls has to be changed to their async versions because sync versions shouldn't be used when GUI is involved. Thank you Marek ::: gtk/gtkprintbackend.c @@ +441,3 @@ G_STRUCT_OFFSET (GtkPrintBackendClass, request_password), NULL, NULL, + _gtk_marshal_VOID__POINTER_POINTER_POINTER_POINTER_STRING_BOOLEAN, You can use NULL instead of _gtk_marshal_VOID__POINTER_POINTER_POINTER_POINTER_STRING_BOOLEAN now. g_cclosure_marshal_generic() will be used in that case. ::: modules/printbackends/cups/Makefile.am @@ +30,3 @@ gtkprintbackendcups.c \ gtkprintercups.c \ + gtkcupsutils.c \ The backslashes should be aligned. ::: modules/printbackends/cups/gtkcupssecretsutils.c @@ +1,1 @@ +/* GTK - The GIMP Toolkit We do not use this expansions nowadays in new files (just remove the line). @@ +38,3 @@ + + /* Connect to the secret service interface */ + *session_connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); You don't need to create this connection. You can use g_dbus_proxy_new_for_bus() later with G_BUS_TYPE_SESSION as the first parameter. @@ +39,3 @@ + /* Connect to the secret service interface */ + *session_connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); + if (!*session_connection) We prefer to use the form "if (*session_connection == NULL)" for these checks (recurring). @@ +64,3 @@ + result = g_dbus_proxy_call_sync (*service_proxy, + "OpenSession", + g_variant_new("(sv)", "plain", There should be space before the left bracket (recurring). @@ +87,3 @@ + } + + const gchar *const session_path = g_variant_get_string(subresult, NULL); Declare variables in the beginning of blocks please (recurring). @@ +107,3 @@ + +done: + if (*session_connection && *session_proxy && *service_proxy && !error) You don't need to check the error, it will be set only if creation of the session_proxy fails. @@ +112,3 @@ + /* Cleanup in case of errors */ + if (error) + g_error_free (error); We use indentation of 2 spaces (recurring). @@ +130,3 @@ + + if (*session_connection) + g_object_unref (*session_connection); Is it possible to use the close_session() here? @@ +177,3 @@ + return FALSE; + + service_proxy = g_dbus_proxy_new_sync (session_connection, You should unref this. @@ +227,3 @@ + { + /* The user must be prompted to unlock the collection */ + GDBusProxy *prompt_proxy = g_dbus_proxy_new_sync(session_connection, You should use combination of g_bus_get() and g_dbus_connection_call() instead of g_dbus_proxy_new_sync() and g_dbus_proxy_call_sync() if you don't use a special feature of GDBusProxy (recurring). GDBusProxy has more overhead. @@ +388,3 @@ + } + + GVariantBuilder *const attr_builder = g_variant_builder_new (G_VARIANT_TYPE_DICTIONARY); You should unref this at the end. @@ +394,3 @@ + if (additional_labels) + { + int i = 0; You don't need to initialize the "i" variable. @@ +395,3 @@ + { + int i = 0; + for (i = 0; i < g_strv_length (additional_labels); i++) This could be written as: "for (i = 0; additional_labels[i] != NULL; i++)". @@ +475,3 @@ + + /* Check for an unlocked item get it and be done */ + gchar const **items = NULL; Could you place the "const" modifiers before other parts of declarations where possible (recurring)? @@ +535,3 @@ + } + else + GTK_NOTE (PRINTING, g_print ("Failed to unlock collection.\n")); Using brackets also for the else branch would be more consistent. @@ +626,3 @@ + if (auth_info) + { + gboolean success = 1; You should use the macro "TRUE" for true and "FALSE" for false. ::: modules/printbackends/cups/gtkcupssecretsutils.h @@ +23,3 @@ +G_BEGIN_DECLS + +gchar ** query_secrets_service (gchar **auth_info_required, Maybe secrets_service_query() would me more consistent with the other functions. ::: modules/printbackends/cups/gtkprintbackendcups.c @@ +987,3 @@ + overwrite_and_free (password); + g_free (username); + g_free (hostname); This should be a separate patch. @@ +1104,3 @@ + auth_info_required, auth_info_default, + auth_info_display, auth_info_visible, prompt, + FALSE); /* Cups password is only cached not stored. */ Why don't you store CUPS passwords? @@ +1234,3 @@ length = g_strv_length (dispatch->request->auth_info_required); + for (i = 0; i < length; i++) Place the block between brackets please. @@ +1287,3 @@ + FALSE); + for (i = 0, length = g_strv_length (auth_info); i < length; i++) + if (auth_info[i] != NULL) Using "for (i = 0; auth_info[i] != NULL; i++)" allows you to remove the "if". @@ +1290,3 @@ + { + memset (auth_info[i], 0, strlen (auth_info[i])); + g_free (auth_info[i]); You can use the overwrite_and_free() here. @@ +1294,3 @@ + } + g_free (auth_info); + auth_info = NULL; This can be shortened by use of g_clear_pointer().
Hi Marek, thanks for reviewing! Apologies for the late reply but I mistakenly expected that commenting on a bug would add me to the CC of that bug. (In reply to comment #13) > Review of attachment 274905 [details] [review]: > First of all I think that we should place the code for storing secrets inside > of gtkprintbackend.c because this will allow us to use it for all backends in > the future. I think that it should be part of the password dialog. Backend > would just tell whether it allows to store the password + the used label. I've kept it as gtkcupssecretutils as the more conservative approach as I intended it specific for that use case and having it in the cupsprintbackend allowed me to use sync api as the request_auth_info is afaik already async with the GUI. This is also quite a lot of code and the gtkprintbackend would double it's size with secrets related stuff which imho is a minor usecase when printing. Also the cloudprint backend (which i guess could also profit from secrets storage) was not around at the time when I started to work on this patch ;) I would still prefer to keep it in the cupsbackend as I did not intend it for general purpose use. There are some specifics, the way the attributes are built, username hostname, domain etc. Ideally we would have generic "gtk secrets module" like a libsecret in gtk but this is more involved than this specific usage of the secrets service api. > > Another question is the format of the stored secrets. I'm not sure whether your > proposal to system-config-printer will be accepted since current state allows > users to store just one password and use it for all printers of a server. I'll > wait for a decision of system-config-printer's upstream here. Ok, if the patch gets rejected it would be a bit more complicated to integrate with system-config-printer authentication information as the cups backend currently does not hold the device_uri of a printer job and we need it to store the auth info in a compatible way with system-config-printer. > Also, all those _sync calls has to be changed to their async versions because > sync versions shouldn't be used when GUI is involved. As stated above I think this is not necessary here as there is no GUI involved apart from the Password Request dialog which is not blocked by any of the sync calls and the main GUI is in another thread. > ::: modules/printbackends/cups/gtkcupssecretsutils.h > @@ +23,3 @@ > +G_BEGIN_DECLS > + > +gchar ** query_secrets_service (gchar **auth_info_required, > > Maybe secrets_service_query() would me more consistent with the other > functions. Yes this would be more consistent :) > ::: modules/printbackends/cups/gtkprintbackendcups.c > @@ +987,3 @@ > + overwrite_and_free (password); > + g_free (username); > + g_free (hostname); > > This should be a separate patch. > Yes, this slipped a bit in I had some more changes in that code at one point in the patch and noticed that this was still correct even without those changes. Will split it out. > @@ +1104,3 @@ > + auth_info_required, auth_info_default, > + auth_info_display, auth_info_visible, prompt, > + FALSE); /* Cups password is only cached not > stored. */ > > Why don't you store CUPS passwords? I wanted to keep this patch minimal (as it had grown quite large already) and I also did not have a test setup for this so I wanted to leave the behavior there unchanged for now. I'm also unsure what to do about the caching, keep it around in case no secrets service is available or remove it? I found the whole password / auth_info split confusing and I think the two code paths request_auth_info and request_password could and should be unified but I think this is out of scope of this issue. The comments I did not reply to are all clear to me and I will update the Patch accordingly. Please excuse a delay on this as I am on vacation for the next two weeks. Regards, and thanks again for the good review. Andre
Hi Andre, I'm sorry I'm replying so late. I was on vacation last week and it took some time to get through my mailbox since then. (In reply to comment #14) > Hi Marek, > > thanks for reviewing! Apologies for the late reply but I mistakenly expected > that commenting on a bug would add me to the CC of that bug. Yeah, this hit me several times too (this should be settable in preferences of your bugzilla account). > (In reply to comment #13) > > Review of attachment 274905 [details] [review] [details]: > > First of all I think that we should place the code for storing secrets inside > > of gtkprintbackend.c because this will allow us to use it for all backends in > > the future. I think that it should be part of the password dialog. Backend > > would just tell whether it allows to store the password + the used label. > > I've kept it as gtkcupssecretutils as the more conservative approach as I > intended it specific for that use case and having it in the cupsprintbackend > allowed me to use sync api as the request_auth_info is afaik already async with > the GUI. This is also quite a lot of code and the gtkprintbackend would double > it's size with secrets related stuff which imho is a minor usecase when > printing. > > Also the cloudprint backend (which i guess could also profit from secrets > storage) was not around at the time when I started to work on this patch ;) > > I would still prefer to keep it in the cupsbackend as I did not intend it for > general purpose use. There are some specifics, the way the attributes are > built, username hostname, domain etc. Ideally we would have generic "gtk > secrets module" like a libsecret in gtk but this is more involved than this > specific usage of the secrets service api. Ok, I was thinking about this again and it will be better to keep the code in the CUPS backend only as in your original patch. > > Another question is the format of the stored secrets. I'm not sure whether your > > proposal to system-config-printer will be accepted since current state allows > > users to store just one password and use it for all printers of a server. I'll > > wait for a decision of system-config-printer's upstream here. > > Ok, if the patch gets rejected it would be a bit more complicated to integrate > with system-config-printer authentication information as the cups backend > currently does not hold the device_uri of a printer job and we need it to store > the auth info in a compatible way with system-config-printer. > > > Also, all those _sync calls has to be changed to their async versions because > > sync versions shouldn't be used when GUI is involved. > > As stated above I think this is not necessary here as there is no GUI involved > apart from the Password Request dialog which is not blocked by any of the sync > calls and the main GUI is in another thread. Unfortunately, it is necessary. Try to place a "g_usleep (10000000)" into one of those functions listed in "gtkcupssecretsutils.h". It will block. > > ::: modules/printbackends/cups/gtkcupssecretsutils.h > > @@ +23,3 @@ > > +G_BEGIN_DECLS > > + > > +gchar ** query_secrets_service (gchar **auth_info_required, > > > > Maybe secrets_service_query() would me more consistent with the other > > functions. > > Yes this would be more consistent :) I was thinking about this a little bit more and it would be better to name the functions according to Gtk conventions as: gtk_cups_secrets_service_available() gtk_cups_secrets_service_query() gtk_cups_secrets_service_store() > > ::: modules/printbackends/cups/gtkprintbackendcups.c > > @@ +987,3 @@ > > + overwrite_and_free (password); > > + g_free (username); > > + g_free (hostname); > > > > This should be a separate patch. > > > > Yes, this slipped a bit in I had some more changes in that code at one point in > the patch and noticed that this was still correct even without those changes. > Will split it out. Thank you. > > @@ +1104,3 @@ > > + auth_info_required, auth_info_default, > > + auth_info_display, auth_info_visible, prompt, > > + FALSE); /* Cups password is only cached not > > stored. */ > > > > Why don't you store CUPS passwords? > > I wanted to keep this patch minimal (as it had grown quite large already) and I > also did not have a test setup for this so I wanted to leave the behavior there > unchanged for now. I'm also unsure what to do about the caching, keep it around > in case no secrets service is available or remove it? I found the whole > password / auth_info split confusing and I think the two code paths > request_auth_info and request_password could and should be unified but I think > this is out of scope of this issue. I'll look at possibilities we have here yet. > The comments I did not reply to are all clear to me and I will update the Patch > accordingly. > Please excuse a delay on this as I am on vacation for the next two weeks. Thank you. > Regards, > and thanks again for the good review. > > Andre You're welcome :) Marek
Created attachment 277577 [details] [review] Patch to save / load credentials Hi, I've attached an updated patch against current master with the following changes: gtkprintbackend.c: - Changed g_signal_new call to use genric marshalling - updated calls to gtk_cups_secrets_service pattern - style improvements as mentioned gtkmarshalers.list: - removed marshaler Makefile.am: - Added tabs for better alignment gtkcupssecretsutils.h - changed function names to gtk_cups_secrets_service.. gtkcupssecretsutils.c - unrefed both GVariantBuilder - removed inline read only variable declarations, moved all delcations to the beginning of a block - explict checks for NULL instead for false - style improvements as mentioned This of course still needs some work with regards to the dbus calls / using g_bus_get and / g_dbus_connection_call and doing everything async but: I started looking at the asnyc dbus API more closely and I am still unsure how to implement this / where to store persistent data over the async calls. The secrets_service_available check we can move easily into gtkprintbackendcups add a callback that takes the backend as user_data and sets a new variable secrets_service_available which can later be checked. So far so good. But with regard to the query and store functions: There are a lot dbus calls in there that have to be done in order. Opening a session, searching an Item, checking the locked state of an Item, looking up the default collection, unlocking the default collection. Please correct me if I am wrong but my naive approach into making this async would be to create a callback for each call and handing over persistent data (e.g. the session path / proxy) as user_data. Another approach (a bit more like the avahi code) would be to add more member variables to the gtkprintbackendcups class to store persistent data and exchange a reference to it through the callbacks. But both approaches does not appear very robust to me and I do not think anything is gained from getting back into the main loop between some of those calls (as there are only two results that are interesting in both functions failure / success and we do not need to interact with anything else before we know the result) while it increases complexity and error handling requirements in my opinion. So my Question is: would it not be reasonable to put those two calls into a different thread (as i originally thought they were) with callback functions for failure / success as parameters that they would in return call with g_idle_add to return to the main loop once all their sync calls are done or an error occurred? Best Regards Andre
Why is this bug still unconfirmed? I can confirm that I have had the same issue for more than a year using Ubuntu, now on 13.10 64 bits up to date. Also wanted to mention that the bug report mentioned by the reporter is a duplicate of the following one: https://bugs.launchpad.net/ubuntu/+source/gtk+3.0/+bug/445333 Looking forward to a fix for this time consuming bug (the workaround described in here does not work for me, I still have to write by hand both my user name and my password every single time I print a document). Cheers
(In reply to comment #17) > Why is this bug still unconfirmed? Because nobody cares about UNCONFIRMED vs NEW here. It's the same. > I can confirm that I have had the same issue for more than a year using Ubuntu, > now on 13.10 64 bits up to date. Please provide version information for gtk+ and gnome-keyring. No idea which versions "Ubuntu 13.10" ships. > Also wanted to mention that the bug report mentioned by the reporter is a > duplicate of the following one: > https://bugs.launchpad.net/ubuntu/+source/gtk+3.0/+bug/445333 Please feel free to bring up duplicates entirely in another ticket system in that other ticket system. Thanks!
Hi André Sorry about my poor understanding of the bug reporting system for Gnome! gnome-keyring 3.8.2-0ubuntu3.1 not sure about gtk+, I do not have a package with that name... Thanks for your reply.
Hi, I'm sorry for my late reply, I was busy with something else. (In reply to comment #16) > Created an attachment (id=277577) [details] [review] > Patch to save / load credentials > > Hi, > > I've attached an updated patch against current master with > the following changes: > > gtkprintbackend.c: > - Changed g_signal_new call to use genric marshalling > - updated calls to gtk_cups_secrets_service pattern > - style improvements as mentioned > > gtkmarshalers.list: > - removed marshaler > > Makefile.am: > - Added tabs for better alignment > > gtkcupssecretsutils.h > - changed function names to gtk_cups_secrets_service.. > > gtkcupssecretsutils.c > - unrefed both GVariantBuilder > - removed inline read only variable declarations, moved all delcations to the > beginning of a block > - explict checks for NULL instead for false > - style improvements as mentioned > > This of course still needs some work with regards to the dbus calls > / using g_bus_get and / g_dbus_connection_call and doing everything async > but: > > I started looking at the asnyc dbus API more closely and I am > still unsure how to implement this / where to store persistent > data over the async calls. You can use GTask for this. > The secrets_service_available check we can move easily into > gtkprintbackendcups add a callback that takes the backend > as user_data and sets a new variable secrets_service_available > which can later be checked. So far so good. You don't need to store the "secrets_service_available" in the backend since it is used just once. > But with regard to the query and store functions: There > are a lot dbus calls in there that have to be done in > order. Opening a session, searching an Item, checking the > locked state of an Item, looking up the default collection, > unlocking the default collection. GTask should simplify chaining of async calls too. > Please correct me if I am wrong but my naive approach into making > this async would be to create a callback for each call and > handing over persistent data (e.g. the session path / proxy) as > user_data. Basically, yes. You can use g_task_set_task_data() to carry the data through the calls. > Another approach (a bit more like the avahi code) would > be to add more member variables to the gtkprintbackendcups class > to store persistent data and exchange a reference to it through > the callbacks. It would be better to keep this just as *_async() and *_finish() pairs without adding members to the backend. > But both approaches does not appear very robust to me and I do not > think anything is gained from getting back into the main loop > between some of those calls (as there are only two results that > are interesting in both functions failure / success and we do > not need to interact with anything else before we know the result) What about prompt? It should be attached to the dialog window. > while it increases complexity and error handling requirements in my opinion. > > So my Question is: would it not be reasonable to put those two calls > into a different thread (as i originally thought they were) with > callback functions for failure / success as parameters that they would in > return > call with g_idle_add to return to the main loop once all their sync calls > are done or an error occurred? Don't add another thread here please. We've already put a lot of effort into not requiring additional thread in CUPS code. E.g. gdbus has already its own thread, this would add another one. > Best Regards > Andre Thank you very much for working on this Regards Marek
Created attachment 280295 [details] [review] Patch to save / load credentials async version Hi, (In reply to comment #20) > I'm sorry for my late reply, I was busy with something else. Ditto :) > (In reply to comment #16) > > Created an attachment (id=277577) [details] [review] [details] [review] > > Patch to save / load credentials ... > > This of course still needs some work with regards to the dbus calls > > / using g_bus_get and / g_dbus_connection_call and doing everything async > > but: > > > > I started looking at the asnyc dbus API more closely and I am > > still unsure how to implement this / where to store persistent > > data over the async calls. > > You can use GTask for this. Ok, I'm now using GTask API for this, hope I understood it's usage correctly ;) See attached (updated) patch. The printbackendcups now calls lookup_auth_info before calling request_auth_info. This function decides how to proceed: If no secrets service is available it just moves on to request_auth_info otherwise it triggers the task creation with gtk_cups_secrets_service_query_task. (I've decided to go with that naming as the parameters are somewhat similar to g_task_new). lookup_auth_info_cb is the according callback. > > The secrets_service_available check we can move easily into > > gtkprintbackendcups add a callback that takes the backend > > as user_data and sets a new variable secrets_service_available > > which can later be checked. So far so good. > > You don't need to store the "secrets_service_available" in the backend > since it is used just once. I changed the whole semantics of this. Instead of just checking if the interface exists once in secrets_service_available I've now added gtk_cups_secrets_service_watch to monitor the secrets service interface and to gracefully handle cases where the secrets service is killed or disappears after the printbackend is initialized. > > But with regard to the query and store functions: There > > are a lot dbus calls in there that have to be done in > > order. Opening a session, searching an Item, checking the > > locked state of an Item, looking up the default collection, > > unlocking the default collection. > GTask should simplify chaining of async calls too. Still this was a big rewrite :/ but GTask certainly helped and I've learned a lot about GIO. > > Please correct me if I am wrong but my naive approach into making > > this async would be to create a callback for each call and > > handing over persistent data (e.g. the session path / proxy) as > > user_data. > > Basically, yes. You can use g_task_set_task_data() to carry the data > through the calls. I've added a secrets_service_data struct that I use to carry the data around as the task_data. > > Another approach (a bit more like the avahi code) would > > be to add more member variables to the gtkprintbackendcups class > > to store persistent data and exchange a reference to it through > > the callbacks. > > It would be better to keep this just as *_async() and *_finish() pairs > without adding members to the backend. I've tried to minimize it but I had to add the cancellable and also the watch id to cancel / unwatch correctly when the backend is destroyed. And I still think that we need to save the information if a secrets service is available as we need to have this information in request_auth_info to determine if we can proceed with showing the normal "Password dialog" (without remember password option) or if we should try to look into the secrets service store (which might cause a delay). > > So my Question is: would it not be reasonable to put those two calls > > into a different thread (as i originally thought they were) with > > callback functions for failure / success as parameters that they would in > > return > > call with g_idle_add to return to the main loop once all their sync calls > > are done or an error occurred? > > Don't add another thread here please. We've already put a lot of effort into > not requiring > additional thread in CUPS code. E.g. gdbus has already its own thread, this > would add another one. *sigh* this would have been so much simpler ;-) > Thank you very much for working on this Thanks for staying with me. Remarks: I've stuck with using a proxy for the secrets item as it allowed me to lookup the properties of the secret without further ado. For storing I've chosen a "fire and forget" approach without a callback into the printbackend as there is no interaction required if it fails in the background. I've also chosen to copy the data necessary to store to avoid problems in case the printbackend is destroyed before the storing finishes. Best Regards, Andre
Review of attachment 280295 [details] [review]: Hi, (In reply to comment #21) > Created an attachment (id=280295) [details] [review] > Patch to save / load credentials async version > > Hi, > > (In reply to comment #20) > > I'm sorry for my late reply, I was busy with something else. > > Ditto :) It took me several days to review the patch and test it, so the delay (and some other tasks too...). :) > > (In reply to comment #16) > > > Created an attachment (id=277577) [details] [review] [details] [review] [details] [review] > > > Patch to save / load credentials > ... > > > This of course still needs some work with regards to the dbus calls > > > / using g_bus_get and / g_dbus_connection_call and doing everything async > > > but: > > > > > > I started looking at the asnyc dbus API more closely and I am > > > still unsure how to implement this / where to store persistent > > > data over the async calls. > > > > You can use GTask for this. > > Ok, I'm now using GTask API for this, hope I understood it's usage > correctly ;) See attached (updated) patch. > The printbackendcups now calls lookup_auth_info before calling > request_auth_info. This function decides how to proceed: > If no secrets service is available it just > moves on to request_auth_info otherwise it triggers the > task creation with gtk_cups_secrets_service_query_task. (I've > decided to go with that naming as the parameters are somewhat > similar to g_task_new). > lookup_auth_info_cb is the according callback. It looks good. > > > The secrets_service_available check we can move easily into > > > gtkprintbackendcups add a callback that takes the backend > > > as user_data and sets a new variable secrets_service_available > > > which can later be checked. So far so good. > > > > You don't need to store the "secrets_service_available" in the backend > > since it is used just once. > > I changed the whole semantics of this. Instead of > just checking if the interface exists once in secrets_service_available > I've now added gtk_cups_secrets_service_watch to monitor the secrets > service interface and to gracefully handle cases where the > secrets service is killed or disappears after the printbackend is initialized. Good idea :). > > > But with regard to the query and store functions: There > > > are a lot dbus calls in there that have to be done in > > > order. Opening a session, searching an Item, checking the > > > locked state of an Item, looking up the default collection, > > > unlocking the default collection. > > GTask should simplify chaining of async calls too. > > Still this was a big rewrite :/ but GTask certainly helped and I've learned a > lot about GIO. I see that you've put a lot effort into this. > > > Please correct me if I am wrong but my naive approach into making > > > this async would be to create a callback for each call and > > > handing over persistent data (e.g. the session path / proxy) as > > > user_data. > > > > Basically, yes. You can use g_task_set_task_data() to carry the data > > through the calls. > > I've added a secrets_service_data struct that I use to carry the > data around as the task_data. I would name it SecretsServiceData. > > > Another approach (a bit more like the avahi code) would > > > be to add more member variables to the gtkprintbackendcups class > > > to store persistent data and exchange a reference to it through > > > the callbacks. > > > > It would be better to keep this just as *_async() and *_finish() pairs > > without adding members to the backend. > > I've tried to minimize it but I had to add the cancellable and also the > watch id to cancel / unwatch correctly when the backend is destroyed. Sure. > And I still think that we need to save the information if a secrets service > is available as we need to have this information in request_auth_info to > determine if we can proceed with showing the normal "Password dialog" > (without remember password option) or if we should try to look into > the secrets service store (which might cause a delay). > > > > So my Question is: would it not be reasonable to put those two calls > > > into a different thread (as i originally thought they were) with > > > callback functions for failure / success as parameters that they would in > > > return > > > call with g_idle_add to return to the main loop once all their sync calls > > > are done or an error occurred? > > > > Don't add another thread here please. We've already put a lot of effort into > > not requiring > > additional thread in CUPS code. E.g. gdbus has already its own thread, this > > would add another one. > > *sigh* this would have been so much simpler ;-) I know, I was in similar situation several times. > > Thank you very much for working on this > > Thanks for staying with me. > > Remarks: > I've stuck with using a proxy for the secrets item as it allowed > me to lookup the properties of the secret without further ado. > > For storing I've chosen a "fire and forget" approach > without a callback into the printbackend as there is no interaction > required if it fails in the background. Yeah, there's really no need for an interaction there. > I've also chosen to copy the data necessary to store to avoid problems > in case the printbackend is destroyed before the storing finishes. > > Best Regards, > Andre I'll attach proposed changes in the form of a patch so you can see what I exactly mean by my comments. Thank you very much for all the work. Regards Marek ::: gtk/gtkprintbackend.c @@ +683,3 @@ +static void +store_auth_info_toggled (GtkCheckButton *chkbtn, + gpointer user_data) Please align the parameters names. @@ +686,3 @@ +{ + gboolean *data = (gboolean *) user_data; + *data = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON (chkbtn)); There should be a space before the "(". @@ +716,2 @@ else + gtk_print_backend_set_password (backend, priv->auth_info_required, NULL, priv->store_auth_info); I think that you should pass FALSE here. It would illustrate the purpose better. @@ +836,3 @@ + g_signal_connect (chkbtn, "toggled", + G_CALLBACK (store_auth_info_toggled), + &(priv->store_auth_info)); Could you move this block 2 spaces to the left (so the indentation is 2 spaces)? ::: modules/printbackends/cups/gtkcupssecretsutils.c @@ +27,3 @@ +#define SECRETS_IFACE(interface) "org.freedesktop.Secret."interface +#define SECRETS_PATH "/org/freedesktop/secrets" +#define SECRETS_TIMEOUT 5000 Could you align those values? @@ +32,3 @@ +{ + GDBusConnection *dbus_connection; + gboolean store_info; I would prefer to have an enum here instead of the boolean. Something like "SecretsServiceAction action;": typedef enum { SECRETS_SERVICE_ACTION_QUERY, SECRETS_SERVICE_ACTION_STORE } SecretsServiceAction; @@ +60,3 @@ +{ + GVariantBuilder *attr_builder = NULL; + GVariant *ret = NULL; Indentation. @@ +73,3 @@ + g_variant_builder_add (attr_builder, "{ss}", "uri", printer_uri); + + if (additional_labels) I would prefer "if (additional_labels != NULL)" for newly written code. @@ +184,3 @@ + if (secret == NULL || g_variant_n_children (secret) != 4) + { + GTK_NOTE (PRINTING, g_print ("Get secret response invalid.\n")); You should unref the "secret" here if "secret != NULL". @@ +194,3 @@ + g_variant_unref (secret); + + if (ba_passwd == NULL || strlen (ba_passwd) > len + 1) Is it possible that the ba_passwd is longer then len + 1? @@ +201,3 @@ + } + + auth_info[pw_field] = g_strdup (ba_passwd); Maybe you could just do this (and remove the check for the length before): auth_info[pw_field] = g_strndup (ba_passwd, len); @@ +225,3 @@ + { + /* Not all fields of auth_info are neccessarily written so we can not + use strfreev here */ I think that you can use the strfreev() since this is basically it. @@ +255,3 @@ + + g_variant_get (output, "(&o&o)", &item, NULL); + if (item && strlen (item) > 1) if (item != NULL && strlen (item) > 1) @@ +349,3 @@ + secret, + TRUE), + NULL, It would be good to have the expected signatures here (G_VARIANT_TYPE ("(oo)") in this case). As in all other functions. @@ +399,3 @@ + + /* Prompt successfull, proceed to get or store secret */ + if (task_data->store_info) You can change this to this if you use the enum: switch (task_data->action) { case SECRETS_SERVICE_ACTION_STORE: ... break; case SECRETS_SERVICE_ACTION_QUERY: ... break; } @@ +480,3 @@ + g_variant_get (output, "(@ao&o)", NULL, &prompt_path); + + if (prompt_path != NULL && strlen (prompt_path) > 2) I would expect that "> 1" is enough (since the path for which we don't need to do the prompt is just "/"). @@ +609,3 @@ + if (locked == NULL) + { + GTK_NOTE (PRINTING, g_print ("Failed to look up locked property on item.\n")); Maybe you should highlight the name of the property somehow: e.g. "Failed to look up \"Locked\" property on item.\n". @@ +696,3 @@ + if (*items == NULL) + { + g_variant_unref (item_paths); You should call the "g_free ((gpointer) items);" here too. @@ +734,3 @@ + *session_variant; + secrets_service_data *task_data; + const gchar *session_path; You can save this variable by storing the path directly to the "task_data->session_path". @@ +759,3 @@ + } + + session_path = g_variant_get_string (session_variant, NULL); task_data->session_path = g_variant_dup_string (session_variant, NULL); @@ +829,3 @@ + GTask *task; + secrets_service_data *task_data; + GDBusConnection *dbus_connection; You can save also this variable by directly using the "task_data->dbus_connection". @@ +889,3 @@ + secrets_service_data *task_data = data; + + if (task_data->collection_path != NULL) There is no need for this check. @@ +894,3 @@ + } + + if (task_data->auth_info_labels != NULL) This one too. @@ +899,3 @@ + } + + if (task_data->printer_uri != NULL) And this one. @@ +911,3 @@ + g_clear_pointer (&task_data->auth_info[i], g_free); + } + g_clear_pointer (task_data->auth_info, g_free); Shouldn't this line be: g_clear_pointer (&task_data->auth_info, g_free); ? @@ +1047,3 @@ + { + task_data->auth_info_labels[i] = g_strdup (auth_info_labels[i]); + } g_strdupv() should do the job: task_data->auth_info = g_strdupv (auth_info); task_data->auth_info_labels = g_strdupv (auth_info_labels); ::: modules/printbackends/cups/gtkcupssecretsutils.h @@ +28,3 @@ + GAsyncReadyCallback callback, + gpointer user_data, + GtkCupsRequest *request); I think that you should pass the printer URI and auth_info_required as parameters and not as a part of the request. @@ +34,3 @@ +void gtk_cups_secrets_service_store (gchar **auth_info, + gchar **auth_info_labels, + const gchar *printer_uri); Could you align all the function names and parameters names of the functions? ::: modules/printbackends/cups/gtkprintbackendcups.c @@ +800,3 @@ cups_get_local_default_printer (backend_cups); + + backend_cups->secrets_service_cancellable = NULL; I would prefer to create the GCancellable here. @@ +928,3 @@ gchar **auth_info_required, + gchar **auth_info, + gboolean store_auth_info) Could you align the names here and in all other functions? @@ +1269,3 @@ + + g_object_unref (task); + return; There is no need for this return (as in all other functions). @@ +1296,3 @@ + } + + if (dispatch->backend->secrets_service_available && need_secret_auth_info) You should move the "g_idle_add (check_auth_info, user_data);" from the request_auth_info() above this line, otherwise the application won't allow user to quit the application if he cancels the password dialog. @@ +6062,3 @@ + if (backend->secrets_service_cancellable == NULL) + { + backend->secrets_service_cancellable = g_cancellable_new (); I would prefer to have this in the gtk_print_backend_cups_init(). @@ +6076,3 @@ + if (backend->secrets_service_cancellable != NULL) + { + g_clear_object (&backend->secrets_service_cancellable); This could be removed then.
Created attachment 281877 [details] [review] proposed changes
I forgot to mention one more change: + if (can_store_auth_info) + { + chkbtn = gtk_check_button_new_with_label (_("Remember Password")); There should be _("Remember password").
Created attachment 283912 [details] [review] Combined patch with proposed changes from Marek Hi, (In reply to comment #22) > Review of attachment 280295 [details] [review]: > > ::: gtk/gtkprintbackend.c > @@ +683,3 @@ > +static void > +store_auth_info_toggled (GtkCheckButton *chkbtn, > + gpointer user_data) > > Please align the parameters names. > > @@ +686,3 @@ > +{ > + gboolean *data = (gboolean *) user_data; > + *data = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON (chkbtn)); > > There should be a space before the "(". Ok. > > @@ +716,2 @@ > else > + gtk_print_backend_set_password (backend, priv->auth_info_required, NULL, > priv->store_auth_info); > > I think that you should pass FALSE here. It would illustrate the purpose > better. Ok. > @@ +836,3 @@ > + g_signal_connect (chkbtn, "toggled", > + G_CALLBACK (store_auth_info_toggled), > + &(priv->store_auth_info)); > > Could you move this block 2 spaces to the left (so the indentation is 2 > spaces)? Ok (sorry to have missed this in the first place). > ::: modules/printbackends/cups/gtkcupssecretsutils.c > @@ +27,3 @@ > +#define SECRETS_IFACE(interface) "org.freedesktop.Secret."interface > +#define SECRETS_PATH "/org/freedesktop/secrets" > +#define SECRETS_TIMEOUT 5000 > > Could you align those values? Ok. > @@ +32,3 @@ > +{ > + GDBusConnection *dbus_connection; > + gboolean store_info; > > I would prefer to have an enum here instead of the boolean. Something like > "SecretsServiceAction action;": > typedef enum > { > SECRETS_SERVICE_ACTION_QUERY, > SECRETS_SERVICE_ACTION_STORE > } SecretsServiceAction; I can see that this could be useful in the future. > @@ +60,3 @@ > +{ > + GVariantBuilder *attr_builder = NULL; > + GVariant *ret = NULL; > > Indentation. Ok. > > @@ +73,3 @@ > + g_variant_builder_add (attr_builder, "{ss}", "uri", printer_uri); > + > + if (additional_labels) > > I would prefer "if (additional_labels != NULL)" for newly written code. Yes, missed that. > > @@ +184,3 @@ > + if (secret == NULL || g_variant_n_children (secret) != 4) > + { > + GTK_NOTE (PRINTING, g_print ("Get secret response invalid.\n")); > > You should unref the "secret" here if "secret != NULL". Right. > > @@ +194,3 @@ > + g_variant_unref (secret); > + > + if (ba_passwd == NULL || strlen (ba_passwd) > len + 1) > > Is it possible that the ba_passwd is longer then len + 1? > > @@ +201,3 @@ > + } > + > + auth_info[pw_field] = g_strdup (ba_passwd); > > Maybe you could just do this (and remove the check for the length before): > auth_info[pw_field] = g_strndup (ba_passwd, len); Change 1: The idea there was that the secret is not necessarily zero terminated (should not happen as we don't request it encoded in the session) but another application might put anything in there. In that case I wanted to fail as early as possible. Stripping some bytes of the secret will automatically make it invalid and in that case I rather have the function fail then return an invalid secret. I've changed the error check back and added a comment but left the g_strndup in place as it is more explicit. > > @@ +225,3 @@ > + { > + /* Not all fields of auth_info are neccessarily written so we can not > + use strfreev here */ > > I think that you can use the strfreev() since this is basically it. Change 2: No, this walks over the g_strv_length (task_data->auth_info_required) and not task_data->auth_info while g_strfreev stops at the first NULL this ensures that if you have something like is properly handled: auth_info[0] Username: foobar auth_info[1] Hostname: NULL auth_info[2] Password: baz g_strfreev would not free auth_info[2] As this situation is an explicit cause for an error (See the Error out if we did not find everything above) it should be handled. > > @@ +255,3 @@ > + > + g_variant_get (output, "(&o&o)", &item, NULL); > + if (item && strlen (item) > 1) > > if (item != NULL && strlen (item) > 1) > > @@ +349,3 @@ > + secret, > + TRUE), > + NULL, > > It would be good to have the expected signatures here (G_VARIANT_TYPE ("(oo)") > in this case). As in all other functions. Ok. I thought this was optional as I always check the return values of g_variant_get calls. > > @@ +399,3 @@ > + > + /* Prompt successfull, proceed to get or store secret */ > + if (task_data->store_info) > > You can change this to this if you use the enum: > switch (task_data->action) > { > case SECRETS_SERVICE_ACTION_STORE: > ... > break; > > case SECRETS_SERVICE_ACTION_QUERY: > ... > break; > } Ok. > @@ +480,3 @@ > + g_variant_get (output, "(@ao&o)", NULL, &prompt_path); > + > + if (prompt_path != NULL && strlen (prompt_path) > 2) > > I would expect that "> 1" is enough (since the path for which we don't need to > do the prompt is just "/"). You are right. I can't remember why I wrote 2 there. > > @@ +609,3 @@ > + if (locked == NULL) > + { > + GTK_NOTE (PRINTING, g_print ("Failed to look up locked property on > item.\n")); > > Maybe you should highlight the name of the property somehow: e.g. "Failed to > look up \"Locked\" property on item.\n". Yes could be confusing otherwise. > > @@ +696,3 @@ > + if (*items == NULL) > + { > + g_variant_unref (item_paths); > > You should call the "g_free ((gpointer) items);" here too. Yes. > @@ +734,3 @@ > + *session_variant; > + secrets_service_data *task_data; > + const gchar *session_path; > > You can save this variable by storing the path directly to the > "task_data->session_path". > > @@ +759,3 @@ > + } > + > + session_path = g_variant_get_string (session_variant, NULL); > > task_data->session_path = g_variant_dup_string (session_variant, NULL); > > @@ +829,3 @@ > + GTask *task; > + secrets_service_data *task_data; > + GDBusConnection *dbus_connection; > > You can save also this variable by directly using the > "task_data->dbus_connection". > > @@ +889,3 @@ > + secrets_service_data *task_data = data; > + > + if (task_data->collection_path != NULL) > > There is no need for this check. > > @@ +894,3 @@ > + } > + > + if (task_data->auth_info_labels != NULL) > > This one too. > > @@ +899,3 @@ > + } > + > + if (task_data->printer_uri != NULL) > > And this one. Sorry, bad habit :) > @@ +911,3 @@ > + g_clear_pointer (&task_data->auth_info[i], g_free); > + } > + g_clear_pointer (task_data->auth_info, g_free); > > Shouldn't this line be: > g_clear_pointer (&task_data->auth_info, g_free); > ? Urgh, of course. > @@ +1047,3 @@ > + { > + task_data->auth_info_labels[i] = g_strdup (auth_info_labels[i]); > + } > > g_strdupv() should do the job: > > task_data->auth_info = g_strdupv (auth_info); > task_data->auth_info_labels = g_strdupv (auth_info_labels); > > ::: modules/printbackends/cups/gtkcupssecretsutils.h > @@ +28,3 @@ > + GAsyncReadyCallback callback, > + gpointer user_data, > + GtkCupsRequest *request); > > I think that you should pass the printer URI and auth_info_required as > parameters and not as a part of the request. Yes i can see the advantage that this reduces the coupling with the gtkprintbackendcups. > > @@ +34,3 @@ > +void gtk_cups_secrets_service_store (gchar **auth_info, > + gchar **auth_info_labels, > + const gchar *printer_uri); > > Could you align all the function names and parameters names of the functions? Ok. > ::: modules/printbackends/cups/gtkprintbackendcups.c > @@ +800,3 @@ > cups_get_local_default_printer (backend_cups); > + > + backend_cups->secrets_service_cancellable = NULL; > > I would prefer to create the GCancellable here. Ok. > > @@ +928,3 @@ > gchar **auth_info_required, > + gchar **auth_info, > + gboolean store_auth_info) > > Could you align the names here and in all other functions? Ok. > > @@ +1269,3 @@ > + > + g_object_unref (task); > + return; > > There is no need for this return (as in all other functions). Ok I thought this would be preferable style. > @@ +1296,3 @@ > + } > + > + if (dispatch->backend->secrets_service_available && need_secret_auth_info) > > You should move the "g_idle_add (check_auth_info, user_data);" from the > request_auth_info() above this line, otherwise the application won't allow user > to quit the application if he cancels the password dialog. > > @@ +6062,3 @@ > + if (backend->secrets_service_cancellable == NULL) > + { > + backend->secrets_service_cancellable = g_cancellable_new (); > > I would prefer to have this in the gtk_print_backend_cups_init(). Oh. Ok. > > @@ +6076,3 @@ > + if (backend->secrets_service_cancellable != NULL) > + { > + g_clear_object (&backend->secrets_service_cancellable); > > This could be removed then. Ok. I've applied your Patch from Comment 23 (thanks for that) With the two changes (marked with "Change 1" and "Change 2" in this reply) and created a combined patch. I've tested this patch with gtk 3.10.8 under Ubuntu but the attached version is against master (applies without changes against 3.10.8). Best regards, Andre
Review of attachment 283912 [details] [review]: Hi, thank you for the patch. (In reply to comment #25) > Hi, > > (In reply to comment #22) > > Review of attachment 280295 [details] [review] [details]: > > > > @@ +194,3 @@ > > + g_variant_unref (secret); > > + > > + if (ba_passwd == NULL || strlen (ba_passwd) > len + 1) > > > > Is it possible that the ba_passwd is longer then len + 1? > > > > @@ +201,3 @@ > > + } > > + > > + auth_info[pw_field] = g_strdup (ba_passwd); > > > > Maybe you could just do this (and remove the check for the length before): > > auth_info[pw_field] = g_strndup (ba_passwd, len); > > Change 1: The idea there was that the secret is not necessarily zero terminated > (should not happen as we don't request it encoded in the session) but another > application might put anything in there. In that case I wanted to fail as early > as possible. Stripping some bytes of the secret will automatically make it > invalid and in that case I rather have the function fail then return an invalid > secret. > > I've changed the error check back and added a comment but left the g_strndup in > place as it is more explicit. Ok, looks good then. > > > > @@ +225,3 @@ > > + { > > + /* Not all fields of auth_info are neccessarily written so we can not > > + use strfreev here */ > > > > I think that you can use the strfreev() since this is basically it. > > Change 2: No, this walks over the g_strv_length (task_data->auth_info_required) > and not task_data->auth_info while g_strfreev stops at the first NULL this > ensures that if you have something like is properly handled: > auth_info[0] Username: foobar > auth_info[1] Hostname: NULL > auth_info[2] Password: baz > > g_strfreev would not free auth_info[2] > > As this situation is an explicit cause for an error (See the Error out if we > did not find everything above) it should be handled. Right, I overlooked that the length is taken from the task_data->auth_info_required. > > > > @@ +255,3 @@ > > + > > + g_variant_get (output, "(&o&o)", &item, NULL); > > + if (item && strlen (item) > 1) > > > > if (item != NULL && strlen (item) > 1) > > > > @@ +349,3 @@ > > + secret, > > + TRUE), > > + NULL, > > > > It would be good to have the expected signatures here (G_VARIANT_TYPE ("(oo)") > > in this case). As in all other functions. > > Ok. I thought this was optional as I always check the return values of > g_variant_get calls. It is good to have it there because it fails in standard way then (NULL result + GError filled). It is useful if the API changes. > > @@ +889,3 @@ > > + secrets_service_data *task_data = data; > > + > > + if (task_data->collection_path != NULL) > > > > There is no need for this check. > > > > @@ +894,3 @@ > > + } > > + > > + if (task_data->auth_info_labels != NULL) > > > > This one too. > > > > @@ +899,3 @@ > > + } > > + > > + if (task_data->printer_uri != NULL) > > > > And this one. > > Sorry, bad habit :) Actually, a good habit, but not needed here :). > I've applied your Patch from Comment 23 (thanks for that) With the two changes > (marked with "Change 1" and "Change 2" in this reply) and created a combined > patch. The changes look good. > I've tested this patch with gtk 3.10.8 under Ubuntu but the attached version is > against master (applies without changes against 3.10.8). I've just tested it on master and it works well for me. > Best regards, > Andre I'm going to propose this for 3.14 but we need to ask release-team for exception right now (https://wiki.gnome.org/ThreePointThirteen). Thank you for the huge amount of work you've put into this. Regards Marek
> + chkbtn = gtk_check_button_new_with_label (_("Remember password")); Out of curiosity, why isn't this gtk_check_button_new_with_mnemonic?
(In reply to comment #27) > > + chkbtn = gtk_check_button_new_with_label (_("Remember password")); > > Out of curiosity, why isn't this gtk_check_button_new_with_mnemonic? You are right, since we use mnemonic already in the dialog, we should do it here as well. I'll push it with "gtk_check_button_new_with_mnemonic (_("_Remember password"))".
Comment on attachment 283912 [details] [review] Combined patch with proposed changes from Marek I've just pushed the patch to master together with the change mentioned in comment #28. Thank you Andre for all the work you've put into this. Regards Marek
It would be really great to have these patches in GTK2, too? @Marek do you think this is possible?
(In reply to comment #30) > It would be really great to have these patches in GTK2, too? @Marek do you > think this is possible? From the coding point of view, it is possible. But since I consider gtk2 as a super-stable release, I'm not sure whether we should introduce this feature there.
@Marek, I understand your concerns about stability for GTK+2. From our point of view, the changeset would not have any negative impact on stability of the components. What about, if we would provide a patchset with that in mind, for review? As having this feature in GTK+2 would be a great benefit to us. We are still limited on a large number of applications that make use of this version of the toolkit.
Hi, (In reply to comment #31) > (In reply to comment #30) > > It would be really great to have these patches in GTK2, too? @Marek do you > > think this is possible? > > From the coding point of view, it is possible. But since I consider gtk2 as a > super-stable release, I'm not sure whether we should introduce this feature > there. Now after some months of testing this I thought we could maybe revisit the decision of weather or not to apply this to GTK 2. I've opened BUG 743166 for that. Regards, Andre