GNOME Bugzilla – Bug 678623
printing: g-s-d doesn't show notifications for remote CUPS server
Last modified: 2019-03-20 11:02:09 UTC
If /etc/cups/client.conf points to a remote server then g-s-d doesn't show notifications because it doesn't get them through dbus. This has to be solved by a polling of the CUPS server for new notifications. Marek
Created attachment 249793 [details] [review] Show notifications for print jobs on remote servers This patch implements solution proposed by Tim Waugh here https://bugzilla.redhat.com/show_bug.cgi?id=886845. It periodically checks for new notifications on remote CUPS servers.
Review of attachment 249793 [details] [review]: The patch mixes code reorganisation and functional changes. Please separate them in different patches. ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +159,3 @@ + return TRUE; + } + else { on the same line as the brace above. @@ +309,3 @@ + +static void +process_cups_notification (GsdPrintNotificationsManager *manager, Could you make that split a separate patch please? It's pretty hard to read. @@ +317,3 @@ + const char *printer_state_reasons, + gboolean printer_is_accepting_jobs, + gint notify_job_id, Why was it a uint before an now an int? Please make a separate patch for that. @@ +438,1 @@ + if (g_strcmp0 (notify_subscribed_event, "printer-added") == 0) { Again, could this be a separate patch? @@ +447,3 @@ secondary_text = g_strdup (printer_name); } + } else if (g_strcmp0 (notify_subscribed_event, "printer-deleted") == 0) { Ditto. @@ +458,3 @@ cupsFreeDests (manager->priv->num_dests, manager->priv->dests); manager->priv->num_dests = cupsGetDests (&manager->priv->dests); + } else if (g_strcmp0 (notify_subscribed_event, "job-completed") == 0 && my_job) { Ditto. @@ +492,3 @@ break; } + } else if (g_strcmp0 (notify_subscribed_event, "job-state-changed") == 0 && my_job) { Ditto. @@ +504,3 @@ break; case IPP_JOB_STOPPED: + g_hash_table_remove (manager->priv->printing_printers, Should all those additions be separate as well? @@ +1267,3 @@ + if (server_is_local (cupsServer ())) { + manager->priv->num_dests = cupsGetDests (&manager->priv->dests); + gnome_settings_profile_msg ("got dests"); Why is this a profile message? Use g_debug instead. @@ +1271,2 @@ + renew_subscription (manager); + g_timeout_add_seconds (RENEW_INTERVAL, renew_subscription, manager); When do you disable this? @@ +1285,3 @@ + NULL); + } + else { Same line as the brace above. @@ +1337,3 @@ + if (manager->priv->check_source_id > 0) + g_source_remove (manager->priv->check_source_id); And set it to 0;
Created attachment 249903 [details] [review] Use debug message instead of profiling message
Created attachment 249904 [details] [review] Don't use DBus recipient URI for remote servers
Created attachment 249905 [details] [review] Don't run connection test for local server
Created attachment 249906 [details] [review] Use IPP method for getting IPP notifications
Created attachment 249907 [details] [review] Regularly check for notifications from remote CUPS servers
Created attachment 249908 [details] [review] Show final job states for remote CUPS server
Created attachment 249909 [details] [review] Stop renewing of CUPS subscriptions once we are finished
Created attachment 249910 [details] [review] Coding style fixes
Hi, thank you for the review. (In reply to comment #2) > Review of attachment 249793 [details] [review]: > > The patch mixes code reorganisation and functional changes. Please separate > them in different patches. I tried to separate it to different patches but the biggest part is still quite big. The function on_cups_notification() is now used as just a trigger from now (trigerred over DBus by CUPS). It says us that there is a new notification on local CUPS server. We call then the process_new_notifications() which gets new notifications from the print server and processes each of them them by process_cups_notification() (its body is the taken from the original on_cups_notification()). The process_new_notifications() is also called from timeout source for remote servers. > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +159,3 @@ > + return TRUE; > + } > + else { > > on the same line as the brace above. Done. > @@ +309,3 @@ > + > +static void > +process_cups_notification (GsdPrintNotificationsManager *manager, > > Could you make that split a separate patch please? It's pretty hard to read. Done. > @@ +317,3 @@ > + const char *printer_state_reasons, > + gboolean printer_is_accepting_jobs, > + gint notify_job_id, > > Why was it a uint before an now an int? Please make a separate patch for that. I turned it back to uint since specification says that the id should be 1 or higher so I can use 0 for signalising that the value was not present in the IPP response. > @@ +438,1 @@ > + if (g_strcmp0 (notify_subscribed_event, "printer-added") == 0) { > > Again, could this be a separate patch? > > @@ +447,3 @@ > secondary_text = g_strdup (printer_name); > } > + } else if (g_strcmp0 (notify_subscribed_event, "printer-deleted") == 0) { > > Ditto. Done. > @@ +458,3 @@ > cupsFreeDests (manager->priv->num_dests, manager->priv->dests); > manager->priv->num_dests = cupsGetDests (&manager->priv->dests); > + } else if (g_strcmp0 (notify_subscribed_event, "job-completed") == 0 && my_job) { > > Ditto. Done. > @@ +492,3 @@ > break; > } > + } else if (g_strcmp0 (notify_subscribed_event, "job-state-changed") == 0 && my_job) { > > Ditto. Done. > @@ +504,3 @@ > break; > case IPP_JOB_STOPPED: > + g_hash_table_remove (manager->priv->printing_printers, > > Should all those additions be separate as well? Separated. > @@ +1267,3 @@ > + if (server_is_local (cupsServer ())) { > + manager->priv->num_dests = cupsGetDests (&manager->priv->dests); > + gnome_settings_profile_msg ("got dests"); > > Why is this a profile message? Use g_debug instead. Changed. > @@ +1271,2 @@ > + renew_subscription (manager); > + g_timeout_add_seconds (RENEW_INTERVAL, renew_subscription, manager); > > When do you disable this? Added patch for that. > @@ +1285,3 @@ > + NULL); > + } > + else { > > Same line as the brace above. Done. > @@ +1337,3 @@ > > + if (manager->priv->check_source_id > 0) > + g_source_remove (manager->priv->check_source_id); > > And set it to 0; Done. Regards Marek
Review of attachment 249903 [details] [review]: ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +1010,2 @@ manager->priv->num_dests = cupsGetDests (&manager->priv->dests); + g_debug ("got dests"); Maybe it would be useful to differentiate the 2 debug messages?
Review of attachment 249904 [details] [review]: Looks good.
Review of attachment 249905 [details] [review]: ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +1054,3 @@ + g_socket_client_connect_to_host_async (client, + address, + 631, Any reason you don't use the ippPort() output? @@ +1088,2 @@ + renew_subscription (manager); + g_timeout_add_seconds (RENEW_INTERVAL, renew_subscription, manager); This needs to have an ID assigned to it so you can cancel it on exit. @@ +1089,3 @@ + g_timeout_add_seconds (RENEW_INTERVAL, renew_subscription, manager); + + manager->priv->cups_bus_connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, NULL); Urgh. No sync please. @@ +1091,3 @@ + manager->priv->cups_bus_connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, NULL); + + g_dbus_connection_signal_subscribe (manager->priv->cups_bus_connection, This needs to be cancellable as well.
Review of attachment 249906 [details] [review]: ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +395,3 @@ N_("There is a problem on printer '%s'.") }; + if (g_strcmp0 (notify_subscribed_event, "printer-added") != 0 && Why do you use different identifiers than the original code? If you're going to translate the notification names from CUPS to signal names, could you split it up from this patch? @@ +447,1 @@ /* Translators: A printer has been removed */ This translator comment won't be seen by translators, the comment needs to be the line above the gettext marker, not all the way outside the conditional. Please review all the translator comments for this problem. @@ +860,3 @@ + notify_sequence_number = ippGetInteger (attr, 0); + + if (notify_sequence_number > manager->priv->last_notify_sequence_number) { Braces. @@ +901,3 @@ + job_impressions_completed = -1; + } + else if (g_strcmp0 (attr_name, "notify-subscribed-event") == 0) { else on the same line as the closing brace. @@ +904,3 @@ + notify_subscribed_event = ippGetString (attr, 0, NULL); + } + else if (g_strcmp0 (attr_name, "notify-text") == 0) { Ditto, etc. @@ +964,3 @@ + job_impressions_completed); + + if (printer_state_reasons != NULL) { No need for braces on one-line conditionals. Especially in this case where g_clear_pointer() handles NULL pointers just fine. @@ +969,3 @@ + + if (job_state_reasons != NULL) { + g_clear_pointer (&job_state_reasons, g_free); Ditto. @@ +973,3 @@ + } + + if (response != NULL) { No need for braces on one-line conditionals.
Review of attachment 249907 [details] [review]: > Get new notifications from each 60 seconds if current CUPS server is a remote one. "Get new notifications every 60 seconds if the CUPS server is a remote one." ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +53,3 @@ #define REASON_TIMEOUT 15000 #define CUPS_CONNECTION_TEST_INTERVAL 300 +#define CHECK_INTERVAL 60 Put the unit in a comment: #define CHECK_INTERVAL 60 /* secs */ @@ +1185,2 @@ renew_subscription (user_data); g_timeout_add_seconds (RENEW_INTERVAL, renew_subscription_with_connection_test, manager); This timeout doesn't save the return value, and isn't cancellable.
Review of attachment 249908 [details] [review]: ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +508,3 @@ + primary_text = g_strdup (_("Printing stopped")); + /* Translators: "print-job xy" on a printer */ + secondary_text = g_strdup_printf (_("\"%s\" on %s"), job_name, printer_name); Use a context for all those translations.
Review of attachment 249909 [details] [review]: Goodie, ignore my previous comments about this code :)
Review of attachment 249910 [details] [review]: Great, squash this into the previous patches if any of that code is new.
Created attachment 250447 [details] [review] Honor CUPS' default port number (In reply to comment #14) > Review of attachment 249905 [details] [review]: > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +1054,3 @@ > + g_socket_client_connect_to_host_async (client, > + address, > + 631, > > Any reason you don't use the ippPort() output? This patch uses the value of ippPort(). The value is not important because we already pass it through "address" string. The change clarifies the intention.
Created attachment 250448 [details] [review] Connect to message bus asynchronously (In reply to comment #14) > Review of attachment 249905 [details] [review]: > @@ +1089,3 @@ > + g_timeout_add_seconds (RENEW_INTERVAL, renew_subscription, > manager); > + > + manager->priv->cups_bus_connection = g_bus_get_sync > (G_BUS_TYPE_SYSTEM, NULL, NULL); > > Urgh. No sync please. This patch makes the connection to the system bus asynchronous.
Created attachment 250449 [details] [review] Cancel subscription to DBus signals when finished (In reply to comment #14) > Review of attachment 249905 [details] [review]: > @@ +1091,3 @@ > + manager->priv->cups_bus_connection = g_bus_get_sync > (G_BUS_TYPE_SYSTEM, NULL, NULL); > + > + g_dbus_connection_signal_subscribe > (manager->priv->cups_bus_connection, > > This needs to be cancellable as well. This patch stores the id of the subscription and remove it when stopping manager.
Created attachment 250450 [details] [review] Don't run connection test for local server This is updated version of the attachment #249905 [details].
Created attachment 250451 [details] [review] Move translator comments to appropriate positions (In reply to comment #15) > Review of attachment 249906 [details] [review]: > @@ +447,1 @@ > /* Translators: A printer has been removed */ > > This translator comment won't be seen by translators, the comment needs to be > the line above the gettext marker, not all the way outside the conditional. > > Please review all the translator comments for this problem. This patch moves those translator comments to appropriate positions.
Created attachment 250453 [details] [review] Use IPP method for getting IPP notifications (In reply to comment #15) > Review of attachment 249906 [details] [review]: > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +395,3 @@ > N_("There is a problem on printer '%s'.") }; > > + if (g_strcmp0 (notify_subscribed_event, "printer-added") != 0 && > > Why do you use different identifiers than the original code? > If you're going to translate the notification names from CUPS to signal names, > could you split it up from this patch? We are moving from using of DBus signals to CUPS' IPP method in this patch. We have to do this change in one patch or we will end up in an inconsistent state. The names of DBus signals are not the same as of those notification events. I've changed the variable names so we can easily identify attributes from which we've got information about the event. > @@ +860,3 @@ > + notify_sequence_number = ippGetInteger (attr, 0); > + > + if (notify_sequence_number > > manager->priv->last_notify_sequence_number) { > > Braces. Removed. > @@ +901,3 @@ > + job_impressions_completed = -1; > + } > + else if (g_strcmp0 (attr_name, "notify-subscribed-event") == > 0) { > > else on the same line as the closing brace. Done. > @@ +904,3 @@ > + notify_subscribed_event = ippGetString (attr, 0, > NULL); > + } > + else if (g_strcmp0 (attr_name, "notify-text") == 0) { > > Ditto, etc. Done. > @@ +964,3 @@ > + job_impressions_completed); > + > + if (printer_state_reasons != NULL) { > > No need for braces on one-line conditionals. Especially in this case where > g_clear_pointer() handles NULL pointers just fine. I removed those tests. > @@ +969,3 @@ > + > + if (job_state_reasons != NULL) { > + g_clear_pointer (&job_state_reasons, g_free); > > Ditto. > > @@ +973,3 @@ > + } > + > + if (response != NULL) { > > No need for braces on one-line conditionals. Removed.
Created attachment 250454 [details] [review] Regularly check for notifications from remote CUPS servers (In reply to comment #16) > Review of attachment 249907 [details] [review]: > > > Get new notifications from each 60 seconds if current CUPS server is a remote one. > > "Get new notifications every 60 seconds if the CUPS server is a remote one." > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +53,3 @@ > #define REASON_TIMEOUT 15000 > #define CUPS_CONNECTION_TEST_INTERVAL 300 > +#define CHECK_INTERVAL 60 > > Put the unit in a comment: > #define CHECK_INTERVAL 60 /* secs */ Done.
Created attachment 250455 [details] [review] Show final job states for remote CUPS server (In reply to comment #17) > Review of attachment 249908 [details] [review]: > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +508,3 @@ > + primary_text = g_strdup (_("Printing > stopped")); > + /* Translators: "print-job xy" on a printer */ > + secondary_text = g_strdup_printf (_("\"%s\" on > %s"), job_name, printer_name); > > Use a context for all those translations. I've added contexts to those translations and updated all *.po files.
Created attachment 250456 [details] [review] Use better debugging messages (In reply to comment #12) > Review of attachment 249903 [details] [review]: > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +1010,2 @@ > manager->priv->num_dests = cupsGetDests > (&manager->priv->dests); > + g_debug ("got dests"); > > Maybe it would be useful to differentiate the 2 debug messages? I've changed those messages so that they are more specific now.
Created attachment 250457 [details] [review] Stop renewing of CUPS subscriptions once we are finished (In reply to comment #18) > Review of attachment 249909 [details] [review]: > > Goodie, ignore my previous comments about this code :) Actually I had to modify this yet. There is another place where we create the timeout source for renewing of CUPS subscription (for local and remote servers).
Created attachment 250458 [details] [review] Coding style fixes (In reply to comment #19) > Review of attachment 249910 [details] [review]: > > Great, squash this into the previous patches if any of that code is new. Updated version of the patch.
Created attachment 250459 [details] [review] Don't use DBus recipient URI for remote servers (In reply to comment #13) > Review of attachment 249904 [details] [review]: > > Looks good. Update version of the patch (so it applies with those other patches).
Review of attachment 250447 [details] [review]: Rest looks good. ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +955,3 @@ GSocketClient *client; gchar *address; + int port = ippPort (); Please separate the declaration from the assignment.
Review of attachment 250448 [details] [review]: It would be good if you could also use a GCancellable to avoid the plugin causing problems on exit. This can be a separate patch (and a separate bug). ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +1065,3 @@ + GError *error = NULL; + + manager->priv->cups_bus_connection = g_bus_get_finish (res, &error); You're leaking error. @@ +1079,3 @@ + NULL); + } else { + g_debug ("Connection to message bus failed."); You probably want to make this fatal to the plugin. g_warning() and print the error?
Review of attachment 250449 [details] [review]: ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +1147,3 @@ manager->priv->dests = NULL; + if (manager->priv->cups_dbus_subscription_id > 0) { && manager->priv->cups_bus_connection ?
Review of attachment 250450 [details] [review]: ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +1110,2 @@ + renew_subscription (data); + if (manager->priv->renew_source_id > 0) I'd rather see this in a separate function, something like "renew_timeout_enable (manager, enable);"
Review of attachment 250451 [details] [review]: Looks good.
Review of attachment 250453 [details] [review]: Looks fine to me though a bit opaque to understand ;)
Review of attachment 250454 [details] [review]: Looks fine to commit apart from that. ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +1280,3 @@ manager->priv->cups_dbus_subscription_id = 0; manager->priv->renew_source_id = 0; + manager->priv->check_source_id = 0; That's not needed (the ones above that aren't needed either, priv is zero'ed when created).
Review of attachment 250455 [details] [review]: Translators have fallen in love!
Review of attachment 250457 [details] [review]: I think I've already reviewed that same patch above, marking as needs-work as the enabling/disabling needed splitting into a separate function.
Comment on attachment 250447 [details] [review] Honor CUPS' default port number I've separated the declaration from the assignment.
Created attachment 250548 [details] [review] Connect to message bus asynchronously (In reply to comment #33) > Review of attachment 250448 [details] [review]: > > It would be good if you could also use a GCancellable to avoid the plugin > causing problems on exit. This can be a separate patch (and a separate bug). I'll file the bug and will prepare the patch. > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +1065,3 @@ > + GError *error = NULL; > + > + manager->priv->cups_bus_connection = g_bus_get_finish (res, &error); > > You're leaking error. Fixed. > @@ +1079,3 @@ > + NULL); > + } else { > + g_debug ("Connection to message bus failed."); > > You probably want to make this fatal to the plugin. g_warning() and print the > error? The plugin does not need to be stopped because of this but yes, this should be warning and it should show the error. Fixed in the attached version of the patch.
Created attachment 250549 [details] [review] Cancel subscription to DBus signals when finished (In reply to comment #34) > Review of attachment 250449 [details] [review]: > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +1147,3 @@ > manager->priv->dests = NULL; > > + if (manager->priv->cups_dbus_subscription_id > 0) { > > && manager->priv->cups_bus_connection > ? This shouldn't be needed but I've added it there.
Review of attachment 250548 [details] [review]: Looks good
Review of attachment 250549 [details] [review]: Looks good.
Created attachment 250550 [details] [review] Stop renewing of CUPS subscriptions once we are finished (In reply to comment #40) > Review of attachment 250457 [details] [review]: > > I think I've already reviewed that same patch above, marking as needs-work as > the enabling/disabling needed splitting into a separate function. I've split the code to separate function renew_subscription_timeout_enable().
Created attachment 250551 [details] [review] Don't run connection test for local server (In reply to comment #35) > Review of attachment 250450 [details] [review]: > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +1110,2 @@ > + renew_subscription (data); > + if (manager->priv->renew_source_id > 0) > > I'd rather see this in a separate function, something like > "renew_timeout_enable (manager, enable);" Separated. I had to add one more parameter to the function (renew_subscription_timeout_enable()) since we don't want to run renew_subscription_with_connection_test() for local CUPS server.
Review of attachment 250550 [details] [review]: Looks good
Review of attachment 250551 [details] [review]: Looks good otherwise. ::: plugins/print-notifications/gsd-print-notifications-manager.c @@ +1013,3 @@ manager->priv->renew_source_id = g_timeout_add_seconds (RENEW_INTERVAL, + with_connection_test ? This is pretty ugly. Could you please separate the ternary from the function call?
Comment on attachment 250454 [details] [review] Regularly check for notifications from remote CUPS servers (In reply to comment #38) > Review of attachment 250454 [details] [review]: > > Looks fine to commit apart from that. > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +1280,3 @@ > manager->priv->cups_dbus_subscription_id = 0; > manager->priv->renew_source_id = 0; > + manager->priv->check_source_id = 0; > > That's not needed (the ones above that aren't needed either, priv is zero'ed > when created). I've removed the initialization of "check_source_id" and committed the patch.
Comment on attachment 250455 [details] [review] Show final job states for remote CUPS server (In reply to comment #39) > Review of attachment 250455 [details] [review]: > > Translators have fallen in love! :)
Comment on attachment 250551 [details] [review] Don't run connection test for local server (In reply to comment #49) > Review of attachment 250551 [details] [review]: > > Looks good otherwise. > > ::: plugins/print-notifications/gsd-print-notifications-manager.c > @@ +1013,3 @@ > manager->priv->renew_source_id = > g_timeout_add_seconds (RENEW_INTERVAL, > + with_connection_test ? > > This is pretty ugly. Could you please separate the ternary from the function > call? I've separated that. Thank you very much for all those reviews, I appreciate it. I've hoped in getting this into 3.8 originally. But it become much bigger since then. Is there any chance you would accept these patches for 3.8? Marek
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/185.