181273 - "Set As Wallpaper" should not save in windows directory

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

Users may not have write access to the windows directory, and even if they do,
multiple users will all share the same wallpaper file :-(

Comment 1

[Frank Wein [:mcsmurf]]

http://lxr.mozilla.org/mozilla/source/xpfe/components/winhooks/nsWindowsHooks.cpp#985
and following had to be changed. You'll probably have to use
http://lxr.mozilla.org/mozilla/source/profile/src/nsProfileAccess.h#57 for the
profile string (there you can save the wallpaper)
Rest is up to you :)

Comment 2

[Zachary Echlin]

What directory should mozilla save the image to?

Comment 3

[Scott Jentsch]

When setting an image as the background, IE saves its wallpaper background to
the following location:

c:\Documents and Settings\*accountname*\Application Data\Microsoft\Internet Explorer

Where *accountname* is the name of the account that was active at the time.

This prevents accounts from stepping on each other as they would if they were
saving to the c:\windows directory, as Mozilla currently does.

Comment 4

[Larry Belan]

I just completed a corporate deployment of Moz 1.7 to 400 workstations, and this
was the FIRST user/useability issue I've run into.

I find it odd that Moz would want store the wallpaper in the windows folder,
let's just store it as "MozillaWallpaper.bmp" in the profile directory?  On our
Win 2000 systems, users do NOT have write access to the WINNT folder.  I one lab
user sets a risque picture as their wallpaper, and the next user sits down, they
might not be impressed at what they see.

This looks like a very simple patch to put into place, I've looked at the LXR
links, and I can see where things are going...could we just point it to the
User's Profile folder??  I DO wish I could code.  :(  Having a file there called
"MozillaWallpaper.bmp" IS pretty common sense.

I put a ? in the blocking 1.7.3 and aviary 1.0 branches.....  since my userbase
already found this, and a couple users would rather use a different browser. 
This IS a Useability issue.

Back to the patch:  can it be just as simple as replacing "winPath" located at:

http://lxr.mozilla.org/mozilla/source/xpfe/components/winhooks/nsWindowsHooks.cpp#991
http://lxr.mozilla.org/mozilla/source/xpfe/components/winhooks/nsWindowsHooks.cpp#995

with "profileDir" as used in:

http://lxr.mozilla.org/mozilla/source/profile/src/nsProfile.cpp#1554

I'm not a programmer, but looking through LXR, that's only clear reference to
the users profile folder location.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)

> with "profileDir" as used in:
> 
> http://lxr.mozilla.org/mozilla/source/profile/src/nsProfile.cpp#1554
> 
> I'm not a programmer, but looking through LXR, that's only clear reference to
> the users profile folder location.

I think that that "profile" refers to the directory in which the mozilla profile
is stored, not the Windows profile.

I think what's needed is something more like

 const char* profileDir  = PR_GetEnv("PROFILE_HOME");
 if (!profileDir) {
   profileDir = PR_GetEnv("HOME");
 }
 if (!profileDir) return NS_ERROR_FAILURE;

but I may be completely wrong, I'm kind of out of my league here.

See
http://lxr.mozilla.org/mozilla/source/browser/components/migration/src/nsDogbertProfileMigrator.cpp#336

Comment 6

[Lewis Jardine]

In my opinion, saving in the (mozilla) profile directory is probably a mistake -
if the user deletes her profile, the wallpaper will unexpectedly vanish.

In 2000 and XP, The Best Thing To Do would be to save the image in 'my
pictures'. It would probably be A Good Thing to save each wallpaer with a unique
name (the filename on the server perhaps, or 'mozilla wallpaper 1...n'?), so as
not to overwrite older wallpapers. Saving in a 'mozilla wallpapers' subdirectory
would probably be A Good Thing too.

In XP and 2k, the 'select wallpaper' tab of 'display properties' will populate
its list with any pictures that are in the user's 'my pictures'. In my opinion,
putting files where the OS expects them to be put would improve mozilla's
usability, especially with less experienced users.

'My Pictures' can be set per user (it defaults to %profiledir%/my documents/my
pictures), and its location can be found by reading
"HKEY_CURRENT_USER/software/microsoft/windows/currentversion/explorer/shell
folders/my pictures".


In my opinion, it would be best to do something like:

if (HKCU/.../my pictures exists), save in the directory HKCU/.../my pictures
describes //XP, 2k
else if (HKCU/.../personal ('my documents') exists) save in this directory //9x, NT4
else save in the windows directory //3.x?

Comment 7

[chris hofmann]

unlikely that we will get to this for aviary 1.0.  renominate if someone can get
a  resonable patch together.

Comment 8

[Larry Belan]

