Closed Bug 241282 Opened 16 years ago Closed 15 years ago

When choosing an alternative location via "Browse...", the Installer "defaults" to ns_temp subfolder

Categories

(Firefox :: Installer, defect, trivial)

x86
Windows XP
defect
Not set
trivial

Tracking

()

VERIFIED FIXED

People

(Reporter: robertjm, Assigned: jhenry)

References

Details

Attachments

(3 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040421 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040421 Firefox/0.8.0+

When installing either Firefox or Thunderbird, using the Installer build, if you
do not choose the default install location of c:\program files\Mozilla Firefox,
it defaults to the ns_temp subfolder, and you have to navigate to the folder you
want to install to.

Reproducible: Always
Steps to Reproduce:
1. Run installer program
2. When asked if you want to install to default directory, choose Browse
3. browse window opens up defaulting to c:\Documents and Settings\root\Local
Settings\Temp\ns_temp subfolder

Actual Results:  
I had to navigate to the folder location I wanted to install to.

Expected Results:  
It should open at the c:\program files folder, rather than some hidden
subfolder. That makes it a lot easier to find the folder I want.

This happens with the Installer version of both Firefox and Thunderbird, from
4/21/04
Status: UNCONFIRMED → NEW
Depends on: 225505
Ever confirmed: true
Flags: blocking1.0?
Same issue with 5/2/04 builds.
-ing will take patch
Flags: blocking1.0? → blocking1.0-
Attached image Screenshot of bug
Screenshot
Attached patch Proposed patch (obsolete) — Splinter Review
This reads the path to the user's Program Files directory from the registry and
uses that, rather than the temp dir. I've tested it successfully on my WinXP
system under both admin and limited user accounts, but I can't guarantee it
works on other setups (particularly Win 98). Feedback would be appreciated.

There's some other stuff which was just me fixing wonky spacing in the
function, but if that's unwanted I can roll up a patch without that.
Comment on attachment 153121 [details] [diff] [review]
Proposed patch

Ben, could you please look at this when you get the chance?
Attachment #153121 - Flags: review?(bugs)
*** Bug 251422 has been marked as a duplicate of this bug. ***
*** Bug 259558 has been marked as a duplicate of this bug. ***
*** Bug 262955 has been marked as a duplicate of this bug. ***
Summary: When choosing an alternative installation location, the Installer "defaults" to ns_temp subfolder → When choosing an alternative location via "Browse...", the Installer "defaults" to ns_temp subfolder
*** Bug 242435 has been marked as a duplicate of this bug. ***
Nominating for 1.1 because of its evilness and because a patch exists. Oh, and
because this bug makes it so easy to install to temp folder...
Flags: blocking-aviary1.1?
Blocks: 282239
No longer blocks: 282239
Blocks: 282239
*** Bug 283584 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1+
(In reply to comment #10)
> Nominating for 1.1 because of its evilness and because a patch exists. Oh, and
> because this bug makes it so easy to install to temp folder...

Can someone add "dataloss" keyword?
Attachment #153121 - Flags: review?(bugs) → review?(benjamin)
Flags: blocking-aviary1.1+ → blocking-aviary1.1-
Is this patch working or does it need updating?
Comment on attachment 153121 [details] [diff] [review]
Proposed patch

This should not be using the registry directly. The correct windows APIs are
SHGetSpecialFolderLocation or SHGetSpecialFolderPath, but the *Location version
is available on plain-jane win95 while the *Path version is not: could you
please resubmit this patch using SHGetSpecialFolderLocation?
Attachment #153121 - Flags: review?(benjamin) → review-
Assignee: bugs → jhenry
QA Contact: bugzilla → benjamin
Attached patch Revised patch (obsolete) — Splinter Review
Like this? The patch seems to compile and work for me.
Attachment #153121 - Attachment is obsolete: true
Attachment #186875 - Flags: review?(benjamin)
Comment on attachment 186875 [details] [diff] [review]
Revised patch

SHGetSpecialFolderLocation can fail; please check it for success before calling
SHGetPathFromIDList, and if it fails manually set something useful like
GetCurrentDirectory or "C:\".
Attachment #186875 - Flags: review?(benjamin) → review-
Attached patch Third try (obsolete) — Splinter Review
Error checking is good. How does this look now, Benjamin?
Attachment #186875 - Attachment is obsolete: true
Attachment #187051 - Flags: review?(benjamin)
Comment on attachment 187051 [details] [diff] [review]
Third try

Calling SHGetPathFromIDList after SHGetSpecialFolderLocation fails doesn't
sound good.

if (GetFolderLocation != S_OK ||
  SHGetPathFromIDList != S_OK)
  strncpy...
Attachment #187051 - Flags: review?(benjamin) → review-
Attached patch Next...Splinter Review
You're right...thanks for sticking with this.
Attachment #187051 - Attachment is obsolete: true
Attachment #187071 - Flags: review?(benjamin)
Attachment #187071 - Flags: review?(benjamin)
Attachment #187071 - Flags: review+
Attachment #187071 - Flags: approval-aviary1.1a2+
Thanks for the review comments and approval. I have no CVS access, could someone
please check this in when you have the chance? Thanks.
checked-in.

Checking in toolkit/mozapps/installer/windows/wizard/setup/dialogs.c;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/wizard/setup/dialogs.c,v  <--
 dialogs.c
new revision: 1.34; previous revision: 1.33
done
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
This patch broke my build. I get a compiler error in dialog.c now complaining
about CSIDL_PROGRAM_FILES not being defined anywhere.

c:/build/trees/dbg/mozilla/toolkit/mozapps/installer/windows/wizard/setup/dialog
s.c(620) : error C2065: 'CSIDL_PROGRAM_FILES' : undeclared identifier
make[6]: *** [dialogs.obj] Error 2
make[6]: Leaving directory `/cygdrive/c/build/trees/dbg/mozilla/toolkit/mozapps/
installer/windows/wizard/setup'
make[5]: *** [libs] Error 2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix bustageSplinter Review
Attachment #187224 - Flags: review?(benjamin)
Attachment #187224 - Flags: approval-aviary1.1a2?
Scott:

What are you using the build environment? MinGW?
# beast and pacifica are successed.
patrocles is red due to this checkin so not all of the official build machines
are building.

As for me, I use MS studio 6.0 to build.

This patch fixed the bustage for me.
backed-out.

# I will sleep soon, sorry.
The check-in appears to have also broken the Win32 XULRunner tinderbox, so be
sure to keep an eye on that tinderbox when you land this patch again.
Comment on attachment 187224 [details] [diff] [review]
fix bustage

Is this constant not in older SDK headers? Does that mean it is not honored in
some versions of Windows? In any case, r+a=me to land this along with the other
patch.
Attachment #187224 - Flags: review?(benjamin)
Attachment #187224 - Flags: review+
Attachment #187224 - Flags: approval-aviary1.1a2?
Attachment #187224 - Flags: approval-aviary1.1a2+
checked-in.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
I tested Firefox trunk build(2005062606)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050626
Firefox/1.0+

custom install folder in dialogue is
Actual Results:
C:\Program Files

Expected Results:
C:\Program Files\Deer Park Alpha1
(In reply to comment #30)
> I tested Firefox trunk build(2005062606)
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050626
> Firefox/1.0+
> 
> custom install folder in dialogue is
> Actual Results:
> C:\Program Files
> 
> Expected Results:
> C:\Program Files\Deer Park Alpha1


If first install, "C:\Program Files\Deer Park Alpha1" may not be exist.
I think that always using "C:\Program Files" is better solution.
*** Bug 251424 has been marked as a duplicate of this bug. ***
Status: RESOLVED → VERIFIED
No longer depends on: 225505
*** Bug 228327 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.