246616 - moz-icon should be able to draw special desktop/my documents folder icons

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

moz-icon currently draws generic icons for the Virtual Namespace special folders
"Desktop" and "My Documents"... it should draw the special ones Windows uses for
that purpose.

I have a patch to moz-icon that does this:

http://www.bengoodger.com/software/mb/downloads/desktopicon.png

My patch only does Desktop and My Documents. For XUL2.0 we probably want to
massage moz-icon a bit further to support some kind of system for mapping
directory service defs to special icons.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

This one doesn't crash if the url doesn't contain a valid file path

Comment 3

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

how do the moz-icon urls for desktop/my documents look?

Comment 4

[[e-mail]]

biesi, here's a moz-icon: URL that already works for My Computer.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

they just detect paths that match the CSIDL_DESKTOP or CSIDL_PERSONAL path at
present - allowing for file hierarchies. 

It also probably makes sense to add a distinct, "keyed" method of accessing all
of the available values, especially for the locations that do not have file
system mapping like Desktop and My Documents - like My Computer and My Network
Places. But that's a different bug.

This one is fixed, br & trunk. 

Comment 6

[David G King]

This seems to have broken my Cygwin build. I'm not sure why as I have
<shlobj.h>, which contains SHGetSpecialFolderPath, but gcc says it is undefined.

Comment 7

[[e-mail]]

My Network Places is not a bug, it's
moz-icon:file:///C:/My%20Network%20Places.%7B208D2C60-3AEA-1069-A2D7-08002B30309D%7D/?size=16

Comment 8

[[e-mail]]

And I forgot to mention that My Documents has always been available via
moz-icon:file:///C:/My%20Documents.%7B450D8FBA-AD25-11D0-98A8-0800361B1103%7D/?size=16

Comment 9

[[e-mail]]

>This seems to have broken my Cygwin build. I'm not sure why as I have
><shlobj.h>, which contains SHGetSpecialFolderPath, but gcc says it is
>undefined.
Because it's compiling for Windows 95, which doesn't have
SHGetSpecialFolderPath. Reopening.

Comment 10

[Malcolm Rowe]

[Hmm - is sr= no longer required for checkins to libpr0n?]

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #9)

> Because it's compiling for Windows 95, which doesn't have
> SHGetSpecialFolderPath. Reopening.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/functions/shgetspecialfolderpath.asp

"The Microsoft® Internet Explorer 4.0 Desktop Update must be installed for this
function to be available."

So this checkin creates a dependency on IE?

Comment 12

[David G King]

As written, this patch relies on functionality that is added when IE v4 (or
later) is installed.

It's difficult to determine if this is a big problem as Windows builds are still
created using MSVC v6.

Comment 13

[David G King]

A potential solution, although a horrible hack, is to add "#define _WIN32_IE
0x0400" to the begining of
/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp

I'm testing this at the moment to see if it compiles.

Comment 14

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

I don't think mozilla should depend on msie 4 installed. mozilla should be able
to run on systems that don't have it.

Comment 15

[David G King]

My idea in Comment #13 isn't any good. The code compiles, but doesn't run. So, I
guess we are back to relying on IE being installed for
Mozilla/Firefox/Thunderbird to compile and run.

Comment 16

[Stephen Walker]

Can http://groups.yahoo.com/group/mingw32/message/1546 be used instead of
SHGetSpecialFolderPath to get MinGW/Cygwin building again?

Comment 17

[[e-mail]]

Or we could always use our own directory service, which does all this already.

Comment 18

[Lorenzo Colitti]

Attachment: to make it compile under cygwin

this patch implements the suggestion in comment #16.

It compiles under cygwin, but I have no idea if it works.

Comment 19

[Lorenzo Colitti]

Comment on attachment 151754 [details] [diff] [review]
to make it compile under cygwin

The patch seems to work as it should. Requesting review.

Comment 20

[David G King]

Thunderbird compiles and runs with the patch on Win XP/cygwin/MinGW.
Testing Firefox now, and maybe Sunbird.

Comment 21

[David G King]

Firefox compiles and runs with the cygwin patch. A few Assertions, but nothing
that looked too bad.

Comment 22

[Scott MacGregor]

Comment on attachment 151754 [details] [diff] [review]
to make it compile under cygwin

This patch looks ok to me. I know Neil also ran into this build bustage. Neil,
feel free to take over the r= if this patch does indeed correctly fix the
bustage for you.

Comment 23

[[e-mail]]

Comment on attachment 151754 [details] [diff] [review]
to make it compile under cygwin

I noticed that this patch duplicates the call to SHGetSpecialFolderLocation, 
so I reordered the code to avoid the duplication.

Comment 24

[[e-mail]]

Fix checked in.

Comment 25

[nivoside]

http://webtools.mozilla.org/bonsai/cvslog.cgi?file=mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp&rev=AVIARY_1_0_20040515_BRANCH&root=/cvsroot
AVIARY_1_0_20040515_BRANCH ?

It seems that the bird of a birdhouse can also feel hungry :-)

Comment 26

[Lorenzo Colitti]

Hmm, yes, this should be checked in to the aviary branch as well, since it stops
FF from working under Win95 and NT4...

Comment 27

[nivoside]

Please reopen.

And I want a patch to check in at a branch.

Comment 28

[Žiga Sancin]

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

Comment 29

[Žiga Sancin]

Reopen, the patch would be welcome on the aviary branch. I ran into problems, so
I  filed a new bug (just for the aviary branch), but then I found this one. You
can find the duplicate here -> bug #250150

I'll try to build aviary-builds on my computer with the trunk-patch in the meantime.

Comment 30

[Žiga Sancin]

The patch works also with the latest source pulled from the aviary branch.

Comment 31

[Jean-Marc Desperrier]

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

Comment 32

[Seb Perez-D]

This bug is marked resolved but the fix has not been checked in the branch. The
description of this bug does not fit what is seen by branch users under Win NT4:
namely, Firefox  doesn't start (see bug 251341 for description of what happens).

Please reopen, or create a new bug with the correct description.

Comment 33

[Michael Lefevre]

ok, to avoid confusing this bug, I'm going to confirm bug 253101 to cover builds
not starting on NT.

Comment 34

[Steffen Wilberg]

Comment on attachment 151754 [details] [diff] [review]
to make it compile under cygwin

This should fix the smoketest blocker bug 253101.