Created attachment 25749 [details] [review]
Suggested patch. Description follows.

The above patch addresses these issues:

- gtk_file_system_win32_get_volume_for_path: Include the backslash. 
Otherwise double-clicking a vole didn't open it.

- gtk_file_system_win32_get_parent: Workaround for bug #137316. If 
GTK+ 2.4.1 requires GLib 2.4.1, and 137316 is fixed in 2.4.1, this 
isn't needed. Make more like Unix version, require absolute pathname, 
avoid duplicating and freeing string. 

- gtk_file_folder_win32_get_info: Allow NULL path, like the Unix 
version.

- filename_get_info: No initial-dot-files-are-hidden convention on 
Win32. Just use the FILE_ATTRIBUTE_HIDDEN.

- filename_is_root: Accept also forward slash after the colon.

Tor,

I've taken your patch and made a few more changes to produce an
updated patch. One comment: "c:" is currently passed by
filename_is_root, so I've added explicit code to cope with this in
get_info but I'm not clear we couldn't just change filename_is_root.

The following problems remain:

* Can't mount floppy drives
* Bad error message on accessing an empty CD drive
* Bad error message on creating a directory (even though it's created)
* Removing a CD with the CD drive volume open causes meltdown.

My changes:

Fri Mar 19 20:30:03 2004  J. Ali Harlow  <ali@juiblex.co.uk>

        * gtk/gtkfilesystemwin32.c (list_volumes): Cope with the
        theoretical possibility of more than 26 logical drives.
        (get_folder): Check that it's actually a directory.
        (get_display_name): Check for empty volume name and do
        the right thing if non-ASCII characters are present.
        (volume_for_path): Fix a check that could never work.
        (get_info): Cope with roots with no directory specified
        in the root name (eg., "c:").
        (bookmarks_serialize): Now actually removes bookmarks.

Created attachment 25814 [details] [review]
Updated patch

I see one problem in Ali's patch: I don't think we can use 
GetVolumeInformationW() directly as it isn't implemented on Win9x. (I 
don't know whether it is exported from kernel32.dll on Win9x but 
always returns failure, or not present at all.) We don't want to 
break GTK+ on Win9x deliberatly, do we?

The code should check at run-time if on Win9x, and in that case use 
GetVolumeInformationA() instead, and call g_locale_to_utf8() on the 
returned volume name. Or maybe do that all the time; it can't be very 
common to have volume names not expressable in the system codepage? 
GetVolumeInformationW(), if still used on NT-type systems, should be 
looked up with GetProcAddress().

Gtk+ is already broken on Win95 systems that don't have the unicode
extensions. Error message as follows:

The LIBGTK-WIN32-2.0-0.DLL file is
linked to missing export KERNEL32.DLL:GetFileAttributesExA

I think we loose nothing, and gain a lot of simplicity, by accepting
that anybody who is running Win95 will need to install the unicode
extensions (I believe Win98 should be fine out of the box).

> Gtk+ is already broken on Win95 systems 

Ouch, didn't know that. That has not been intentional, I think. As 
long as it isn't absolutely necessary, I think we shouldn't 
unconditionally use API that isn't available on Win95. Does 
gtkfilesystemwin32.c even currently really use any of the information 
available only through the GetFileAttributesEx(), and not through 
GetFileAttributes()?

If we at some point (2.6.0?) decide to drop any hope of running on 
Win9x, I think we then could as well go all the way and use the wide-
char versions of the API all over gdk/win32, use Unicode-enabled 
windows (or whatever the correct terminology is), etc?

Yes, the information from GetFileAttributesEx is used, but it could be
picked up from other places to maintain Win95 compatibility.
GetFileSize() can return the size and GetFileTime() can return the
modification time but these need the file to be opened.
FindFirstFile() would probably be the best bet under Win95 since this
also returns the flags that get_info() needs.

2004-03-20  Hans Breuer  <hans@breuer.org>

	* gtk/gtkfilesystemwin32.c : applied the undisputable and
	required [due to recent gtkfilesystem internal api semantic 
	changes] part of patches to fix bug #137543 (Tor Lillqvist,
	J. Ali Harlow)

