GNOME Bugzilla – Bug 779656
Search for printer
Last modified: 2017-05-10 09:06:12 UTC
Since we have received some feedback/complains related to the fact that it might be harder with the new design to find a printer given a long list of devices (say an office).
Created attachment 347320 [details] [review] printers: Make printers-list a GtkListBox By making the printers list a GtkListBox instead of a GtkBox, we can use listbox's capabilities for sorting and filter.
Created attachment 347321 [details] [review] printers: Add search capabilities to the panel Due to the recent changes towards the new design, it became slightly harder to find a printer given a long list of entries. This patch introduces search capabilities to the panel, filtering based on the printer name.
Review of attachment 347320 [details] [review]: The panel has a black border now. See the attached screenshot.
Created attachment 347852 [details] screenshot
Review of attachment 347321 [details] [review]: Thank you for the patch. We should coordinate this with new printer dialog where we search also through location of printer which is useful if you are looking for a printer in specific location/floor. ::: panels/printers/cc-printers-panel.c @@ +610,3 @@ self); + g_object_set_data (G_OBJECT (printer_entry), "printer", printer.name); Instead of this, add property "printer-name" to the PpPrinterEntry please. @@ +1074,2 @@ cups = pp_cups_new (); pp_cups_connection_test_async (cups, connection_test_cb, self); This line removal doesn't belong to this patch.
(In reply to Marek Kašík from comment #3) > Review of attachment 347320 [details] [review] [review]: > > The panel has a black border now. See the attached screenshot. Also, the the parent instance in _PpPrinterEntry should be changed to GtkListBoxRow.
Created attachment 347909 [details] [review] printers: Make printers-list a GtkListBox By making the printers list a GtkListBox instead of a GtkBox, we can use listbox's capabilities for sorting and filter.
Created attachment 347910 [details] [review] printers: Add search capabilities to the panel Due to the recent changes towards the new design, it became slightly harder to find a printer given a long list of entries. This patch introduces search capabilities to the panel, filtering based on the printer name.
Created attachment 347911 [details] [review] printers: Filter the printer-location in the search Show printer search results where the searched string matches the printer-location.
Can we get this for 3.26?
Yes, we can. I'll look at it this week.
I've looked at the first patch here and I think that we should use the margins there but due to the black-margin bug we can't. I'll look whether I can fix it in Gtk+ next week (see #773459). (I'm sorry, I hadn't much time due to handling of some security bugs this week)
Hi, I just built gtk+ from git master today (HEAD at 2c174319) and the black margins are no longer presented in GtkListBox for me. Can you still reproduce it?
(In reply to Felipe Borges from comment #13) > Hi, I just built gtk+ from git master today (HEAD at 2c174319) and the black > margins are no longer presented in GtkListBox for me. Can you still > reproduce it? Current gnome-control-center can not build with current gtk+. What version of gtk3 do you have?
(In reply to Marek Kašík from comment #14) > Current gnome-control-center can not build with current gtk+. What version > of gtk3 do you have? 3.22.12
gtk+ git master is GTK+ 4. Did you mean the gtk+ 3.x branch?
Created attachment 350971 [details] screenshot2 (In reply to Bastien Nocera from comment #16) > gtk+ git master is GTK+ 4. Did you mean the gtk+ 3.x branch? my g-c-c is on Gtk+ 3.22.12, and I no longer see the dark borders.
Maybe I was not precise enough. I'm testing again the first version of patch which changed the GtkBox to GtkListBox which has margins defined. These margins are black for me in current gtk 3.22 git and the system one gtk3 3.22.12 too (both wayland and X). If we don't use the margins there we can not get the same look as with the GtkBox.
(In reply to Marek Kašík from comment #18) > Maybe I was not precise enough. I'm testing again the first version of patch > which changed the GtkBox to GtkListBox which has margins defined. These > margins are black for me in current gtk 3.22 git and the system one gtk3 > 3.22.12 too (both wayland and X). sure. I acknowledge the existence of this bug. > If we don't use the margins there we can not get the same look as with the > GtkBox. The newest version of the patch sets the margins in the rows instead. This way there's no black border. So Bug 773459 doesn't affect us here.
Review of attachment 347909 [details] [review]: (In reply to Felipe Borges from comment #19) > (In reply to Marek Kašík from comment #18) > > If we don't use the margins there we can not get the same look as with the > > GtkBox. > > The newest version of the patch sets the margins in the rows instead. This > way there's no black border. So Bug 773459 doesn't affect us here. The problem here is margin-top and margin-bottom which we would need. Originally, the borders + margins were: 30 pixels at the top and at the bottom 60 pixels on the left and on the right 20 pixels between entries with your patch we have: 20 pixels at the top and at the bottom 50 pixels on the left and on the right 40 pixels between entries Lets set it to (until the bug is fixed or we port to Gtk+-4.0): 10 pixels at the top and at the bottom 60 pixels on the left and on the right 20 pixels between entries ( = ~10 pixels entry's margin) ::: panels/printers/printer-entry.ui @@ +64,2 @@ <property name="valign">center</property> + <property name="margin">20</property> Original spacing between those entries was 20 pixels. Use 10 pixels here then. @@ +65,3 @@ + <property name="margin">20</property> + <property name="margin-start">50</property> + <property name="margin-end">50</property> Original border + margin was 60 pixels. Use it please.
Review of attachment 347910 [details] [review]: This patch looks good except those few comments. ::: panels/printers/cc-printers-panel.c @@ +610,3 @@ self); + g_object_set (G_OBJECT (printer_entry), "printer-name", g_strdup (printer.name), NULL); You've already set the printer-name property in pp_printer_entry_new(). @@ +939,3 @@ + return TRUE; + + g_object_get (G_OBJECT (row), "printer-name", &printer_name, NULL); You have to free the printer_name string here. ::: panels/printers/pp-printer-entry.c @@ +102,3 @@ + switch (prop_id) + { + case PROP_PRINTER_NAME: Indent this block by another 2 spaces please. @@ +120,3 @@ + switch (prop_id) + { + case PROP_PRINTER_NAME: ditto
Also, remove please the line "self->printer_name = g_strdup (printer.name);" from pp_printer_entry_new(). It is not needed once the "printer-name" is set.
Review of attachment 347911 [details] [review]: I have just 2 comments on this patch. ::: panels/printers/cc-printers-panel.c @@ +948,2 @@ name = cc_util_normalize_casefold_and_unaccent (printer_name); + location = cc_util_normalize_casefold_and_unaccent (printer_location); Free this when not needed. @@ +950,3 @@ search = cc_util_normalize_casefold_and_unaccent (gtk_entry_get_text (GTK_ENTRY (search_entry))); + retval = strstr (name, search) != NULL || strstr (location, search) != NULL; Please set the property "printer-location" inside pp_printer_entry_new() and remove the line "self->printer_location = g_strdup (location);" from there. And handle situation when location is NULL here.
Created attachment 351436 [details] [review] printers: Make printers-list a GtkListBox By making the printers list a GtkListBox instead of a GtkBox, we can use listbox's capabilities for sorting and filter.
Created attachment 351437 [details] [review] printers: Add search capabilities to the panel Due to the recent changes towards the new design, it became slightly harder to find a printer given a long list of entries. This patch introduces search capabilities to the panel, filtering based on the printer name.
Created attachment 351438 [details] [review] printers: Filter the printer-location in the search Show printer search results where the searched string matches the printer-location.
Review of attachment 351436 [details] [review]: Thank you for the change. It looks good to me.
Review of attachment 351437 [details] [review]: Thank you for the changes. Please push it to master.
Review of attachment 351438 [details] [review]: Thank you for working on this.
Thanks for the reviews! Attachment 351436 [details] pushed as e26756d - printers: Make printers-list a GtkListBox Attachment 351437 [details] pushed as 34aedbe - printers: Add search capabilities to the panel Attachment 351438 [details] pushed as 870b4e7 - printers: Filter the printer-location in the search