123197 - gtk2 needs to have the filepicker hooked up

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Christopher Blizzard (:blizzard)]

Need to hook up the filepicker with gtk2.

Comment 1

[Brian Tarricone]

i was looking into this, and, well, frankly, the GtkFileSelector kinda sucks. 
it doesn't even have a dropdown box to set a file pattern to filter the list. 
one can be added, of course, since GtkFileSelector is just a GtkDialog. 
blizzard: is this what you had in mind for this?  if so, i'll work on this,
unless you or someone else has started on it.

Comment 2

[Christopher Blizzard (:blizzard)]

I'm didn't have any specific plans, except it would be nice to use the native
filepicker where possible.  Go right ahead, if you're motivated.

Comment 3

[Brian Tarricone]

sounds good to me.  i'll let you know how it goes ^_~

Comment 4

[Brian Tarricone]

sorry i haven't gotten to this sooner...  anyway, i've been playing with the
gtk2 filepicker, and figured out how to add i filter box to the window, but
there are a couple problems.  first of all, there's no way to easily filter the
file listbox.  the gtk api call that i thought would work,
gtk_file_selection_complete() actually applies a filter to both the file _and_
directory list boxes.  i honestly can't think of one useful reason to do this.
also, there's the problem of handling a filter with multiple definitions, such
as "*.jpg; *.gif."  *_complete() doesn't handle this as-is.  i suppose i could
write a routine to filter the file list box based on the filter, using the
semicolon as a separator for multiple extensions in a filter.  in theory tho,
the gtk team will at some point redesign the filepicker, and who knows what it
will look like then. (i'm actually not sure if it is the gtk filepicker or gnome
filepicker that is going to be redesigned.)

so what do we think?  should i go ahead and write up a filtering routine, leave
out the filter box and use the GtkFileSelection as-is, or scrap using a native
dialog and keep moz's internal one for now?  i could also look for a third-party
filepicker that uses gtk2.  thoughts?

Comment 5

[Brian Tarricone]

Attachment: gtk2 filepicker hookup patch

ok, went ahead and implemented my own filtering code.  it appears to work ok,
tho i wasn't really sure how to test it aside from clicking on filters and
navigating to a bunch of directories.

i think i caught all possible mem leaks and checked for all appropriate error
conditions, but it should be looked over by someone other than me, certainly.

apply with -p0 in widget/src/gtk2

Comment 6

[Louie Zhao]

Brian, I tested your patch on recently trunk, it can compile, but when mozila
start up, the "Registering nsWidgetGtk2Module components" seems has problem. Can
you test your patch on recent trunk, thanks a lot!

In addition, I have some question about File Picker:

1. What the File Picker do? It seem to be concern the "File->OpenFile...". Under
mozilla/widget/src, gtk module hasn't implemented nsIFilePicker, but
"File->OpenFile..." does work. gtk2 module are same.

2. other modules such as mac, windows implement nsIFilePicker, which is really
used by other part of mozilla.Then why gtk(gtk2) can work properly without them ?

I am so confused by these question. I will be appreciated if someone can give me
some lights.

Comment 7

[Brian Tarricone]

sorry i've taken a while to get back to this...

the filepicker is indeed the file->save as... or file->open file... box that
pops up.  the one currently in use by gtk and gtk2 builds is part of mozilla's
set of XP widgets.  it's just a bit nicer and more consistent to use native
widgets and dialogs where possible, hence this bug.

as for your runtime issues...  my current CVS isn't compiling, despite
yellow/green status on tinderbox, so i guess i'll try re-pulling the tree or
something, which i don't have time to do right now - perhaps tomorrow.

Comment 8

[Brian Tarricone]

Attachment: new gtk2 filepicker hookup patch - incl. missing bits

ok, i figured out what the problem was.  in my first patch i forgot the
modification to widget/src/xpwidgets/Makefile.in, to include
nsBaseFilepicker.cpp for gtk2 builds.  updated the patch in places for the
current trunk (a few trivial differences).  it builds and works fine for me. 
give it a try and let me know what you think.

* NOTE: this patch needs to be applied with 'patch -p0' in widget/src this
time, NOT in widget/src/gtk2 *

(p.s. is there some kind of standard for patches, e.g. should all be made from
the root of the mozilla tree, etc.?)

Comment 9

[Andrew Sobala]

Just a note: AFAIK there will be a new file chooser in GTK 2.2, currently under
development. New API and totally revamped UI.

Comment 10

[Biswapesh Chatterjee]

Couple of points:
1) GTK2 file dialog will remain API and ABI-compatible with GTk2.2.
2) Filtering is already avaibale - try writing "*.txt" in the input field and
pressing <TAB>.

Comment 11

[Brian Tarricone]

while this is obviously not the place to debate this (perhaps i'll open a bug on
gtk's bugzilla at some point), i feel the need to offer at least a vague defence
for the work i've done.

> 2) Filtering is already avaibale - try writing "*.txt" in the input field and
> pressing <TAB>.

i find this filtering method totally unnaceptable and unintuitive.  while it is
reminiscent of shell-type tab-completion, that paradigm does not really fit with
a GUI so well:

