After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759308 - Instant apply in printing dialog (number of copies)
Instant apply in printing dialog (number of copies)
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Printing
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: Felipe Borges
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-12-10 15:23 UTC by Frédéric Parrenin
Modified: 2017-06-07 12:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
printunixdialog: Update collate icon as entry changes (1.09 KB, patch)
2017-05-10 14:55 UTC, Felipe Borges
none Details | Review
printunixdialog: Update collate icon as entry changes (1.60 KB, patch)
2017-05-18 14:18 UTC, Felipe Borges
none Details | Review
printunixdialog: Update collate icon as entry changes (1.61 KB, patch)
2017-05-31 10:43 UTC, Felipe Borges
none Details | Review
printunixdialog: Update collate icon as entry changes (1.69 KB, patch)
2017-06-06 11:55 UTC, Felipe Borges
committed Details | Review

Description Frédéric Parrenin 2015-12-10 15:23:29 UTC
Steps to reproduce:
- open a pdf document with several pages
- hit CTRL-P to open the printing dialog
- go to the "copy" text field and enter "2" with the keyboard
=> the small icon to illustrate the assembling of copies is not updated
- now hit TAB
=> now the icon is updated

This is a minor bug but every detail matters!
Comment 1 Matthias Clasen 2015-12-10 20:27:45 UTC
This is normal spin button behavior - text editing changes are taking effect when you activate the entry (hit Enter), or on focus out.
Comment 2 Frédéric Parrenin 2015-12-11 13:16:48 UTC
I disagree here.
If you enter "2" with the keyboard in the text field, and hit TAB, the "Assemble" Checkbox *will* get activated *BUT* the icon is *not* updated.
So the behavior is inconsistent.
Moreover, this behavior is *not* intuitive. A beginner will not know he has to focus out for the entry to take effect. It should just work instantaneously (like instant search in google for example).
Comment 3 Matthias Clasen 2015-12-11 14:21:36 UTC
sure. a patch would be welcome.
Comment 4 Frédéric Parrenin 2017-04-30 17:38:17 UTC
This bug is still present in 3.22.
Comment 5 Felipe Borges 2017-05-10 14:55:52 UTC
Created attachment 351555 [details] [review]
printunixdialog: Update collate icon as entry changes

By relying on GtkSpinButton default activation behavior, the
collate icon doesn't get updated when a new number is typed
in the copies spin button.
Comment 6 Felipe Borges 2017-05-10 14:56:48 UTC
I find this solution to be quite hack-ish, but it does the job if this is a desired behavior.

/me not proud. :)
Comment 7 Marek Kašík 2017-05-11 09:40:38 UTC
Review of attachment 351555 [details] [review]:

(In reply to Felipe Borges from comment #6)
> I find this solution to be quite hack-ish, but it does the job if this is a
> desired behavior.
>
> /me not proud. :)

So what about to make it a little less hack-ish :) ?

I would propose to modify the function dialog_get_n_copies(). You can get the spin button's text there and try to convert it with e.g. g_ascii_strtoull() (don't forget to check for failing conversion there). If the conversion fail or the value is out of the adjustment's range then you will use the original value (the gtk_spin_button_get_value_as_int (GTK_SPIN_BUTTON (dialog->priv->copies_spin)) one). If the value is converted correctly and is inside the range then use the converted one.
Simple, isn't it? :)
Comment 8 Felipe Borges 2017-05-18 14:13:22 UTC
(In reply to Marek Kašík from comment #7)
> Review of attachment 351555 [details] [review] [review]:
> 
> (In reply to Felipe Borges from comment #6)
> > I find this solution to be quite hack-ish, but it does the job if this is a
> > desired behavior.
> >
> > /me not proud. :)
> 
> So what about to make it a little less hack-ish :) ?
> 
> I would propose to modify the function dialog_get_n_copies(). You can get
> the spin button's text there and try to convert it with e.g.
> g_ascii_strtoull() (don't forget to check for failing conversion there). If
> the conversion fail or the value is out of the adjustment's range then you
> will use the original value (the gtk_spin_button_get_value_as_int
> (GTK_SPIN_BUTTON (dialog->priv->copies_spin)) one). If the value is
> converted correctly and is inside the range then use the converted one.
> Simple, isn't it? :)

Oh, I didn't know you would be willing to accept a patch for this bug anyway. This way I will cook up a patch as you suggested in a bit. Thanks Marek!
Comment 9 Felipe Borges 2017-05-18 14:18:45 UTC
Created attachment 352101 [details] [review]
printunixdialog: Update collate icon as entry changes

By relying on GtkSpinButton default activation behavior, the
collate icon doesn't get updated when a new number is typed
in the copies spin button.
Comment 10 Marek Kašík 2017-05-30 13:51:53 UTC
Review of attachment 352101 [details] [review]:

::: gtk/gtkprintunixdialog.c
@@ +2560,3 @@
+
+  text = gtk_entry_get_text (GTK_ENTRY (priv->copies_spin));
+  n_copies = g_ascii_strtoull (text, NULL, 0);

You don't check whether the conversion was successful.

@@ +2564,3 @@
+      (n_copies <= gtk_adjustment_get_upper (adjustment)))
+    {
+      gtk_spin_button_set_value (GTK_SPIN_BUTTON (priv->copies_spin), n_copies);

I didn't propose to set the value, just return it if the conversion was successful, otherwise return the same value as before.

@@ +2569,1 @@
   if (gtk_widget_is_sensitive (dialog->priv->copies_spin))

I think that this check is still relevant for the whole code.
Comment 11 Felipe Borges 2017-05-31 10:43:09 UTC
Created attachment 352939 [details] [review]
printunixdialog: Update collate icon as entry changes

By relying on GtkSpinButton default activation behavior, the
collate icon doesn't get updated when a new number is typed
in the copies spin button.
Comment 12 Marek Kašík 2017-06-06 10:52:08 UTC
Review of attachment 352939 [details] [review]:

::: gtk/gtkprintunixdialog.c
@@ +2564,3 @@
   if (gtk_widget_is_sensitive (dialog->priv->copies_spin))
+    {
+      if ((n_copies != 0) &&

Here I expected to check whether the pointer from second parameter was set to "text" (which would indicate an error) or whether it points to the ending '\0' (which would signalize correct conversion of the whole string).
Basically, I would replace this with:
if ((n_copies != 0 && endptr != text && (endptr != NULL && endptr[0] == '\0')) &&

You can test what happen if you enter there "3aaa" to see the difference.
Comment 13 Felipe Borges 2017-06-06 11:55:56 UTC
Created attachment 353256 [details] [review]
printunixdialog: Update collate icon as entry changes

By relying on GtkSpinButton default activation behavior, the
collate icon doesn't get updated when a new number is typed
in the copies spin button.
Comment 14 Marek Kašík 2017-06-06 12:58:42 UTC
Review of attachment 353256 [details] [review]:

Thank you for the patch and the modifications. The patch looks good to me now. Push it to master and gtk-3-22 branches please.
Comment 15 Felipe Borges 2017-06-07 12:41:10 UTC
Attachment 353256 [details] pushed as d940388 - printunixdialog: Update collate icon as entry changes
Attachment 353256 [details] pushed as 322ba75 - printunixdialog: Update collate icon as entry changes