- afaik win95 (not win9x!) support of gtk is broken for years (to 
  me it is very reasonable to use functions introduced with win98
  unconditional, like GetFileAttributesEx). There are other functions
  in the dependency stack already, e.g. InterlockedExchange*() which
  simply are not available for win95 (iirc with win98 the 80386
  support was dropped and they require a 486 at least, or was it a
  pentium? )
- dropping win98/winme support does not appear to be necessary (at
  least not until my little notebook breaks into pieces, i.e. as 
  long as somebody cares ;-)

BTW: my commit does include the usage of GetVolumeInformationW() 
with fallback, but it appears to simply work on win98se ...

Stuff I left out of the patches :
- showing dot files/directories wasn't done with all previous 
  versions of gtk on windows, why should it now ?
- the g_file_test(DIRECTORY) stuff did produce lots of those 
  floating warnng messages from above (didn't look yet why ;)
- addding a workaround for a soon to fix bug #137316

Regarding further problems with GtkFileSystemWin32 IMO there should 
be extra bug reports for the various flaws, like bug #137815

Hans,

I don't think there's any point falling back on GetVolumeInformation
if we explicitly link with GetVolumeInformationW. IMHO we either need
to accept a Win98 dependency or use GetProcAddress as Tor suggested.

Win98 compatability is important for me as well since our (AVRC)
current generation of systems use it. Win95 is too old for me to worry
about and too hard to maintain compatibility with.

I'll just note here that you left out a lot more than you mentioned in
your comments. I'll produce a revised patch on Monday.

Those tests for directories are needed now.  gtkfilechooserdefault's
check_is_folder depends on the testing.

What, exactly, was the problem?

I found that even with bug #137316 fixed, you can crash 
testfilechooser by double-clicking A: (with no floppy present).

This is because g_filename_to_uri() requires an absolute path, 
otherwise it returns NULL. gtk_file_system_win32_path_to_uri() gets 
called when printing the error message about no floppy in drive, and 
it calls g_filename_to_uri(). A minimal fix is to make 
gtk_file_system_win32_volume_get_base_path() return the path 
including the backslash, not just the drive letter and colon. Will 
commit that now. 

If there is no longer any way to crash the file chooser, should we 
close this bug? For remaining issues separate bugs can be opened.

Tor,

I can open seperate bugs for most remaining issues but I think we need
to decide whether to drop the fallback to GetVolumeInformation or to
use GetProcAddress as part of this bug. I don't see any advantage in
the current implementation. If you make a decision I'm happy to make
the relevant changes.

I would also like to see empty volume labels displayed as simply the
drive name. The current implementation displays these as " (D:\)"
which I think is ugly.

I think part of the solution is to get rid of the idea that "a:" is
an absolute path.  It isn't.  For example, IMHO,
filename_is_root should return "FALSE" for "a:"; anything else is
just asking for trouble.

I've filed bug #137940 to track the issue with more than 25 drives,
bug #137942 to track the issue with get_info("c:"), bug #137943 to
track the issue with removing bookmarks and bug #137945 to track the
issue with bad error messages on directory creation.

That leaves the question of whether :get_folder should check if the
supplied path represents a folder or not, which I'll leave for Morten
to file if he so desires.

It also leaves the question of display names for which I attach an
updated patch. This patch:
* Ignores empty volume labels
* Assumes that GetVolumeInformation will fail of GetVolumeInformationW
does
* catches a small memory leak in the current implementation.

Created attachment 25885 [details] [review]
Proposed fix

Patch is no-go.

For the call to W functions like GetVolumeInformationW, you need to use
G_N_ELEMENTS, not sizeof.

Thanks, Morten. Updated patch to also address this follows.

Created attachment 25887 [details] [review]
Proposed fix

2004-03-22  J. Ali Harlow  <ali@juiblex.co.uk>

        * gtk/gtkfilesystemwin32.c
        (gtk_file_system_win32_volume_get_display_name): Ignore empty
        volume labels; assume that GetVolumeInformation would fail if
        GetVolumeInformationW does; catches a small memory leak;
        pass the buffer size to GetVolumeInformationW in wide
        characters instead of bytes. Fixes bug #137543