GNOME Bugzilla – Bug 678637
printers: New features for Printers panel
Last modified: 2012-09-02 21:51:50 UTC
Created attachment 217031 [details] [review] implement new features of Printers panel On https://live.gnome.org/ThreePointFive/Features/PrinterPanelImprovements and https://live.gnome.org/Design/SystemSettings/Printers are listed some improvements which improves user experience with Printers panel. These improvements includes: 1) Rework of "New printer dialog" 2) Add installable options and all other options to the "Options dialog" 3) Possibility to change printer driver of already installed printer Attached patch implements these 3 features but it is not finished yet, so use it just for testing purposes. Marek
Hey Marek, a few comments after your demo yesterday. First of all, this looks really good already. 1. The context menu where you allow to select different drivers had some fairly unseemly strings in it (like paths, or something) - We should try really hard to avoid that and instead show some more meaningful label or title, if the ppds contain something that can be used for this. 2. The search entry should use GtkSearchEntry, now that we have it. 3. The sizing of the entry was a bit odd, being centered instead of full-width. Eventually, putting the spinner like an icon in the entry would be nice, but that needs gtk work first. 4.The heuristics for treating the entry contents as either server address or as refining search terms was a little wierd. Not sure if we want to move the 'connect to server' functionality somewhere else ?
jrb was also commenting on the tabs-on-the-side in the options dialog - is that what the designers want there, or should we rather look at doing a real sidebar ?
Lastly, it would be good to break this into separate patches for individual features.
Hey, (In reply to comment #1) > Hey Marek, a few comments after your demo yesterday. > > First of all, this looks really good already. Thanks. > 1. The context menu where you allow to select different drivers had some > fairly unseemly strings in it (like paths, or something) - We should try > really hard to avoid that and instead show some more meaningful label or > title, if the ppds contain something that can be used for this. I plan to use printer-make-and-model attribute of those PPDs or maybe ModelName entry from PPD (this would need to actually read the PPDs). > 2. The search entry should use GtkSearchEntry, now that we have it. OK, will look at it. > 3. The sizing of the entry was a bit odd, being centered instead of > full-width. Eventually, putting the spinner like an icon in the entry would > be nice, but that needs gtk work first. This was done according to the design. I'll look at the possibility of the embedded spinner when it is available. > 4.The heuristics for treating the entry contents as either server address or > as refining search terms was a little wierd. Not sure if we want to move the > 'connect to server' functionality somewhere else ? It shouldn't be difficult to change but it would be good to get some feedback from designers. (In reply to comment #2) > jrb was also commenting on the tabs-on-the-side in the options dialog - is > that what the designers want there, or should we rather look at doing a real > sidebar ? It seems to me now that I misunderstood the original design. I'll change it. (In reply to comment #3) > Lastly, it would be good to break this into separate patches for individual > features. I have it actually in 3 different directories but I've merged it to the 1 patch because it needs manual application of some parts if applied separately. I'll attach those features separated to individual patches when they will be ready. Regards Marek
Created attachment 217527 [details] [review] allow users to change printer's PPD Hi, this patch contains the part which allows users to change printer's PPD. It adds a popup menu to the Model field. The popup menu contains 3 choices: 1) list of best PPD files for the device (it takes time to create it) 2) select PPD from database of installed ones 3) select PPD on filesystem All needed operations are asynchronous so it doesn't block ui. Regards Marek
Ok, I've tried this. The async loading is much better. A few issues remain: - Even with async being much better, we should not go through the same wait twice. Imo, the panel should keep the ppd db information around until it gets unloaded. As things are now, if I dismiss the dialog and reopen it right away, I get to wait again - "Getting device informations..." is still wierd. What is that doing in the menu, and why is it insensitive ? I think it should just go away ? - The spacing in the dialog is not quite right. The margins are too narrow, and the buttons don't line up with the right edge of the list. Maybe the heading should be centered with some more vertical space ? Best ask Jimmac. - Why does 'Setting a new driver" take so long ? - After selecting a wrong driver from the db on purpose, there seems to be no way to get back to the 'default' driver ?
(In reply to comment #6) > - Even with async being much better, we should not go through the same wait > twice. Imo, the panel should keep the ppd db information around until it gets > unloaded. As things are now, if I dismiss the dialog and reopen it right away, > I get to wait again Fixed (locally) > - "Getting device informations..." is still wierd. What is that doing in the > menu, and why is it insensitive ? I think it should just go away ? This is there because I thought that it would be good to inform user that we are getting informations about the best suitable PPDs. Without this he will see nothing. After we have all the informations, 3 new items representing 3 best PPDs are added to the popup menu or the text "No suitable PPD found" is shown in the popup menu. > - The spacing in the dialog is not quite right. The margins are too narrow, and > the buttons don't line up with the right edge of the list. Maybe the heading > should be centered with some more vertical space ? Best ask Jimmac. The problem with the buttons is that these are placed in action area of GtkDialog. I have to find out how to expand the action area or shrink the content area since Glade is not helping much here. Or, at the worst case, replace the whole dialog with something else. > - Why does 'Setting a new driver" take so long ? This is driven by CUPS. It is CUPS_ADD_MODIFY_PRINTER request to be more specific. > - After selecting a wrong driver from the db on purpose, there seems to be no > way to get back to the 'default' driver ? This can be achieved by waiting for the 3 best PPDs and select the first one.
Created attachment 217604 [details] [review] allow users to change printer's PPD This version doesn't get the list of PPD more than once, it stores it. This version also fixes the alignment of buttons with the lists.
Ok, I've tried this version now. Better. Keeping the database around makes things much quicker the second time around. Unfortunately, the "Getting device informations..." text is still there for a split second. We should avoid that altogether when the data is already available. As for the text itself, I understand that having some text there while the data is loading, maybe we can just improve the wording: Searching for preferred drivers With a spinner before it to indicate activity.
(In reply to comment #9) > Ok, I've tried this version now. Better. Keeping the database around makes > things much quicker the second time around. Unfortunately, the "Getting device > informations..." text is still there for a split second. We should avoid that > altogether when the data is already available. I'll modify it a little. > As for the text itself, I understand that having some text there while the > data is loading, maybe we can just improve the wording: > > Searching for preferred drivers OK, I'll use this. > With a spinner before it to indicate activity. This is a little bit harder to achieve, but I'll add it there (if possible). Thanks for your comment. Btw, I'm finishing the Options dialog where I have some problems caused by the async nature of the dialog. Marek
Created attachment 218537 [details] [review] add Options dialog Hi, this patch implements the Options dialog. It allows user to change more options than current dialog. It removes the setting of allowed users which I think is not suitable for this dialog, it is for administrators. The dialog is completely asynchronous so it doesn't block anything. During working on this I encountered a problem with atk but I hope that it is not caused by the dialog. The problem is that an atk function tries to access some widgets after the dialog was destroyed (it goes through a cache in which it has several widgets). Please mention here if you encounter this (it happens if I open/close the dialog quickly). It doesn't happen if the atk-bridge is not loaded. Regards Marek
Thanks, initial impressions: - I think we want to let the user see the options even when he's not allowed to change them. Should just make the controls in the dialog insensitive in that case - The dialog should come up without scrollbars. Maybe we should do without the scrollbars altogether, but there's probably a danger of far too many options ? At the very least, we should never have a horizontal scrollbar, and try really hard to come up without a vertical scrollbar - The dialog probably needs some theming love (the white content area without a bevel or any clear demarcation looks somewhat odd) - but we can fix that later, I think
Created attachment 218624 [details] [review] allow users to change printer's PPD (In reply to comment #9) > Ok, I've tried this version now. Better. Keeping the database around makes > things much quicker the second time around. Unfortunately, the "Getting device > informations..." text is still there for a split second. We should avoid that > altogether when the data is already available. It stores the information now so it doesn't search for it again. > As for the text itself, I > understand that having some text there while the data is loading, maybe we can > just improve the wording: > > Searching for preferred drivers Fixed. Also, we have there the text "No suitable driver found" when no driver has been found. Is it ok? > With a spinner before it to indicate activity. Added. I've also modified a little the handling of async calls which needs to be run in another thread.
I'll give it another try this afternoon
Created attachment 218631 [details] screenshot of options dialog (In reply to comment #14) > I'll give it another try this afternoon Thanks. (In reply to comment #12) > Thanks, initial impressions: > > - I think we want to let the user see the options even when he's not allowed to > change them. Should just make the controls in the dialog insensitive in that > case > > - The dialog should come up without scrollbars. Maybe we should do without the > scrollbars altogether, but there's probably a danger of far too many options ? > At the very least, we should never have a horizontal scrollbar, and try really > hard to come up without a vertical scrollbar > > - The dialog probably needs some theming love (the white content area without a > bevel or any clear demarcation looks somewhat odd) - but we can fix that later, > I think I think that we can not get rid off the vertical scrollbar. But we can try to split the "Advanced" group into several smaller groups where each group would be listed below the "Advanced" group in the treeview (see last tab in gtk print dialog for a printer with a lot of options (e.g. Canon iR 3170C)). For the horizontal scrollbar, I think that wrapping the text could help here. Attached is a screenshot of the wrapped text. Marek
Review of attachment 218624 [details] [review]: I think behavior-wise and UI-wise this is fine now. Some comments on the code below. ::: panels/printers/cc-printers-panel.c @@ +191,2 @@ if (priv->cups_status_check_id > 0) g_source_remove (priv->cups_status_check_id); Should probably reset cups_status_check_id to 0 here, just to be on the safe side. @@ +524,3 @@ GtkTreeIter iter; GtkWidget *widget; + GtkWidget *widget2; Could have more meaningful variable names here, like model_button and model_label, or just button/label. @@ +2229,3 @@ + priv->driver_change_list = g_list_remove_link (priv->driver_change_list, iter); + g_list_free_full (iter, g_free); + break; This is weird - why a use a loop, if you unconditionally break ? Also, we generally write just iter = iter->next instead of bothering with g_list_next @@ +2232,3 @@ + } + + actualize_sensitivity (self); 'actualize' sounds a little grandiose... could just go with 'update' ? @@ +2461,3 @@ + else if (g_strcmp0 ((gchar *) g_object_get_data (G_OBJECT (iter->data), "spinner"), + "placeholder3") == 0) + placeholders[2] = GTK_WIDGET (iter->data); Copy-paste error ? You are assigning placeholders[2] in two cases here @@ +2521,3 @@ + gtk_widget_set_sensitive (GTK_WIDGET (placeholders[i]), TRUE); + gtk_widget_show (placeholders[i]); + } You seem to have essentially the same loop twice here ? I think should be reorganized a little, so you don't have two copies of the same code. @@ +2593,3 @@ + * when we want to actually append new menu item in a callback. + * But unfortunately it is not possible to connect to "activate" + * signal of such menu item (appended after gtk_menu_popup()). What is the problem with that ? There might be one, but that would certainly be a GTK+ bug... @@ +2618,3 @@ + gtk_widget_hide (item); + + Unneeded extra newline here, and a few places below @@ +3057,3 @@ + gtk_misc_set_padding (GTK_MISC (label), offset, 0); + } +} Have you tested how this looks in a rtl language, say he_IL ?
After showing this to Jon, some more detailed feedback On the ppd selection: - The wait for the ppds when opening the menu for the first time is just too long. We should start getting that data right away when first opening the printer panel. - In the office, when I opened the menu for my home printer, when it eventually stopped loading, it said 'no suitable driver found'. That is clearly a lie, since my printer was showing up with a ppd in the main page. - When opening the driver dialog, it should try to select the right manufacturer - we do know that about the printer, no ? - The list of manufacturer names needs to be sanitized. Showing something like EPSON or MINOLTA/KONICA MINOLTA/MINOLTA-QMS is just not acceptable. Ideally, these strings would be fixed in the original ppds; failing that, We should maintain a map in the panel that maps these bad strings to better ones: EPSON -> Epson MINOLTA -> Minolta KONICA MINOLTA -> Minolta MINOLTA-QMS -> Minolta ... - The driver names have too much detail in them; we should not display a comma-separated list of values - The driver dialog title should not have ... in it - The file chooser for selecting a ppd file has a pointless filter combo (there's only one filter), and its label is far too big. We should just not display the combo. I think you need to use set_filter instead of add_filter. On the options dialog (based on your screenshot): - Wrapping and the uneven height that it causes is not acceptable. Can we just let the dialog grow wider, horizontally ? Unrelated to the patches: - The main page (Location, Model, IP Address,...) is too spaced out. The fields should have the same spacing as e.g. in the user panel
Created attachment 218892 [details] [review] allow users to change printer's PPD Hi, thank you for your input. Here is another modification of the patch for changing PPDs. (In reply to comment #16) > Review of attachment 218624 [details] [review]: > > I think behavior-wise and UI-wise this is fine now. Some comments on the code > below. > > ::: panels/printers/cc-printers-panel.c > @@ +191,2 @@ > if (priv->cups_status_check_id > 0) > g_source_remove (priv->cups_status_check_id); > > Should probably reset cups_status_check_id to 0 here, just to be on the safe > side. Fixed. > @@ +524,3 @@ > GtkTreeIter iter; > GtkWidget *widget; > + GtkWidget *widget2; > > Could have more meaningful variable names here, like model_button and > model_label, or just button/label. Renamed. > @@ +2229,3 @@ > + priv->driver_change_list = g_list_remove_link > (priv->driver_change_list, iter); > + g_list_free_full (iter, g_free); > + break; > > This is weird - why a use a loop, if you unconditionally break ? This was a typo, it should be in brackets. This just removes and frees one item from the list. > Also, we generally write just iter = iter->next instead of bothering with > g_list_next Changed. > @@ +2232,3 @@ > + } > + > + actualize_sensitivity (self); > > 'actualize' sounds a little grandiose... could just go with 'update' ? Renamed. > @@ +2461,3 @@ > + else if (g_strcmp0 ((gchar *) g_object_get_data (G_OBJECT > (iter->data), "spinner"), > + "placeholder3") == 0) > + placeholders[2] = GTK_WIDGET (iter->data); > > Copy-paste error ? You are assigning placeholders[2] in two cases here You are right, this was a leftover of an idea. > @@ +2521,3 @@ > + gtk_widget_set_sensitive (GTK_WIDGET (placeholders[i]), > TRUE); > + gtk_widget_show (placeholders[i]); > + } > > You seem to have essentially the same loop twice here ? I think should be > reorganized a little, so you don't have two copies of the same code. Reorganized. > @@ +2593,3 @@ > + * when we want to actually append new menu item in a callback. > + * But unfortunately it is not possible to connect to "activate" > + * signal of such menu item (appended after gtk_menu_popup()). > > What is the problem with that ? There might be one, but that would certainly be > a GTK+ bug... I'll prepare a reproducer when I'm finished with this. > @@ +2618,3 @@ > + gtk_widget_hide (item); > + > + > > Unneeded extra newline here, and a few places below Removed (these newlines help me with orientation in the code but I can live without that). > @@ +3057,3 @@ > + gtk_misc_set_padding (GTK_MISC (label), offset, 0); > + } > +} > > Have you tested how this looks in a rtl language, say he_IL ? Yes, it works fro me. Have you encountered a problem here? (In reply to comment #17) > After showing this to Jon, some more detailed feedback > > On the ppd selection: > > - The wait for the ppds when opening the menu for the first time is just too > long. We should start getting that data right away when first opening the > printer panel. Done. > - In the office, when I opened the menu for my home printer, when it eventually > stopped loading, it said 'no suitable driver found'. That is clearly a lie, > since my printer was showing up with a ppd in the main page. The problem here is that we don't have original DeviceID (IEEE 1284) for the device so I ask cups to detect all connected devices and find the one I need among those (with respect to device-uri). If CUPS doesn't find the device, like in your case (because it is not available), then method "GetBestDrivers" can not find the best drivers because it needs the DeviceID. If we would take the DeviceID from the current PPD then we would end with the best PPD pointing to the current PPD. > - When opening the driver dialog, it should try to select the right > manufacturer - we do know that about the printer, no ? Added. > - The list of manufacturer names needs to be sanitized. Showing something like > EPSON or MINOLTA/KONICA MINOLTA/MINOLTA-QMS is just not acceptable. Ideally, > these strings would be fixed in the original ppds; failing that, We should > maintain a map in the panel that maps these bad strings to better ones: > > EPSON -> Epson > MINOLTA -> Minolta > KONICA MINOLTA -> Minolta > MINOLTA-QMS -> Minolta > ... I've added a "hash table" which translates those names to more pleasant ones. > - The driver names have too much detail in them; we should not display a > comma-separated list of values Do you mean the list of the best drivers or the list of all drivers? Anyway, I would like to look at this after we have all those features in master. > - The driver dialog title should not have ... in it Removed. > - The file chooser for selecting a ppd file has a pointless filter combo > (there's only one filter), and its label is far too big. We should just not > display the combo. I think you need to use set_filter instead of add_filter. I've changed it to the "set_filter". Regards Marek
Created attachment 218904 [details] [review] add Options dialog (In reply to comment #15) > (In reply to comment #12) > > Thanks, initial impressions: > > > > - I think we want to let the user see the options even when he's not > > allowed to change them. Should just make the controls in the dialog > > insensitive in that case Done. > > - The dialog should come up without scrollbars. Maybe we should do without > > the scrollbars altogether, but there's probably a danger of far too many > > options ? > > At the very least, we should never have a horizontal scrollbar, and try > > really hard to come up without a vertical scrollbar > > > > I think that we can not get rid off the vertical scrollbar. But we can try to > split the "Advanced" group into several smaller groups where each group would > be listed below the "Advanced" group in the treeview (see last tab in gtk > print dialog for a printer with a lot of options (e.g. Canon iR 3170C)). I've extended lists of option groups which sorts options to groups according to available PPDs. It helps to have those groups populated more evenly. > On the options dialog (based on your screenshot): > - Wrapping and the uneven height that it causes is not acceptable. Can we just > let the dialog grow wider, horizontally ? I've removed the wrapping and let the dialog grow horizontally. Sometimes it can be quite wide, but I think that it is acceptable. I've also changed height of the dialog from 350 to 400 pixels so that it can show more options without horizontal scrollbars. > > - The dialog probably needs some theming love (the white content area > > without a bevel or any clear demarcation looks somewhat odd) - but we can > > fix that later, I think I'll look at this later. Regards Marek
(In reply to comment #18) > > 'actualize' sounds a little grandiose... could just go with 'update' ? > > Renamed. There's some more actualize functions in there, but not a big deal. > > > @@ +3057,3 @@ > > + gtk_misc_set_padding (GTK_MISC (label), offset, 0); > > + } > > +} > > > > Have you tested how this looks in a rtl language, say he_IL ? > > Yes, it works fro me. Have you encountered a problem here? No, just wondered. And indeed, seems to work ok > > - In the office, when I opened the menu for my home printer, when it eventually > > stopped loading, it said 'no suitable driver found'. That is clearly a lie, > > since my printer was showing up with a ppd in the main page. > > The problem here is that we don't have original DeviceID (IEEE 1284) for the > device so I ask cups to detect all connected devices and find the one I need > among those (with respect to device-uri). If CUPS doesn't find the device, like > in your case (because it is not available), then method "GetBestDrivers" can > not find the best drivers because it needs the DeviceID. If we would take the > DeviceID from the current PPD then we would end with the best PPD pointing to > the current PPD. Hmm, we'll have to do something better about this situation in general ('printer out of reach'). But not as part of this patch. > > > - The driver names have too much detail in them; we should not display a > > comma-separated list of values > > Do you mean the list of the best drivers or the list of all drivers? Anyway, I > would like to look at this after we have all those features in master. I mean the strings like: HP 915 hpijs, 3.12.6 IBM InfoPrint 12 - CUPS+GutenPrint v5.2.9 Simplified Anyway, lets take this issue on separately.
Review of attachment 218892 [details] [review]: Lets land this, then.
Comment on attachment 218892 [details] [review] allow users to change printer's PPD (In reply to comment #21) > Review of attachment 218892 [details] [review]: > > Lets land this, then. Thank you for your review. I've committed it to the master.
(In reply to comment #19) > Created an attachment (id=218904) [details] [review] > add Options dialog I'm going to rebase this against current master.
Review of attachment 218904 [details] [review]: If you fix those cosmetics, I think this is good - we can fix the remaining things later. ::: panels/printers/cc-printers-panel.c @@ +2213,3 @@ + else if (response_id == GTK_RESPONSE_REJECT) + { + } The else is a bit pointless. Why are you using REJECT and not the more customary CANCEL, anyway ? @@ +2234,3 @@ + g_permission_get_allowed (G_PERMISSION (priv->permission)) && + priv->lockdown_settings && + !g_settings_get_boolean (priv->lockdown_settings, "disable-print-setup"); Could either priv->permission or priv->lockdown_settings actually be NULL here ? ::: panels/printers/pp-options-dialog.c @@ +670,3 @@ + + /* Translators: "Advanced" tab contains all others settings */ + tab_add (_("Advanced"), notebook, treeview, advanced_tab_grid); I think some of these strings could do with translation context, .e.g 'Color' might well conflict with strings from the color panel. Should probably just treat them all the same: C_("Printer Option Group", "General") C_("Printer Option Group", "Color") ... @@ +675,3 @@ + + /* Select the first option group */ + if ((selection = gtk_tree_view_get_selection (treeview)) != NULL) selection can't really be NULL here, or can it ? @@ +730,3 @@ + if (dialog->ppd_filename_set && + dialog->destination_set && + dialog->ipp_attributes_set) Checking destination_set here seems silly, since you've _just_ set it...
Comment on attachment 218904 [details] [review] add Options dialog Hi, thank you for the review. I've committed it with these changes (+ rebased it against current master): (In reply to comment #24) > Review of attachment 218904 [details] [review]: > > If you fix those cosmetics, I think this is good - we can fix the remaining > things later. > > ::: panels/printers/cc-printers-panel.c > @@ +2213,3 @@ > + else if (response_id == GTK_RESPONSE_REJECT) > + { > + } Removed, it shouldn't be there. > The else is a bit pointless. > Why are you using REJECT and not the more customary CANCEL, anyway ? The dialog doesn't use it now. > @@ +2234,3 @@ > + g_permission_get_allowed (G_PERMISSION (priv->permission)) && > + priv->lockdown_settings && > + !g_settings_get_boolean (priv->lockdown_settings, "disable-print-setup"); > > Could either priv->permission or priv->lockdown_settings actually be NULL here > ? Actually, "priv->permission" can be NULL (when an old version of cups-pk-helper is installed) and checking "priv->lockdown_settings" keeps us on safe side. > ::: panels/printers/pp-options-dialog.c > @@ +670,3 @@ > + > + /* Translators: "Advanced" tab contains all others settings */ > + tab_add (_("Advanced"), notebook, treeview, advanced_tab_grid); > > I think some of these strings could do with translation context, .e.g 'Color' > might well conflict with strings from the color panel. Should probably just > treat them all the same: > > C_("Printer Option Group", "General") > C_("Printer Option Group", "Color") > ... Changed. > @@ +675,3 @@ > + > + /* Select the first option group */ > + if ((selection = gtk_tree_view_get_selection (treeview)) != NULL) > > selection can't really be NULL here, or can it ? Actually, it can be NULL, so the check is necessary. > @@ +730,3 @@ > + if (dialog->ppd_filename_set && > + dialog->destination_set && > + dialog->ipp_attributes_set) > > Checking destination_set here seems silly, since you've _just_ set it... I wanted to have it uniform but you are right, it is not necessary to check it. Regards Marek
Marek, whats left here now, reworking the new printer dialog ?
(In reply to comment #26) > Marek, whats left here now, reworking the new printer dialog ? Hi Matthias, yes, the new printer dialog is remaining + moving list of jobs to its own dialog. I'm working on the new printer dialog now.
Do we want to keep there functions which supply the system-config-printer's dbus methods when s-c-p is not installed (looking for the best ppd available)? I would prefer to remove it and rely on installed s-c-p (as we do for cups-pk-helper and packagekit).
Just curious, why would we want to continue to rely on s-c-p at all?
(In reply to comment #29) > Just curious, why would we want to continue to rely on s-c-p at all? Because s-c-p provides methods for getting list of best ppds for given device, for grouping device uris of the same device (one device can have several device-uris) and for getting list of missing executables for given PPD.
To be more specific, they are provided by system-config-printer-libs.
s-c-p-libs is libshouldbeincups, pretty much. We'll continue using it for sharing functionality that really belongs into the printing system
Created attachment 219333 [details] [review] redesign of the new printer dialog Hi, this patch modifies the new printer dialog according to the design mentioned in the first comment. All operations are asynchronous so it doesn't "stuck". There are 3 things I need to solve yet: 1) the situation when no printer has been found - just a text in the middle with "No printers found" ? 2) the time when the new printer is being added - show the printer in the the list of printers with status "Installing", "Configuring" with all other options insensitive? 3) have extra entry for filtering result as mentioned before? Other than that it should be ready. (yeah, I forgot to place a hint into the entry, will do in next version of the patch) Regards Marek
Review of attachment 217031 [details] [review]: mark as rejected to get out of git-bz
(harmless) warning when loading: Gtk-WARNING **: Unknown property: GtkSpinner.fill Does it make sense that it offers me to create a copy of my existing network printer here at home ? Honest question - there might be a use case for setting up a separate printer, e.g. color vs b/w. When I go ahead and hit 'Add', it takes _forever_ until the printer appears in the list. No way to speed that up ? Good that the panel remains responsive in that time, but we need to somehow indicate that something is going on. When the printer finally appears in the list, it is grayed out (I think 'stopped'), and I can't select it. There appears to be at least one blocking call somewhere, though - the panel becomes unresponsive for quite a while after the printer appears in the list. Eventually, the printer state changes from 'stopped' to 'ready', and things go back to normal. Also, when starting to add a printer, and then going back to the overview before it appears in the list, I can't reopen the printer panel until I get the 'printer added' notification. I managed to crash it once when playing around with the panel while a printer add was in progress.
Hi, thank you for your comment. (In reply to comment #35) > Does it make sense that it offers me to create a copy of my existing network > printer here at home ? Honest question - there might be a use case for setting > up a separate printer, e.g. color vs b/w. Yes, this possibility is there because of the different settings for the same printer. > When I go ahead and hit 'Add', it takes _forever_ until the printer appears in > the list. No way to speed that up ? Good that the panel remains responsive in > that time, but we need to somehow indicate that something is going on. It should show the printer in the list immediately but grayed out and if you click on it, you should see a page telling you that the printer is being installed. > When the printer finally appears in the list, it is grayed out (I think > 'stopped'), and I can't select it. There appears to be at least one blocking > call somewhere, though - the panel becomes unresponsive for quite a while after > the printer appears in the list. Eventually, the printer state changes from > 'stopped' to 'ready', and things go back to normal. The situation you see is probably configuring phase of installation of the printer. It is installed at first (so it shows in the list as a normal printer but not configured yet) and then a "configuration" step is performed. This include enabling of the printer, setting to enable accepting of jobs, setting of paper size according to locale, autoconfiguration and installation of missing executables. Maybe the non-responsiveness is caused by actualization of the list of the printer in the printers panel itself. It is still synchronous. Is it also slow when you enable/disable a printer (or set a printer as default)? > Also, when starting to add a printer, and then going back to the overview > before it appears in the list, I can't reopen the printer panel until I get the > 'printer added' notification. I'll look at this closer. > I managed to crash it once when playing around with the panel while a printer > add was in progress. I'll fix this.
Created attachment 219627 [details] [review] redesign of the new printer dialog (ready for review) Hi, this version of the patch improves several things: It doesn't crash now (or at least for me). It shows the new printer immediately with status "Installing" or "Configuring" and selects it. "No printers detected." is centered now. There is a hint in the search entry saying: "Enter address of a printer or a text to filter results". The search entry is GtkSearchEntry now. You can add another instance of the same printer from remote cups (or from localhost - good for duplicating printers :) ). Unfortunately it still can happen that the dialog become unresponsive for a while when you add a printer and immediately try enable/disable another printer (or try to perform another synchronous operation with another printer). Maybe it would be good to indicate addition of the new printer by a spinner in the printers list instead of icon of the printer but I haven't found a way how to do that (except writing new GtkCellRenderer). Regards Marek
Created attachment 219887 [details] [review] move list of jobs to separate dialog Hi, this patch moves list of jobs to separate dialog according to the design. Regards Marek
Created attachment 220144 [details] [review] add support for samba printers Hi, this patch adds support for samba printers. Unfortunately samba printers needs a little different workflow than other printers. We don't have any information about model of the printer. User needs to select a PPD for it during installation. For now, I show dialog for selection of ppd from all installed ppds (the one we already have there for changing model), but we need to be able to select a PPD file directly too. The problem is where to place a button for showing GtkFileChooser for this (to the dialog with list of installed PPDs (a special item in the list?)?, extra dialog?). Another "problem" is that for most samba printers we need to set username and password to be able to print on them. I've created a small authentication dialog for this but I'm sure that it needs some tweak from design point of view. Jon, Allan, what do you think about this? Regards Marek
Created attachment 220539 [details] [review] redesign of the new printer dialog (ready for review) I've rebased the patch for current master.
(In reply to comment #38) > Created an attachment (id=219887) [details] [review] > move list of jobs to separate dialog Hi, I've committed this patch to master. There is not many places where things can go wrong. It moves code to a separate dialog mostly. Regards Marek
Created attachment 220550 [details] [review] redesign of the new printer dialog (ready for review) (In reply to comment #40) > Created an attachment (id=220539) [details] [review] > redesign of the new printer dialog (ready for review) > > I've rebased the patch for current master. Another rebase for current master.
Created attachment 220557 [details] [review] add support for samba printers This is a rebase of samba support patch for current master.
Created attachment 220676 [details] [review] redesign of the new printer dialog (ready for review) Rebase because of today's changes.
Created attachment 220855 [details] [review] redesign of the new printer dialog (ready for review) This version uses "inline-toolbar" style for neighbourhood of the GtkSearchEntry in the new printer dialog.
Review of attachment 220855 [details] [review]: All the pp-utils.[ch] and related changes should be made in a separate commit, ditto for pp-new-printer-dialog.[ch] which only seems to add new API and not modify existing one. ::: panels/printers/cc-printers-panel.c @@ +176,3 @@ + g_free (priv->new_printer_name); + priv->new_printer_name = NULL; g_clear_pointer(); @@ +751,3 @@ + if (!status && + reason && + g_strcmp0 (reason, "none") != 0) g_str_equal() instead of g_strcmp0, seeing as you already know neither will be NULL. @@ +1047,3 @@ + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (widget), FALSE); + g_signal_handlers_unblock_by_func (G_OBJECT (widget), printer_set_default_cb, self); + gtk_widget_set_sensitive (widget, sensitive); Why do you capture sensitive and set it again afterwards? @@ +1357,3 @@ + gtk_tree_model_get (tree_model, iter, PRINTER_PAUSED_COLUMN, &paused, -1); + + if (paused) g_object_set (cell, "sensitive", !paused, NULL); ? @@ +1735,3 @@ + g_free (priv->new_printer_name); + priv->new_printer_name = NULL; Use g_clear_pointer() @@ +2576,3 @@ + gtk_builder_get_object (priv->builder, "printers-treeview"); + + if ((selection = gtk_tree_view_get_selection (treeview)) != NULL && No assignment inside the comparison please. Especially as gtk_tree_view_get_selection() will always return non-NULL unless treeview is invalid. @@ +2586,3 @@ + if (priv->new_printer_name && + current_printer_name && + g_strcmp0 (priv->new_printer_name, current_printer_name) == 0) You don't need to check them both for NULL if you know that they will be the same in that conditiional. @@ +2641,2 @@ widget = (GtkWidget*) gtk_builder_get_object (priv->builder, "printer-location-label"); + cc_editable_entry_set_editable (CC_EDITABLE_ENTRY (widget), local_server && !is_discovered && is_authorized && !is_new); Do you want to store "local_server && !is_discovered && is_authorized && !is_new" somewhere to make this clearer? ::: panels/printers/pp-new-printer-dialog.h @@ +39,3 @@ + gpointer user_data); + +PpNewPrinterDialog *pp_new_printer_dialog_new (GtkWindow *parent, You should use signals here. For example: dialog = dialog_new(); g_signal_connect (dialog, "response", ...); g_signal_connect (dialog, "pre-response", ...); ... ::: panels/printers/pp-utils.h @@ +42,3 @@ + DEFAULT_CUPS_SERVER = 0, + REMOTE_CUPS_SERVER, + SNMP Prefix those enums. @@ +288,3 @@ + gint host_port; + gint acquisition_method; +} PrintDevice; Prefix that structure name @@ +333,3 @@ + gpointer user_data); + +void printer_add_async (gchar *device_name, All the async APIs need to use GIO style async functions. You've defined 5 separate callback types, and they all seem to include success notification in different ways, etc.
Created attachment 221527 [details] [review] asynchronous operations needed for redesigned new printer dialog Hi Bastien, thank you for your review. (In reply to comment #46) > Review of attachment 220855 [details] [review]: > > All the pp-utils.[ch] and related changes should be made in a separate commit, > ditto for pp-new-printer-dialog.[ch] which only seems to add new API and not > modify existing one. I've moved changes related to the pp-utils.{c,h} to its own commit but I left the others in one because I think that they are related. > ::: panels/printers/cc-printers-panel.c > @@ +176,3 @@ > > + g_free (priv->new_printer_name); > + priv->new_printer_name = NULL; > > g_clear_pointer(); Done. > @@ +751,3 @@ > + if (!status && > + reason && > + g_strcmp0 (reason, "none") != 0) > > g_str_equal() instead of g_strcmp0, seeing as you already know neither will be > NULL. Done. > @@ +1047,3 @@ > + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (widget), FALSE); > + g_signal_handlers_unblock_by_func (G_OBJECT (widget), > printer_set_default_cb, self); > + gtk_widget_set_sensitive (widget, sensitive); > > Why do you capture sensitive and set it again afterwards? I actually can not recall why I wrote it this way (this is copy-pasted from another location in the file). I removed it and did some test and it seems that it is not needed now. > @@ +1357,3 @@ > + gtk_tree_model_get (tree_model, iter, PRINTER_PAUSED_COLUMN, &paused, -1); > + > + if (paused) > > g_object_set (cell, "sensitive", !paused, NULL); ? Fixed. > @@ +1735,3 @@ > > + g_free (priv->new_printer_name); > + priv->new_printer_name = NULL; > > Use g_clear_pointer() Done. > @@ +2576,3 @@ > + gtk_builder_get_object (priv->builder, "printers-treeview"); > + > + if ((selection = gtk_tree_view_get_selection (treeview)) != NULL && > > No assignment inside the comparison please. Especially as > gtk_tree_view_get_selection() will always return non-NULL unless treeview is > invalid. Actually the test is there because it could happen that the selection is NULL. > @@ +2586,3 @@ > + if (priv->new_printer_name && > + current_printer_name && > + g_strcmp0 (priv->new_printer_name, current_printer_name) == 0) > > You don't need to check them both for NULL if you know that they will be the > same in that conditiional. Done. > @@ +2641,2 @@ > widget = (GtkWidget*) gtk_builder_get_object (priv->builder, > "printer-location-label"); > + cc_editable_entry_set_editable (CC_EDITABLE_ENTRY (widget), local_server && > !is_discovered && is_authorized && !is_new); > > Do you want to store "local_server && !is_discovered && is_authorized && > !is_new" somewhere to make this clearer? I've added there new variable "already_present_local" to make it shorter. > ::: panels/printers/pp-new-printer-dialog.h > @@ +39,3 @@ > + gpointer user_data); > + > +PpNewPrinterDialog *pp_new_printer_dialog_new (GtkWindow > *parent, > > You should use signals here. For example: > dialog = dialog_new(); > g_signal_connect (dialog, "response", ...); > g_signal_connect (dialog, "pre-response", ...); > ... This would need quite a big change because the PpNewPrinterDialog is actually just a "struct" and not an object. Unfortunately, I don't have time for this now (I'm on holiday right now). But we can commit this and change it later so we can get UI changes in before UI freeze. > ::: panels/printers/pp-utils.h > @@ +42,3 @@ > + DEFAULT_CUPS_SERVER = 0, > + REMOTE_CUPS_SERVER, > + SNMP > > Prefix those enums. Fixed. > @@ +288,3 @@ > + gint host_port; > + gint acquisition_method; > +} PrintDevice; > > Prefix that structure name Done. > @@ +333,3 @@ > + gpointer user_data); > + > +void printer_add_async (gchar *device_name, > > All the async APIs need to use GIO style async functions. You've defined 5 > separate callback types, and they all seem to include success notification in > different ways, etc. I've done it this way because there are no objects for which we could perform these operations (maybe except the printer_add_async()). These functions should be just simple replacements of their synchronous equivalents. Creating new object for each of this functions seems redundant to me. Also, get_cups_devices_async() calls its callback multiple times returning devices for different schemas (so the user doesn't need to wait for detection of all devices before any of them is shown). But if you want it this way I can change it later when I'm back. We can commit this for now to catch the UI freeze. Regards Marek
Created attachment 221529 [details] [review] redesign of the new printer dialog The second patch.
(In reply to comment #47) > Created an attachment (id=221527) [details] [review] > asynchronous operations needed for redesigned new printer dialog > > @@ +2576,3 @@ > > + gtk_builder_get_object (priv->builder, "printers-treeview"); > > + > > + if ((selection = gtk_tree_view_get_selection (treeview)) != NULL && > > > > No assignment inside the comparison please. Especially as > > gtk_tree_view_get_selection() will always return non-NULL unless treeview is > > invalid. > > Actually the test is there because it could happen that the selection is NULL. It can't be, unless the treeview is invalid. And even if you wanted to double-check whether it was non-NULL, just split out the comparison and assignment. > > ::: panels/printers/pp-new-printer-dialog.h > > @@ +39,3 @@ > > + gpointer user_data); > > + > > +PpNewPrinterDialog *pp_new_printer_dialog_new (GtkWindow > > *parent, > > > > You should use signals here. For example: > > dialog = dialog_new(); > > g_signal_connect (dialog, "response", ...); > > g_signal_connect (dialog, "pre-response", ...); > > ... > > This would need quite a big change because the PpNewPrinterDialog is actually > just a "struct" and not an object. Unfortunately, I don't have time for this > now (I'm on holiday right now). But we can commit this and change it later so > we can get UI changes in before UI freeze. This isn't really my problem though. It's a huge amount of code, and I'm really not certain that there will be incentive to getting it fixed after it's merged. So either this is fixed and I can review it before UI freeze, or you'll have to ask for a freeze break. > > @@ +333,3 @@ > > + gpointer user_data); > > + > > +void printer_add_async (gchar *device_name, > > > > All the async APIs need to use GIO style async functions. You've defined 5 > > separate callback types, and they all seem to include success notification in > > different ways, etc. > > I've done it this way because there are no objects for which we could perform > these operations (maybe except the printer_add_async()). Could that one be changed then? > These functions should > be just simple replacements of their synchronous equivalents. Creating new > object for each of this functions seems redundant to me. For get_ppd_names_for_device_async() for example, you could use a boxed type representing the device_id. The work is minimal, and that means that you don't have to use custom callbacks, and people used to GLib can figure out the expected lifecycle of the different objects and properties, as well as being able to understand bugs that'll crop up. > Also, > get_cups_devices_async() calls its callback multiple times returning devices > for different schemas (so the user doesn't need to wait for detection of all > devices before any of them is shown). But if you want it this way I can change > it later when I'm back. We can commit this for now to catch the UI freeze. It's fine not to change get_cups_devices_async() then, but changing the rest needs to be done before we merge this.
(In reply to comment #49) > (In reply to comment #47) > > Created an attachment (id=221527) [details] [review] [details] [review] > > asynchronous operations needed for redesigned new printer dialog > > > @@ +2576,3 @@ > > > + gtk_builder_get_object (priv->builder, "printers-treeview"); > > > + > > > + if ((selection = gtk_tree_view_get_selection (treeview)) != NULL && > > > > > > No assignment inside the comparison please. Especially as > > > gtk_tree_view_get_selection() will always return non-NULL unless treeview is > > > invalid. > > > > Actually the test is there because it could happen that the selection is NULL. > > It can't be, unless the treeview is invalid. And even if you wanted to > double-check whether it was non-NULL, just split out the comparison and > assignment. OK, I split it out. > > > ::: panels/printers/pp-new-printer-dialog.h > > > @@ +39,3 @@ > > > + gpointer user_data); > > > + > > > +PpNewPrinterDialog *pp_new_printer_dialog_new (GtkWindow > > > *parent, > > > > > > You should use signals here. For example: > > > dialog = dialog_new(); > > > g_signal_connect (dialog, "response", ...); > > > g_signal_connect (dialog, "pre-response", ...); > > > ... > > > > This would need quite a big change because the PpNewPrinterDialog is actually > > just a "struct" and not an object. Unfortunately, I don't have time for this > > now (I'm on holiday right now). But we can commit this and change it later so > > we can get UI changes in before UI freeze. > > This isn't really my problem though. It's a huge amount of code, and I'm really > not certain that there will be incentive to getting it fixed after it's merged. I can assure you that there will be incentive to get it fixed. > So either this is fixed and I can review it before UI freeze, or you'll have to > ask for a freeze break. I worked on this last two nights and I haven't finished it yet so I asked on corresponding mailing lists whether they will allow me to do the freeze break when the patch is ready (at the first weekend of September). > > > @@ +333,3 @@ > > > + gpointer user_data); > > > + > > > +void printer_add_async (gchar *device_name, > > > > > > All the async APIs need to use GIO style async functions. You've defined 5 > > > separate callback types, and they all seem to include success notification in > > > different ways, etc. > > > > I've done it this way because there are no objects for which we could perform > > these operations (maybe except the printer_add_async()). > > Could that one be changed then? Yes, I'll change it, I'll create a new object for this. Do you want to have the object in separate source files? > > These functions should > > be just simple replacements of their synchronous equivalents. Creating new > > object for each of this functions seems redundant to me. > > For get_ppd_names_for_device_async() for example, you could use a boxed type > representing the device_id. The work is minimal, and that means that you don't > have to use custom callbacks, and people used to GLib can figure out the > expected lifecycle of the different objects and properties, as well as being > able to understand bugs that'll crop up. Do you mean to create a boxed type and functions get_ppd_names_for_device_async(), get_ppd_names_for_device_finished() + the handling of GAsyncResult? This would need the boxed type to be an descendant of GObject but it isn't (correct me if I'm wrong). > > Also, > > get_cups_devices_async() calls its callback multiple times returning devices > > for different schemas (so the user doesn't need to wait for detection of all > > devices before any of them is shown). But if you want it this way I can change > > it later when I'm back. We can commit this for now to catch the UI freeze. > > It's fine not to change get_cups_devices_async() then, but changing the rest > needs to be done before we merge this. OK
Created attachment 222748 [details] Samba printer installation workflow 1/4 This is series of screenshots for the samba support patch. The basic workflow is to enter an adress of a samba server, select printer from the found printers, select PPD from the installed PPDs and enter authentication info if needed. But we would need to allow user to select a PPD directly through a GtkFileChooser if he doesn't want to select it from the installed ones. So it has to be changed yet.
Created attachment 222749 [details] Samba printer installation workflow 2/4
Created attachment 222750 [details] Samba printer installation workflow 3/4
Created attachment 222751 [details] Samba printer installation workflow 4/4
(In reply to comment #51) > Created an attachment (id=222748) [details] > Samba printer installation workflow 1/4 > > This is series of screenshots for the samba support patch. The basic workflow > is to enter an adress of a samba server, select printer from the found > printers, select PPD from the installed PPDs and enter authentication info if > needed. But we would need to allow user to select a PPD directly through a > GtkFileChooser if he doesn't want to select it from the installed ones. So it > has to be changed yet. The first screenshot is the new new printer dialog with entered address of the samba server from which we want to install new printer (just typed in + enter). The listed devices are not discovered automatically.
I've moved the remaining pieces (add dialog, samba support) over to bug 683229. Lets close this one.