GNOME Bugzilla – Bug 761539
Use PrinterRename method from cups-pk-helper
Last modified: 2016-07-28 14:46:25 UTC
Hi, current cups-pk-helper has new method PrinterRename. Although using this method would save quite a lot of code in gnome-control-center I think that we should keep the renaming code there for one release and check whether the PrinterRename method is available. If it is available use it, if not use the old code and show a warning that in the next release they will need cups-pk-helper 0.2.6 to be able to rename printers. Regards
I was thinking about this and since we won't make this into 3.24 lets improve the code here a little. It would be beneficial to create a class PpPrinter on which we could call some methods, e.g. the rename. The only parameter to its constructor would be just printer name. It won't be for storing/caching information it will be for modification of the object or getting informations about it. So for the renaming we would have new functions pp_printer_rename_async () and pp_printer_rename_finish(). Let's just not collide with Felipe's bug #748336. Add just the renaming and its usage for now.
Created attachment 325770 [details] [review] Use PrinterRename from cups-pk-helper
Review of attachment 325770 [details] [review]: Unfortunately, the patch doesn't compile here. Also, separate it to 2 patches. One which will add the class and second one which will use it. Use G_DECLARE_FINAL_TYPE there so the code is smaller. And add license to the new files.
Created attachment 328117 [details] [review] Introduce printer class
Created attachment 328118 [details] [review] Use of printer class
Review of attachment 328117 [details] [review]: ::: panels/printers/pp-printer.c @@ +1,1 @@ +#include "pp-printer.h" add license @@ +3,3 @@ +typedef struct _PpPrinterPrivate PpPrinterPrivate; + +struct _PpPrinter{ place the opening bracket to new line @@ +5,3 @@ +struct _PpPrinter{ + GObject parent_instance; + gpointer priv; declare the priv as "PpPrinterPrivate *priv" so you can use printer->priv @@ +10,3 @@ +struct _PpPrinterClass { + GObjectClass parent_class; +}; you don't need this declaration @@ +14,3 @@ +struct _PpPrinterPrivate +{ + gchar *printer_name; make this a proper property and add pp_printer_{get/set}_property() functions @@ +40,3 @@ + GObjectClass *gobject_class = G_OBJECT_CLASS (klass); + + g_type_class_add_private (klass, sizeof (PpPrinterPrivate)); you don't need this because you used G_DEFINE_TYPE_WITH_PRIVATE @@ +49,3 @@ +pp_printer_init (PpPrinter *printer) +{ + printer->priv = G_TYPE_INSTANCE_GET_PRIVATE (printer, indent this by just 2 spaces @@ +54,3 @@ +} + +PpPrinter* put a space between the type and "*" and do it also for the rest of the code in the patch @@ +60,3 @@ + + pp_printer_init (self); + ((PpPrinterPrivate *) pp_printer_get_instance_private (self))->printer_name = g_strdup(name); you will be able to initialize the name in the g_object_new() once you implement the name as a property @@ +68,3 @@ +pp_printer_get_name (PpPrinter *printer) +{ + return ((PpPrinterPrivate *) pp_printer_get_instance_private (printer))->printer_name; you can use self->priv once you declare the priv as PpPrinterPrivate @@ +86,3 @@ + bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &error); + g_object_set_data_full ((GObject *) bus, "pp_printer_old_name", priv->printer_name, g_free); + g_object_set_data_full ((GObject *) bus, "pp_printer_new_name", (gpointer) new_name, g_free); this will free the pointer passed in as a parameter (and causes a crash) ::: panels/printers/pp-printer.h @@ +1,1 @@ +#ifndef __PP_PRINTER_H__ add license @@ +9,3 @@ +#define PP_TYPE_PRINTER (pp_printer_get_type ()) + +#define MECHANISM_BUS "org.opensuse.CupsPkHelper.Mechanism" move this to the pp-printer.c file @@ +17,3 @@ + +gchar *pp_printer_get_name (PpPrinter *printer); +gboolean pp_printer_rename_async (PpPrinter *printer, these initiation functions don't return anything usually @@ +22,3 @@ + gpointer user_data); + +GVariant *pp_printer_call_finish (GDBusConnection *connection, base the renaming functions on GTask so we don't need to care about some GDBusConnection in finish function the finishing function should be named pp_printer_call_finish() @@ +25,3 @@ + GAsyncResult *res, + GError **error); + align names of the parameters of the functions and their left bracket and keep 1 line between the functions declarations
Review of attachment 328118 [details] [review]: Since the PpPrinter class will change, I'm marking this as needs-work. ::: panels/printers/Makefile.am @@ +51,3 @@ + cc-printers-panel.h \ + pp-printer.c \ + pp-printer.h this belongs to the patch which adds the class ::: panels/printers/cc-printers-panel.c @@ +1935,3 @@ if (printer_name && printer_delete (printer_name)) actualize_printers_list (self); } add new line here @@ +1936,3 @@ actualize_printers_list (self); } +void make this "static void" @@ +1952,3 @@ + priv = PRINTERS_PANEL_PRIVATE (self); + + bus = G_DBUS_CONNECTION(source_object); add a space before "(" and do the same for the rest of the code in the patches
Created attachment 329844 [details] [review] Introduce printer class
Created attachment 329845 [details] [review] Use of printer class
Review of attachment 329844 [details] [review]: Thank you for the patch. I have just 3 comments on it. Once you address them, feel free to push this to master. ::: panels/printers/pp-printer.c @@ +23,3 @@ +#include "pp-utils.h" + +#define MECHANISM_BUS "org.opensuse.CupsPkHelper.Mechanism" This is already defined in pp-utils.h so no need to have it here. @@ +130,3 @@ + +PpPrinter * +pp_printer_new (gchar *name) "const gchar *" would be better. @@ +281,3 @@ + g_return_val_if_fail (g_task_is_valid (res, printer), FALSE); + + return g_task_propagate_boolean (G_TASK (res), error); We should unref the GTask before the return. It leaks otherwise.
(In reply to Marek Kašík from comment #10) > Review of attachment 329844 [details] [review] [review]: > > Thank you for the patch. I have just 3 comments on it. Once you address > them, feel free to push this to master. > > ::: panels/printers/pp-printer.c > @@ +23,3 @@ > +#include "pp-utils.h" > + > +#define MECHANISM_BUS "org.opensuse.CupsPkHelper.Mechanism" > > This is already defined in pp-utils.h so no need to have it here. > > @@ +130,3 @@ > + > +PpPrinter * > +pp_printer_new (gchar *name) > > "const gchar *" would be better. > > @@ +281,3 @@ > + g_return_val_if_fail (g_task_is_valid (res, printer), FALSE); > + > + return g_task_propagate_boolean (G_TASK (res), error); > > We should unref the GTask before the return. It leaks otherwise. One more thing, you need to free the strings got by g_object_get() so we don't leak there.
Review of attachment 329845 [details] [review]: Once you address the comment, feel free to push the patch to master. Thanks. ::: panels/printers/cc-printers-panel.c @@ +1996,3 @@ + printer_rename_cb, + self); + g_object_unref (printer); Please remove this unref since we need to unref the printer just in the callback. Thanks.
The "show_jobs_dialog_button" should be put into a sizegroup together with gear menu button so that they have the same height. Also the "show_jobs_dialog_button" is sensitive/visible even if there are no jobs. I guess that it shouldn't be. Also subscriptions are not probably working since sending a job to a printer doesn't update its state when already selected. Another thing is that the switch buttons were removed. I think that sometimes it would be handy to be able to make printer to reject jobs.
(In reply to Marek Kašík from comment #13) > The "show_jobs_dialog_button" should be put into a sizegroup together with > gear menu button so that they have the same height. > Also the "show_jobs_dialog_button" is sensitive/visible even if there are no > jobs. I guess that it shouldn't be. > Also subscriptions are not probably working since sending a job to a printer > doesn't update its state when already selected. > > Another thing is that the switch buttons were removed. I think that > sometimes it would be handy to be able to make printer to reject jobs. Wrong bug...
Created attachment 330063 [details] [review] Introduce printer class
Created attachment 330064 [details] [review] Use of printer class
Review of attachment 330063 [details] [review]: Thank you for the changes. I've pushed the patches with a little tweak yet.
Review of attachment 330064 [details] [review]: Pushed to master.
I guess this class should encapsulate all the options of a printer object[0]. Marek, if you agree, I would like to reopen this bug to add new properties to the class. In doing so, it can be used in all instances which we have been accessing cups_dest_t. [0] https://tools.ietf.org/html/rfc2911
(In reply to Felipe Borges from comment #19) > I guess this class should encapsulate all the options of a printer > object[0]. > > Marek, if you agree, I would like to reopen this bug to add new properties > to the class. In doing so, it can be used in all instances which we have > been accessing cups_dest_t. > > [0] https://tools.ietf.org/html/rfc2911 The PpPrinter class is meant to be a class which performs actions on the queue on CUPS server. I originally didn't thought about it as a full-fledged class where changing an option would update it immediately on CUPS server and vice versa (they change so the object could be out-of-date). If you would want to change the PpPrinter to represent the CUPS's queue fully, you would also need to carefully listen to CUPS events and update corresponding properties (and vice versa). So I would prefer to add new async methods to this class (even simple getters/setters need to be async here) instead of properties. If you want to add new methods to this class then create a new bug please (no need to reopen this one).