(In reply to comment #6)
> In my opinion, saving in the (mozilla) profile directory is probably a mistake -
> if the user deletes her profile, the wallpaper will unexpectedly vanish.
> 

I think the Moz profile directory SHOULD be the correct place.  The wallpaper IS
created by using Mozilla, so keep it there.  There's no searching for registry
keys, and IF the user deletes their moz profile...toss the wallpaper too.

Do the coders want to develop and maintain a UI for wallpapers?  Please let them
fix some other more serious issues. 

Just imagine if they save another pic as their wallpaper...isn't the first one
deleted?  Wallpaper is something that folks change for many reasons, so we
shouldn't get too involved with that.  Just let them set a wallpaper to a folder
that they can have access to, not a windows system directory.  Looking at this
blocking bug 260437, I hope that it receives some more review.

Comment 9

[Lewis Jardine]

(In reply to comment 8)
> I think the Moz profile directory SHOULD be the correct place.  The wallpaper IS
> created by using Mozilla, so keep it there.  
So why not save all downloaded files in the mozilla profile directory as well?
They're created by mozilla too.

> There's no searching for registry keys, 

It's not the user that needs to search for registry keys. User Agents are
supposed to jump through hoops in order that the user does not have to. These
registry keys are there so that the user does not need to set the same
preferences in many different applications, just to get them all saving
documents in the same place. 

Where the system provides a mechanism to find out where to save documents,
doesn't it make more sense to use it than to make up yet another incompatable
standard? 

> and IF the user deletes their moz profile...toss the wallpaper too.

Let's be clear on this: you believe the following scenario is [too uncommon to
bother considering]/[would not cause confusion to the user]?

User uses both mozilla and 'other browser'
User occasionally sets image as wallpaper using both mozilla and other browser
User deletes her mozilla profile, and: 
a) if mozilla set the wallpaper, it vanishes
b) if 'other browser' set the wallpaper, it remains in place

In my opinion, this bug does not block bug 260437; they are duplicates of each
other. Changing the wallpaper for all users and not being able to set the
wallpaper on a secure windows install are two symptoms of the same problem:
mozilla is not saving wallpapers in a sensible place. 

For a stopgap solution, how about having 'set as wallpaper' ask the user where
to save the image, just as 'save image as' does, and then have it perform the
additional step of setting the image as the wallpaper? This would stopgap the
problem, making the additional step of defaulting to the user's wallpaper (or
whatever place consenus is reached on) postponable.

Comment 10

[SiKing]

If we are voting on this ... I would also prefer the Mozilla profile directory
over the ...\My Pictures directory. My reasoning for this, is that the ...\My
Pictures directory is not guaranteed to exist at the time the wallpaper is being
created.

Comment 11

[Lewis Jardine]

(In reply to comment 10)
>I would also prefer the Mozilla profile directory over the ...\My Pictures 
> directory. My reasoning for this, is that the ...\My Pictures directory is not
> guaranteed to exist at the time the wallpaper is being created.

SHGetFolderPath* seems to address both this issue and the registry issue: 

call SHGetFolderPath with the CSIDL (CSIDL_FLAG_CREATE & CSIDL_MYPICTURES), and
it will return the path that pictures are supposed to be in, and create the
directory if it is not already present.



*
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/functions/shgetfolderpath.asp
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/enums/csidl.asp#CSIDL_MYPICTURES

Comment 12

[timeless]

Minimum operating systems: Internet Explorer 5.0

the code someone will have to write for this can not just use that, it will have
to provide a fallback, we do not require ie5.

Comment 13

[Lewis Jardine]

"SHGetFolderPath was obtained through SHFolder.dll, distributed with Microsoft
Internet Explorer 4.0 and later versions. SHFolder.dll always calls the current
platform's version of this function. If that fails, it will try to simulate the
appropriate behavior. SHFolder.dll continues to be included for backward
compatibility, though the function is now implemented in Shell32.dll."

shfolder.dll passes through to shell32.dll on platforms where it is present, and
comes up with the answer itself on platforms where it isn't. It first appeared
in IE4, which I believe mozilla requires*. Would this do?

*http://www.mozilla.org/products/firefox/system-requirements.html : 

-------------8<--------------------
Windows
Operating Systems

    * Windows 98
    * Windows 98SE
    * Windows ME
    * Windows NT 4.0
    * Windows 2000
    * Windows XP (Recommended)
-------------8<--------------------

NT4 could be a problem, though - IE4, IIRC, only appeared in service pack one? 

Comment 14

[timeless]

no this will not do. we're adding support for nt3.51. please take the word of
developers when they offer it instead of second guessing them.

Comment 15

[Lewis Jardine]

comment 12:
>Minimum operating systems: Internet Explorer 5.0
...
>we do not require ie5

comment 13:
>Internet Explorer 4.0 and later versions.

comment 14:
>please take the word of developers when they offer it instead of second
>guessing them.

It was not my intention to offend or contradict you; I assumed that you had
missed the part in the middle of the page where it outlined a workaround for
platforms which don't have IE5 but do have IE4*, and that you were ruling out
only IE5, rather than internet explorer in general. Would the current behaviour
be an acceptable fallback?

*I don't think it was an unreasonable assumption that Mozilla might require IE4
when it is compatible with Windows 98, but incompatible with Windows 95. IE4 is
present on the latest service packs of every operating system listed on the
'system requirements' page. 

Comment 16

[timeless]

no, the current fallback is not acceptable. note that neil (the reporter) is
among those of us working on adding support for nt3.51 which is a place where
you're very likely to not have write privs to the windows directory.