1) it is not immediately apparent how to "get back" the entries in the file box
that disappeared after a tab-press.  deleting just one character should
re-filter the box, but it does not- another tab-press is needed.
2) this new "tab-completion" combines shell-type tab-completion with
shell-glob-completion as well (e.g., typing "*.txt" at a bash prompt and hitting
<tab> does not give you a list of all files ending in .txt).
3) this also filters the directory list box.  it's quite confusing and unnerving
(at least to me) to see the directories in a file dialog disappear -
tab-completion in a shell may only show directories that match, but a filter box
never touches the directory list.

i think the basic problem of why this doesn't translate is because in a shell
setting, tab completion shows you possibilities when you have none, where this
new GUI-based tab completion attempts to narrow your possibilities when you have
many.  yes it's useful, but i don't think it's intuitive.

the gtk2 method also rules out the possibility of doing what is possible on
other platforms - allowing mozilla to programmatically restrict the file
display.  if the possible filters are "*.swf" and "*.html" then there is no way
to display, for example, a .pdf file.  some people may not like this, but this
is the reality of how it works across platforms.  often the filter box has a
"*.*" option to combat this.  depending on platform you can enter your own
filter into the file selection box, and hit enter.  but the gtk2 box is totally
inconsistent.

my work brings the filepicker in the gtk2 port of mozilla closer to being
consistent with mozilla's filepicker on other platforms.  the debate rages on as
to whether mozilla should be consistent across platforms or try to look and
behave like other apps one might find on the particular platforms.  the work
i've done supports the former (which i don't always agree with, anyway).

on the other hand, i failed to take into account the tab-completion on the gtk2
file box, so my guess is that if you start doing tab-complete while using my
filter box or vice-versa, odd things may happen.  if there is still any interest
in this bug for the time being, i'll look into it.

Comment 12

[Andrew Sobala]

Can I clarify my earlier comment (in light of recent developments): A new file
selector _is_ being written, but didn't make Gtk/GNOME 2.2 by a long way. It
will be implemented in a future release.

Comment 13

[Aleksander Adamowski]

I vote for using Mozilla's builtin filepicker until GTK filepicker stops sucking. 

It seriously lacks multiple column list view with sorting, and column
hiding/unhiding.

It basically sucks visually, too (IMHO). If anything, this bug should not delay
work on Mozilla's GTK2 port in any part.

Comment 14

[chris hofmann]

blizzard is this one that you're working on for the desktop push?

Comment 15

[Christopher Blizzard (:blizzard)]

Yeah, it will be.  The API for the filepicker has just settled down now, so I
should be working on this soonish.

Comment 16

[Michael Hendy (Hendikins)]

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

Comment 17

[chris hofmann]

this would be a good one to get for 1.8a...  what are the chances?

Comment 18

[Christopher Blizzard (:blizzard)]

Pretty good, but pango comes first.

As a note, you can probably just grab the code out of epiphany.  Marco said that
it was fine with him if we just took it.

Comment 19

[(not currently active) Ted Mielczarek]

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

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Preliminary patch at:
http://people.redhat.com/caillon/patches/mozilla/gtk2-file-chooser/gtk2-file-chooser.patch

Todo:
- Dynamically load symbols

Anyone care to test this a bit?

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

Note: the patch in its current state requires gtk+ 2.4 to build with.  I am
working on making that not the case, but just thought I'd note that :-)

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch, dynamically loading symbols

Mirrored at http://people.redhat.com/caillon/patches/mozilla/gtk2-file-chooser/

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 153796 [details] [diff] [review]
Patch, dynamically loading symbols

blizzard gave r= over IRC.

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 153796 [details] [diff] [review]
Patch, dynamically loading symbols

(blizzard actually gave r+sr) I also showed this to a few gtk people and aside
from the "ewww dlsym, you're friggin crazy" comments had no complaints with it.

Comment 25

[tor]

Looking at the code, it appears to unconditionally use the gtk filepicker
if gtk-2.4 is available.  Shouldn't we allow the theme to pick whether they
want a native or XUL filepicker?  For some themes native is a natural fit,
but for others the native look will clash horribly.

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Tor, if you want to do that, let's do that on all platforms.

Comment 27

[Mike Shaver (:shaver emeritus)]

Is the most-recently-attached patch what landed on the trunk?

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

Shaver: yes

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Supplementary patch

- Fixes issue for clobber builds where the file chooser would not always get
registered.  Unfortunately, this still can not be guaranteed to work because of
how XPCOM (mis)handles multiple components registered with the same contract
ID.  See bug 253136.
- The native widget component factory constructor now handles the logic for
which implementation to choose.  This allows me to remove the proxy logic from
the implementation itself.
- Remembers the last selected directory between invocations.
- Removes some debug stuff I forgot to remove.
- Gets rid of the static nsString cruft that got accidentally added while I was
copying over stuff from other implementations.
- Fix a shutdown "leak".

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: The right patch

This is the one.

Comment 31

[Christopher Aillon (sabbatical, not receiving bugmail)]

Going to close this one out.  The main issue for Mozilla AIUI is really just bug
253136.  I'll entertain other issues in new bug reports.

