264210 - save as, webpage complete doesn't work with gtk2 filepicker

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

using a cvs gtk2 build (20040925) and fedora core 2, i can't save webpage, complete.
I can select it after expanding 'browse for other folders'. It is in the filters
list. It is even selected by default. But when saving, it still only saves the
html page, not the images. There also is no dir created.

This is a smoketest blocker, B.27.

guess: the selected filter isn't handed out to the caller. I think i noticed
that before.

Comment 1

[Boris Zbarsky [:bzbarsky]]

One further note... the fact that this smoketest blocker took so long to catch
mostly indicates to me that the GTK2 filepicker has gotten pretty much no testing.

Comment 2

[Asa Dotzler [:asa]]

The fix for this is to pref back to the old filepicker until we make the GTK2
one better (or the filepicker people do). 

Tracy smoketest the trunk daily but missed this because his linux system was
getting the old filepicker in the runtime test. 

The smoketest is to save a webpage complete. This is currently doable in both
filepickers, it's the saving as other format that's broken in the gtk2 picker. 

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

> The smoketest is to save a webpage complete. This is currently doable in both
> filepickers, it's the saving as other format that's broken in the gtk2 picker. 

no, saving the complete webpage is what i can't do. IF is select it, it still
saves only the html, not the whole webpage. So, it is a smoketest blocker.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Luis cares deeply about this bug.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Apparently this works on the SUSE-patched branch firefox, Luis can maybe tell us
exactly which source base was used (1.0PR?).  The SUSE patch is attached to a
bug I can't find right now, hmm.

Comment 6

[Luis Villa]

(In reply to comment #1)
> One further note... the fact that this smoketest blocker took so long to catch
> mostly indicates to me that the GTK2 filepicker has gotten pretty much no 
> testing.

We're testing it in a decently sized beta program through novell, including
several thousand internal users. But this works here, so we're probably not
using exactly the same code as trunk is. 

Comment 7

[Boris Zbarsky [:bzbarsky]]

Re-upgrading to blocker per comment 3.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #0)
> using a cvs gtk2 build (20040925) and fedora core 2, i can't save webpage,
complete.

https://bugzilla.mozilla.org/show_bug.cgi?id=255900

------- Additional Comment #37 From Christopher A. Aillon 2004-09-27 10:58 PDT
[reply] -------

Attachment 160219 [details] [diff] checked in to trunk.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Still doesn't work with build 20041014, so seems like that patch didn't fix this.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

I would suggest making sure this works on other platforms then.  The GTK
specific code in our RPMs and the trunk matches right now.  It could be
something else.  It works for me on the branch, and I can't build a trunk right
now because of bug 234035 though it looks like a patch is materializing.

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

using a gtk1 (debug) build from the same tree works just fine.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Unsure what to say.  It works for both Novell and Red Hat.  And I am using the
same source that the trunk is.  Perhaps a problem in the GTK+ shipped with FC2?

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

This must be something in my profile.
It works with my test-profile, but not my normal profile. (using the same build)

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

mvl: random guess: maybe the pref for last selected file picker filter index?

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

in my browse-profile, i had browser.download.save_converter_index set to 1. In
my test profile, it was 0. Setting it to 0 in the browse-profile makes it work.
But now, i can't save as html only anymore. It saves as complete webpage no
matter what.
So my guess still stands: the filepicker doesn't pass the selected filter back
to the caller. Reading the source confirms this: mSelectedType is only set in
SetFilterIndex, not after closing the dialog.

Comment 16

[Philip K. Warren]

Attachment: Patch v1

This patch does the following two things:

1) Save the selected filter after showing the dialog, so the correct behavior
of the "Save Page" is restored.
2) Use the currently selected filter (mSelectedType) when showing the dialog,
so the save_converter_index pref works right with the GTK2 filepicker build.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: From my RPMs

Forgot to post this.  This combines this fix with the fix for the "needs to
prompt before overwriting" bug.

Comment 18

[Philip K. Warren]

Comment on attachment 162880 [details] [diff] [review]
From my RPMs

>Index: widget/src/gtk2/nsFilePicker.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/gtk2/nsFilePicker.cpp,v
>retrieving revision 1.6
>diff -d -u -p -r1.6 nsFilePicker.cpp
>--- widget/src/gtk2/nsFilePicker.cpp	12 Oct 2004 07:00:59 -0000	1.6
>+++ widget/src/gtk2/nsFilePicker.cpp	20 Oct 2004 06:07:45 -0000
>+  const PRUnichar *formatStrings[] =
>+  {
>+    NS_LITERAL_STRING("File").get()
>+  };

Shouldn't the filename be used here, instead of the English string "File"?

Also, one thing that this patch is missing from mine is that it doesn't set the
filter based on mSelectedType (so the save_converter_index_pref doesn't work
properly). See the call to _gtk_file_chooser_set_filter in my patch.

Comment 19

[Philip K. Warren]

One more thing:

confirm_overwrite_file checks if the file is non-NULL and if the file exists,
even though that test is already performed in nsFilePicker::Show.

You could also probably use GtkMessageDialog instead of creating your own dialog
to save a little code in confirm_overwrite_file.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 162880 [details] [diff] [review]
From my RPMs

+    NS_LITERAL_STRING("File").get()

you can't do that, this only works if the used compiler supports wide strings;
otherwise you will be referencing freed memory here.

however... why not use file.leafName here?

+  dialog = gtk_dialog_new_with_buttons (NS_ConvertUCS2toUTF8(title).get(),

mind making this UTF16 rather than UCS2? (several times in this patch)

Comment 21

[Philip K. Warren]

Attachment: Patch v2

- Uses file leaf name in the confirmation dialog.
- Uses GtkMessageDialog instead of custom dialog.
- Selects the right filter based on mSelectedType.

I still see some issues which need to be worked out with the GTK2 file picker
dialog, but this addresses the major problems of no confirmation before
overwriting a file and the filter list not functioning properly. I will open
new bugs for the problems I found in my testing.

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 163473 [details] [diff] [review]
Patch v2

>+    // Set the initially selected filter
>+    if (mSelectedType == i)
>+      _gtk_file_chooser_set_filter (GTK_FILE_CHOOSER(file_chooser), filter);

Use braces.  r=caillon

Comment 23

[Philip K. Warren]

Fixed (with braces added as requested in comment 22).

Checking in Makefile.in;
/cvsroot/mozilla/widget/src/gtk2/Makefile.in,v  <--  Makefile.in
new revision: 1.35; previous revision: 1.34
done
Checking in nsFilePicker.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsFilePicker.cpp,v  <--  nsFilePicker.cpp
new revision: 1.8; previous revision: 1.7
done

Comment 24

[Philip K. Warren]

*** Bug 259220 has been marked as a duplicate of this bug. ***