GNOME Bugzilla – Bug 767600
Introduce the new Printers panel
Last modified: 2017-02-13 18:47:56 UTC
This changes are part of the effort of evolving the current Printers panel into the redesign specified at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines My work towards the complete implementation of this redesign is available at https://git.gnome.org/browse/gnome-control-center/log/?h=wip/feborges/new-printers-panel (be aware that this being an wip branch, it might be subject to force-pushes)
Created attachment 329670 [details] [review] printers: use panel-wide page for empty-state Instead of having the empty-state and no-cups-page states displayed as tabs of the printer notebook, go for panel-wide pages, which better highlight these states and doesn't unnecessarily shows the empty printers list. This change is also part of the effort of the panel redesign, according to the mockups at https://wiki.gnome.org/Design/SystemSettings/Printers
Created attachment 329671 [details] [review] printers: enforce the empty-state patterns
Created attachment 329672 [details] [review] printers: present spinner while populating the panel
Created attachment 329673 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog. - impossibility of changing default printer
Created attachment 329674 [details] [review] printers: introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
The panel is opening and closing quite a lot of threads when it is running. I've backtraced this to the function set_as_default_printer() of the PpPrinterEntry. It is called repetitively probably because of the connection on "toggled" signal of the "printer_default_checkbutton" check button (if I remove the connection, it doesn't call it). Btw, the panel doesn't exhibit this behaviour every time for me.
You should swap these two branches in the update_sensitivity() in the fourth patch. I see the "no printers" page because of that: if (already_present_local && !is_class && !priv->getting_ppd_names) gtk_stack_set_visible_child_name (GTK_STACK (widget), "empty-state"); else gtk_stack_set_visible_child_name (GTK_STACK (widget), "printers-list");
Review of attachment 329670 [details] [review]: This patch looks good. Thanks.
Review of attachment 329671 [details] [review]: This one too.
Attachment 329670 [details] pushed as 11dd608 - printers: use panel-wide page for empty-state Attachment 329671 [details] pushed as 8c71b0b - printers: enforce the empty-state patterns
Review of attachment 329672 [details] [review]: This one looks good too.
Comment on attachment 329672 [details] [review] printers: present spinner while populating the panel Attachment 329672 [details] pushed as 93f71e5 - printers: present spinner while populating the panel
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.
Created attachment 335081 [details] [review] printers: Move "Add Printer" button to header bar This patch introduces a change to the Lock/Unlock logic. From now on, unlocking the panel causes the "Lock" button to turn into the "Add Printer" button.
(In reply to Felipe Borges from comment #14) > Created attachment 335081 [details] [review] [review] > printers: Move "Add Printer" button to header bar > > This patch introduces a change to the Lock/Unlock logic. From now > on, unlocking the panel causes the "Lock" button to turn into the > "Add Printer" button. This is a design pattern previously discussed with designers. It is also now present on the "User Accounts" panel (see eb9c110)
Created attachment 335926 [details] [review] printers: Introduce PpPrinterEntry widget This commit introduces the following regressions: * no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog. * no way of setting default printer
Review of attachment 335081 [details] [review]: Thank you for the patch. It works well for me. I have just few nitpicks. Commit it once we agree on all the other patches please, not now and we also have to wait until 3.22 branch is split from master. ::: panels/printers/cc-printers-panel.c @@ +121,3 @@ +#define PAGE_LOCK "_lock" +#define PAGE_ADDPRINTER "_addprinter" Could we use CHILD instead of PAGE here? @@ +176,3 @@ + cc_shell_embed_widget_in_header (shell, priv->headerbar_buttons); + + button = (GtkLockButton*) Use GTK_LOCK_BUTTON() please. @@ +3137,3 @@ + widget = (GtkWidget*) + gtk_builder_get_object (priv->builder, "headerbar-buttons"); + priv->headerbar_buttons = widget; You can assign the object to the 'priv->headerbar_buttons' directly without using 'widget'. Also, lets use GTK_WIDGET() macro here (and in all newly written code).
Comment on attachment 335081 [details] [review] printers: Move "Add Printer" button to header bar Attachment 335081 [details] pushed as cf99ceb - printers: Move "Add Printer" button to header bar (In reply to Marek Kašík from comment #17) > Review of attachment 335081 [details] [review] [review]: > > Thank you for the patch. It works well for me. I have just few nitpicks. > Commit it once we agree on all the other patches please, not now and we also > have to wait until 3.22 branch is split from master. Thanks for your review. Bastien allowed me to branch 3.22 and commit these changes to master.
(In reply to Felipe Borges from comment #16) > Created attachment 335926 [details] [review] [review] > printers: Introduce PpPrinterEntry widget > > This commit introduces the following regressions: > * no possibility of renaming properties such as printer names, > location, or changing model/driver. This issue is going to be > solved nextly by the introduction of the PpDetailsDialog. > * no way of setting default printer I'll wait for the next patch then.
Created attachment 344024 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 344128 [details] [review] printers: Introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
Created attachment 344130 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 344218 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 344220 [details] [review] printers: Introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
Created attachment 344221 [details] [review] printers: Make the printers panel a single column This patch purges all the former TreeView machinery and makes the Printers panel have the printers listed in a scrolled window, as designed at https://wiki.gnome.org/Design/SystemSettings/Printers
Created attachment 344223 [details] [review] printers: Set min-content-height on the scrolled window Set a minimum content height of 490px for the panel when the allocated height is smaller than 490px. 490 is an estimated value for the panels to properly fit on netbook screens. See https://wiki.gnome.org/Design/SystemSettings#Notes
Thank you for the patches! I have several comments on the functionality: When I run the dialog it get into an infinite loop because of setting state of the checkbox button for default printer. I had to comment out the code in set_as_default_printer() to be able to continue. I use 1.2 scaling of fonts on my machine, this causes the button with icon for settings to be smaller than the button with number of paused jobs. The printer panel is small at the beginning (300px height) and resize once I hit scrollbars to the 490px. The ink level is out of center (the colors bar is up a little). On the mockup there is a gray box in the place of ink level if we don't have information about ink level. When do you load the database of PPDs? It started when the dialog was opened before so the user didn't need to wait. Maybe loading it after unlock would be enough. Second click on "Select From Database" never ends. When I press "Search for Drivers" and close the dialog the g-c-c crashes. Once a printer is removed the list is not updated. I have all printer twice on the list (and 9x once I tried to rename a printer). Clicking on the address could try to specify port returned by ippPort() so the web page of the server is opened. Rename of printer and change of location have to respond to closing of the dialog too otherwise the change is not done. I have a lot of active buttons with "No Active Jobs" here and just one is inactive. The button with number of paused jobs shows me that there are 5 paused jobs even if there are 3 paused ones and 2 stopped ones. Number of actual jobs doesn't update automatically on the button in the printer widget neither in jobs dialog. I see that the mockup has description field on it. Maybe we could add it (but remember that this is a general text, it can be long). Do you plan to keep there the "Add Printer" (there is just "Add" in the mockup)? Do you plan to redesign the "Printing Options" dialog in this cycle? Could you address the issues?
Created attachment 345181 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 345182 [details] [review] printers: Introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
Created attachment 345183 [details] [review] printers: Make the printers panel a single column This patch purges all the former TreeView machinery and makes the Printers panel have the printers listed in a scrolled window, as designed at https://wiki.gnome.org/Design/SystemSettings/Printers
Created attachment 345184 [details] [review] printers: Set min-content-height on the scrolled window Set a minimum content height of 490px for the panel when the allocated height is smaller than 490px. 490 is an estimated value for the panels to properly fit on netbook screens. See https://wiki.gnome.org/Design/SystemSettings#Notes
(In reply to Marek Kašík from comment #27) > I see that the mockup has description field on it. Maybe we could add it > (but remember that this is a general text, it can be long). I discussed this with Allan Day in the past and we decided to go with the "sanitized" printer model name instead. > Do you plan to redesign the "Printing Options" dialog in this cycle? It is doable. Quite simple changes according to the mockups.
Created attachment 345225 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 345226 [details] [review] printers: Introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
Created attachment 345227 [details] [review] printers: Make the printers panel a single column This patch purges all the former TreeView machinery and makes the Printers panel have the printers listed in a scrolled window, as designed at https://wiki.gnome.org/Design/SystemSettings/Printers
Created attachment 345228 [details] [review] printers: Set min-content-height on the scrolled window Set a minimum content height of 490px for the panel when the allocated height is smaller than 490px. 490 is an estimated value for the panels to properly fit on netbook screens. See https://wiki.gnome.org/Design/SystemSettings#Notes
Review of attachment 345225 [details] [review]: Thank you for the patch. The design looks great! I have several comments on the code. Btw, when I clear all jobs and close the dialog, the panel crashes. ::: panels/printers/pp-printer-entry.c @@ +124,3 @@ + int i; + + tmp = g_ascii_strdown (printer_make_and_model, -1); We are leaking the tmp here. @@ +225,3 @@ + display_value = value / 100.0 * (width - 3.0); + gdk_cairo_set_source_rgba (cr, &color); + cairo_rectangle (cr, 3.5, 3.5, display_value, SUPPLY_BAR_HEIGHT - 3.0); I'm sorry for nitpicking but could you use spaces here instead of the tabs? (this is the only line with them in this function) @@ +256,3 @@ + gtk_style_context_restore (context); + + // free inkLevelData This is a C++ comment. Maybe you should actually free the data :) sometime. @@ +319,3 @@ + gint num_jobs; + + // FIXME do this async A C++ comment. @@ +327,3 @@ + + if (self->num_jobs <= 0) + button_label = g_strdup ("No Active Jobs"); Please add comment for translators and mark the string for translation. @@ +329,3 @@ + button_label = g_strdup ("No Active Jobs"); + else + button_label = g_strdup_printf ("%u Jobs", num_jobs); Please add comment for translators and mark the string for translation (g_strdup_printf (ngettext ("%u Jobs", "%u Jobs", num_jobs), num_jobs);). Also fix indentation please. @@ +332,3 @@ + + gtk_button_set_label (GTK_BUTTON (self->show_jobs_dialog_button), button_label); + gtk_widget_set_sensitive (self->show_jobs_dialog_button, self->is_accepting_jobs && num_jobs > 0); Do we need to check whether the printer is accepting jobs for the jobs which were already accepted? Ok, I've just tested it and you can cancel/resume print jobs even if the printer is rejecting jobs. @@ +420,3 @@ + } + } + I'm missing the printer-state-reasons here. This should be here for the error/warning/report reporting when showing the "Restart" button. Btw, where is it? @@ +441,3 @@ + case 5: + /* Translators: Printer's state (no jobs can be processed) */ + printer_status = g_strdup ( C_("printer state", "Stopped")); You are leaking the printer_status. @@ +450,3 @@ + printer_icon_name = g_strdup ("printer-network"); + + self->printer_name = printer.name; Maybe you wanted to use the "instance" here or at least free it somewhere. @@ +453,3 @@ + self->is_accepting_jobs = is_accepting_jobs; + self->printer_make_and_model = printer_make_and_model; + self->printer_location = location; Please strdup these 2 strings, they are not meant to persist (and free them in dispose). @@ +456,3 @@ + self->is_authorized = is_authorized; + + self->printer_hostname = printer_get_hostname (printer_type, self->printer_uri, printer_uri); You don't free this in dispose. @@ +467,3 @@ + self->printer_make_and_model = sanitize_printer_model (printer_make_and_model); + + if (self->printer_make_and_model == NULL) Lets check also for emptiness. @@ +473,3 @@ + } + + gtk_label_set_text (self->printer_model, self->printer_make_and_model); It would be good to do this in else branch of the if(). @@ +475,3 @@ + gtk_label_set_text (self->printer_model, self->printer_make_and_model); + + if (location[0] == '\0') Better check for NULL here. @@ +481,3 @@ + } + + gtk_label_set_text (self->printer_location_address_label, location); It would be good to do this in else branch of the if(). @@ +491,3 @@ + + self->pp_options_dialog = NULL; + self->pp_jobs_dialog = NULL; Do we need to set members of the structure to NULL? ::: panels/printers/pp-printer-entry.h @@ +36,3 @@ + gchar *marker_colors; + gchar *marker_types; +} InkLevelData; I think that this doesn't need to be here.
Review of attachment 345226 [details] [review]: Thank you for the dialog. I have several comments on it: The dialog crashes when I open the dialog, rename a printer, press tab and close the dialog. Manual install of PPD does not update the dialog. The separator is missing in the upper right corner on the bar. Please add comments for translators and check pointers against NULL please. ::: panels/printers/details-dialog.ui @@ +18,3 @@ + <property name="halign">center</property> + <child> + <object class="GtkGrid"> I think that there is one redundant row in this grid. ::: panels/printers/pp-details-dialog.c @@ +53,3 @@ + gchar *ppd_file_name; + PPDList *all_ppds_list; + GHashTable *preferred_drivers; I think that this hash table is obsolete now since you always search for the drivers in this new dialog and free the list when destroying it. @@ +97,3 @@ + name = gtk_entry_get_text (GTK_ENTRY (self->printer_name_entry)); + + title = g_strdup_printf ("%s Details", name); Mark this for translation (with context as in the init) and add a comment for translators. @@ +142,3 @@ +} + +static void set_ppd_cb (gchar *printer_name, gboolean success, gpointer user_data); Move this declaration above the _PpDetailsDialog struct definition please to have them on one (common) place. @@ +206,3 @@ + self->get_ppd_names_cancellable = g_cancellable_new (); + get_ppd_names_async (self->printer_name, + 1, I'm little sad that user can not select the driver out of 3 found now but it is probably better :(. @@ +240,3 @@ + GCancellable *cancellable; + + cancellable = g_cancellable_new (); If you don't store the cancellable anywhere then it doesn't need to be used at all. @@ +248,3 @@ + self); + + self->ppd_file_name = g_strdup (ppd_name); Aren't you leaking memory in the self->ppd_file_name here? @@ +271,3 @@ + + g_object_unref (self->get_all_ppds_cancellable); + self->get_all_ppds_cancellable = NULL; Use g_clear_object() here please. @@ +281,3 @@ + gchar *manufacturer = NULL; + + self->ppd_file_name = g_strdup (cupsGetPPD (self->printer_name)); Aren't you leaking memory here? @@ +306,3 @@ + { + manufacturer = g_strdup ("Raw"); + } Fix indentation please. @@ +361,3 @@ + GCancellable *cancellable; + + cancellable = g_cancellable_new (); This cancellable is not used actually. @@ +425,3 @@ + self->ppd_file_name = NULL; + + title = g_strdup_printf (C_("Printer Details dialog title", "%s Details"), printer_name); This needs comment for translators and a g_free() later. @@ +447,3 @@ +{ + g_free (self->printer_name); + self->printer_name = NULL; Use g_clear_pointer here please. @@ +450,3 @@ + + g_free (self->printer_location); + self->printer_location = NULL; Use g_clear_pointer here please.
Looking at the other patch. You still acquire the list of all PPDs in the Printers panel. Could you propagate the list to the PpPrinterEntry and then to the PpDetailsDialog once it is available? (just simple functions like pp_printer_entry_set_ppd_list() and pp_details_dialog_set_ppd_list() would be enough)
Review of attachment 345227 [details] [review]: This patch looks good to me. There is just one thing it remembered to me. You are actualizing the list of printers even if just a state of a printer changed or a number of jobs for a particular printer has changed. This could be easily optimized by just getting name of the printer for which a change happened and refreshing it.
Review of attachment 345227 [details] [review]: Actually, I forgot to ask you to remove the #include of "pp-maintenance-command.h" and of "pp-job.h" from cc-printers-panel.c, these are not needed there. Remove those when you are committing this please.
Review of attachment 345228 [details] [review]: ::: panels/printers/cc-printers-panel.c @@ +872,3 @@ + widget = (GtkWidget*) + gtk_builder_get_object (priv->builder, "scrolled-window"); + gtk_scrolled_window_set_min_content_height (GTK_SCROLLED_WINDOW (widget), SCROLL_HEIGHT); The same effect can be achieved by setting this directly in the UI file. Btw, I still see horizontal scrollbar.
Created attachment 345628 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 345633 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 345634 [details] [review] printers: Introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
Created attachment 345635 [details] [review] printers: Make the printers panel a single column This patch purges all the former TreeView machinery and makes the Printers panel have the printers listed in a scrolled window, as designed at https://wiki.gnome.org/Design/SystemSettings/Printers
Created attachment 345636 [details] [review] printers: Set min-content-height on the scrolled window Set a minimum content height of 490px for the panel when the allocated height is smaller than 490px. 490 is an estimated value for the panels to properly fit on netbook screens. See https://wiki.gnome.org/Design/SystemSettings#Notes
Review of attachment 345633 [details] [review]: Thank you for the changes. Please show the restart button only when the printer is stopped or does not accept jobs.
Review of attachment 345634 [details] [review]: The dialog crashes when I rename printer and quickly close the detail dialog. It also crashes when I press "Search for Drivers" and close the detail dialog.
Created attachment 345652 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 345658 [details] [review] printers: introduce PpPrinterEntry widget This commit introduces the following regressions: - no possibility of renaming properties such as printer names, location, or changing model/driver. This issue is going to be solved nextly by the introduction of the PpDetailsDialog.
Created attachment 345659 [details] [review] printers: Introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
Created attachment 345660 [details] [review] printers: Introduce PpDetailsDialog This dialog handles the editing of printer properties such as name, location, automatic discovery of driver, manual selection of printer driver, and manual selection of ppd file.
Review of attachment 345658 [details] [review]: Thank you for the changes. Please push this patch to master branch.
Review of attachment 345660 [details] [review]: This one too. Thanks.
Thanks for the reviews Marek, lets get to the other issues in other bugs. I'm closing this one. Attachment 345635 [details] pushed as 378e041 - printers: Make the printers panel a single column Attachment 345636 [details] pushed as ae17ba1 - printers: Set min-content-height on the scrolled window Attachment 345658 [details] pushed as 37e3796 - printers: introduce PpPrinterEntry widget Attachment 345660 [details] pushed as 7e3d89e - printers: Introduce PpDetailsDialog