Comment 32

[Ganesh]

Attachment: Patch for Aviary branch

Comment 33

[Ganesh]

please ignore my above attachment. 

Comment 34

[Ganesh]

Attachment: Thsi is based on dynamic loading symbold for Aviary

Comment 35

[Ganesh]

Attachment: from attachement #155072 (for aviary)

Comment 36

[Mike Shaver (:shaver emeritus)]

Comment on attachment 155805 [details] [diff] [review]
Thsi is based on dynamic loading symbold for Aviary

Chris: can you review these patches for aviary-safety?

Comment 37

[Mike Shaver (:shaver emeritus)]

Comment on attachment 155807 [details] [diff] [review]
from attachement #155072 (for aviary)

Chris: can you review these patches for aviary-safety?

Comment 38

[Christopher Aillon (sabbatical, not receiving bugmail)]

These look fine.  I'd rather you just roll them up into one patch than separate
them really.  But according to bryner, it breaks static builds because of the
NS_ERROR_FACTORY_REGISTER_AGAIN stuff.  So I think we need a better solution.

Comment 39

[Ganesh]

Attachment: Combined both the patches in one.

Comment 40

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 156020 [details] [diff] [review]
Combined both the patches in one.

Oh, make sure you get the tweak I added in the final checkin for default button
handling.  You should be able to safely copy the version of the trunk
nsFilePicker.cpp over to the branch

Comment 41

[Jaap A. Haitsma]

I get the following errors in the console when I run the nightly of Thunderbird
of Aug 19, which has the gtk2 filepicker, when I the file open dialog appears


(Gecko:4114): GLib-GObject-WARNING **: invalid cast from `GdkWindow' to `GtkWindow'

(Gecko:4114): Gtk-CRITICAL **: file gtkwindow.c: line 1883
(gtk_window_set_transient_for): assertion `parent == NULL || GTK_IS_WINDOW
(parent)' failed

Comment 42

[Sanjiv Agarwala]

Attachment: this fixes the issue of gtk file picker not coming up

(update of attachment 156020 [details] [diff] [review])
This is the update of attachment 156020 [details] [diff] [review] plus the nsFilePicker.[cpp,h] which
[e-mail]  mentioned to take from Mozilla Trunk + it solves the issue
of gtk file picker not comming up.

Comment 43

[R.K.Aa.]

If this checkin caused blocker bug 258347: Please consider backing it out.
The new filepicker barely works on gtk2 builds anyway. If a file has been
created on disk after i have used filepicker once, i have to quit mozilla and
restart to make the filepicker "see" the file. All in all, Mozilla on gtk2 has
become unusable for basic file related tasks. This is bad.

According to Bienvenue, neither he nor Seth Spitzer are the right persons for
bug 258347. If the checkin isn't backed out, whoever caused this serious
regression should take on the assignment.

Comment 44

[Scott MacGregor]

Hey Chris Aillon, can you help drive the fix for the remaining regressions this
bug caused? It looks like someone posted a patch on 8/24 to try to fix the
problem with the gtk2 file picker not coming up for mailnews when adding an
attachment. Is this the right fix? Can you drive it into the tree if so? This is
a pretty nasty regression that we shouldn't still be waiting to get fixed 2
weeks after the fact. Thanks!

Comment 45

[Christopher Aillon (sabbatical, not receiving bugmail)]

Can someone assign the right bug to me (read: not this one), and post a trunk
diff of what is required to make this work for people?  I get a sexy file
chooser on my trunk build, so unsure what is going on.

Comment 46

[Scott MacGregor]

Blocker Bug #255900 has been re-assigned to you Chris :) 

But the possible patch to fix the regression is listed here in this bug. 

Comment 47

[R.K.Aa.]

I wouldn't call the filepicker sexy.. it's next to useless here. For one, there
is absolutely no indication  of what are files and what are directories. It's
just a list of names. If i didn't know the directory structure beforehand, I
wouldn't have been able to use it.

Comment 48

[R.K.Aa.]

i found out that the filepicker has icons to indicate folders and files if i use
mozilla under gnome. But not under KDE. Meaning for instance that the filepicker
mozilla uses now is themed by Gnome, not Gtk. It also means that filetypes
aren's seen properly, so the user can't filter on filetypes. This makes Mozilla
needlessly difficult to use under KDE.

Also: It is much more difficult to use under Gnome as well,  because the editor
field in the filepicker has been removed and can not be revoked in any way.
This means I can't type or paste in a file-URL into the filepicker. I will have
to maneuver with innumerable clicks and drags to get to a file.

I strongly recommend this checkin be backed out untill the Gtk filepicker is
pure Gtk, and as usable as the one Mozilla had untill this checkin. (I.E. Files
again can be written to it)

Comment 49

[Mike Shaver (:shaver emeritus)]

Press Ctrl-L to reveal the file-path entry widget, and please don't add any more
comments to this bug regarding filepicker widget advocacy.

Comment 50

[Boris Zbarsky [:bzbarsky]]

This patch caused smoketest blocker bug 264210 when the GTK2 filepicker is used...