Hitting "Cancel" on custom Header/Footer dialog replaces original setting with --blank--

RESOLVED FIXED in mozilla1.9beta5

Status

()

Core
Widget: Gtk
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dholbert, Assigned: Michael Ventnor)

Tracking

Trunk
mozilla1.9beta5
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Steps to reproduce:
===================
  1. File|Print
  2. Click "Options" tab of print dialog
  3. Under "Header and Footer", click "Title" (the first dropdown menu) and choose "Custom..."
  4. Press "Cancel" on the dialog that appears

Expected Outcome:
=================
  That first dropdown should keep its default setting of "Title"

Actual Outcome:
===============
  The dropdown is cleared to "--blank--"


This happens with all the header/footer dropdowns on that Options tab.
I think the XXX comment below (in nsPrintDialogGTK.cpp) is referring to this exact bug:

 60 static void
 61 ShowCustomDialog(GtkComboBox *changed_box, gpointer user_data)
 ...
 106   if (diag_response == GTK_RESPONSE_ACCEPT) {
 107     const gchar* response_text = gtk_entry_get_text(GTK_ENTRY(custom_entry));
 108     g_object_set_data_full(G_OBJECT(changed_box), "custom-text", strdup(response_text), (GDestroyNotify) free);
 109   } else {
 110     // XXX I wish there was a way to be smarter than this... but at least we were smarter than before
 111     // where the dropdown stayed on Custom... even after clicking Cancel.
 112     gtk_combo_box_set_active(changed_box, 0);
 113   }

http://mxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsPrintDialogGTK.cpp#106
(Assignee)

Comment 2

10 years ago
This is intentional, however there probably are cases where clicking cancel should not do this.
(Assignee)

Comment 3

10 years ago
Created attachment 305844 [details] [diff] [review]
Patch
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #305844 - Flags: superreview?(roc)
Attachment #305844 - Flags: review?(roc)
I just tried out the patch -- it leaves the drop-down on "Custom" when you hit cancel, which I don't think is an improvement.

I think the best thing to do would be to restore the drop-down to its prior state ("Title" in the situation from Comment 0), if that's possible.

If that solution's not possible, I think I'd lean slightly towards leaving things as they are now, rather than using this patch.  IMHO it's more clear to forcefully switch the dropdown to "--blank--", rather than to leave it at "Custom" with a blank actual custom string set, I think.  (I'm not 100% sure about that, though.)
(Assignee)

Comment 5

10 years ago
Theres no way to do that. And we dont want this happening every time you click cancel, e.g. you wish to edit an existing custom string, but you change your mind and click cancel. We dont want it going back to cancel in that case.
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> We dont want it going back to cancel in that case.

...dont want it going back to *--blank--*...

(In reply to comment #6)
> (In reply to comment #5)
> > We dont want it going back to cancel in that case.
> 
> ...dont want it going back to *--blank--*...
> 

Ah -- I agree with that, I think.
(In reply to comment #4)
> If that solution's not possible, I think I'd lean slightly towards leaving
> things as they are now, rather than using this patch.

Actually, I'm changing my mind about Comment #4 -- I like the patched behavior more than the current state of the code.

With the existing code, it seems like we're trying to make
  { - Select 'Custom', 
    - Set the custom text }
appear to one atomic operation.  *However*, we don't undo the atomic operation correctly when the user hits cancel.  We set the dropdown to --blank--, instead of restoring the previous state, which means we're not correctly reverting the 'atomic' operation.

After Michael's patch, we're essentially splitting that operation into *two* separate atomic operations:
 - { Select 'Custom' }
 - { Set the custom text }

With this two-atomic-operations interpretation, it makes sense that clicking "Cancel" would only revert the second operation (leaving the custom-text-string as it was before), but not revert the first (leaving the dialog set to "Custom").

I personally prefer the idea of having the whole thing be one atomic operation, if it's possible to get that working and reverting correctly. But short of that, FWIW, I think the patch makes more sense than the current behavior.
(Assignee)

Comment 9

10 years ago
Created attachment 305892 [details] [diff] [review]
Patch 2

This stores the previous index as another piece of GObject data, so we can revert to the original index.
Attachment #305844 - Attachment is obsolete: true
Attachment #305892 - Flags: superreview?(roc)
Attachment #305892 - Flags: review?(roc)
Attachment #305844 - Flags: superreview?(roc)
Attachment #305844 - Flags: review?(roc)
Attachment #305892 - Flags: superreview?(roc)
Attachment #305892 - Flags: superreview+
Attachment #305892 - Flags: review?(roc)
Attachment #305892 - Flags: review+
Comment on attachment 305892 [details] [diff] [review]
Patch 2

Would like to get this into b4, as it fixes a problem with the printing code on Linux.
Attachment #305892 - Flags: approval1.9b4?
Attachment #305892 - Flags: approval1.9?
Attachment #305892 - Flags: approval1.9b4?
Comment on attachment 305892 [details] [diff] [review]
Patch 2

a1.9=beltzner
Attachment #305892 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in widget/src/gtk2/nsPrintDialogGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.cpp,v  <--  nsPrintDialogGTK.cpp
new revision: 1.5; previous revision: 1.4
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
You need to log in before you can comment on or make changes to this bug.