fwiw i believe ie4 was nt4sp4, but whatever. anyway. enough of this, requiring
ie is not something we will do.

work will probably involve:
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsDirectoryService.cpp

Comment 17

[Manuel Reimer]

Mozilla currently has an algorithm which detects the folder where the profiles
are stored and the registry.dat is created. Why don't simply use this folder and
place the pictures on the same level as registry.dat? I'm sure IE did the same
(I didn't use IE for several years so I don't know exactly :-) ) and placed the
pictures in "%appdata%\Internet Explorer". I don't aggree that the "My Pictures"
folder should be used. This is one of the first folders I delete after
installing windows as I have created my own folders (with different names) where
I keep my personal data.

Comment 18

[Nicholas Furgiuele]

Shouldn't this bug be fixed by the 1.0 release?

Its the kind of bug that will annoy quite a few people, especially the target
audience that is going to be using Firefox.

Heaps of people use the browser's "Set as Wallpaper" feature, and to have this
bug existing in the final release seems stupid. It will annoy people and turn
them back towards browsers like IE where the "Set as Wallpaper" feature works
properly.

Comment 19

[José Jeria]

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

Comment 20

[O. Atsushi (Torisugari)]

Attachment: patch (for suite)

Based on the suggestion Comment #17

On Windows XP, the default file path will be
C:\Documents and Settings\%USER_NAME%\Application Data\Mozilla\Mozilla
Wallpaper.bmp

Comment 21

[Frank Wein [:mcsmurf]]

Requesting review from a generic account doesn't make much sense (there is noone
behind [e-mail]).

Comment 22

[O. Atsushi (Torisugari)]

(In reply to comment #21)

Oh, I'm sorry for the bug spam.
Then, who should I ask the review?
neil or timeless?

Comment 23

[[e-mail]]

Comment on attachment 169607 [details] [diff] [review]
patch (for suite)

You should probably ask Dean Tessman or Ere Maijala for review.

>-    ::SystemParametersInfo(SPI_SETDESKWALLPAPER, 0, ToNewCString(winPath), SPIF_UPDATEINIFILE | SPIF_SENDWININICHANGE);
Eww, I'm glad you're removing this memory leak.

>+  brandName.Append(NS_LITERAL_STRING(" Wallpaper.bmp").get());
I know you based this on the original code but lines like this were supposed to
have got converted into AppendLiteral(" Wallpaper.bmp"); I wonder why this got
overlooked.

>+                         (PVOID) PromiseFlatCString(nativePath).get(),
nsCAutoString is already flat so you can just call get() directly.

Comment 24

[O. Atsushi (Torisugari)]

Attachment: patch (for suite)

(In reply to comment #23)
Neil, thank you very much.

Comment 25

[Adam Hauner]

(In reply to comment #23)
> >+  brandName.Append(NS_LITERAL_STRING(" Wallpaper.bmp").get());
> I know you based this on the original code but lines like this were supposed 
> have got converted into AppendLiteral(" Wallpaper.bmp"); I wonder why this 

Shouldn't be "Wallpaper" localizable string?

Comment 26

[:deanis74]

Comment on attachment 169609 [details] [diff] [review]
patch (for suite)

The string stuff looks much better to me, but I'll defer to Neil because I'm
weak on that.  Other than that it's pretty straight-forward.  r=me

Comment 27

[:deanis74]

> Shouldn't be "Wallpaper" localizable string?

Probably.  We can fix that in a new bug, since it's always been "Wallpaper".

Comment 28

[[e-mail]]

Comment on attachment 169609 [details] [diff] [review]
patch (for suite)

>-  NS_NewLocalFileOutputStream(getter_AddRefs(stream), path);
>+  NS_NewLocalFileOutputStream(getter_AddRefs(stream), aFile);
I just noticed that this line never gets error checked; as you're changing this
anyway you might as well slip a check in.

>   // build the file name
> 
>+  // A typical file path would be
>+  // C:\Documents and Settings\%USER_NAME%\Application Data
>+  // \Mozilla\Mozilla Wallpaper.bmp
I have removed the - lines to demonstrate the effect of the patch more clearly
and I would say that the original comment has become widowed. Please join
and/or reword these comments. Also if you're going to use %USER_NAME% you might
consider using %APPDATA% or %HOMEDRIVE%%HOMEPATH% instead.

sr=me with these nits fixed.

Comment 29

[O. Atsushi (Torisugari)]

Attachment: patch (for suite)

According as comment #28

Comment 30

[:deanis74]

Comment on attachment 169686 [details] [diff] [review]
patch (for suite)

carrying forward my r=.

Comment 31

[O. Atsushi (Torisugari)]

Thank you r and sr.
Cheked in by timely.
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/winhooks&command=DIFF_FRAMESET&file=nsWindowsHooks.cpp&rev1=1.55&rev2=1.56&root=/cvsroot
Please resolve bug.

Comment 32

[sbporter]

Any chance of this being fixed in Firefox?

Comment 33

[sbporter]

nvm, Firefox is covered by a seperate bug.