51988 - Installer's directory picker erases extra directory info when changing directory

Status: RESOLVED WORKSFORME

(SeaMonkey :: Installer, defect)

Description

[Brian 'netdragon' Bober]

I find this problem with installer packages for many programs. Basically, when 
you start, it says "/Mozilla". When you select Program Files from the directory 
listing, it changes it to "/Program Files", not "Program Files/Mozilla". 
Basically, it should take the directories being added (ie: the directories that 
don't exist already) and insert them after whatever new directory is picked.

If you have:

/Program Files/Communications/Mozilla Rules/Mozilla 
(Mozilla Rules and Mozilla don't currently exist in Communications) and you 
select from the directory box C:\, 

it should change to:

/Mozilla Rules/Mozilla

Then if you select Communications again (subdir. of Program Files), it should 
change to:

/Program Files/Communications/Mozilla Rules/Mozilla 

Then, if you delete /Mozilla Rules so it reads:

/Program Files/Communications/Mozilla 

That Mozilla should be carried when you change directories. Basically, it should 
keep track of whatever you added that doesn't already exist on the drive (or 
maybe just what you added).

Comment 1

[timeless]

Personally I agree, but I will not confirm.
These days, the correct response is: Invalid, New Bug: should use MSI.

Comment 2

[Brian 'netdragon' Bober]

I don't understand.

Comment 3

[timeless]

Confirming because there's a discrepancy.

if you have C:\program files\mozilla [default], and switch drives, it doesn't 
throw out \program files\mozilla, you get <drive letter>:\program 
files\mozilla.

I've seen a bunch of installers that mess this up.
However we do worse, if you type something into the edit box eg:
[original J:\Program Files\Mozilla]
Type D:\Program Files\Mozilla
Now change drive letters, or vice versa?  The result I get is that the 
drive/path and editbox get out of sync. Only double clicking in the folderview 
fixes this.

Brian, see dependency.

Comment 4

[Sean Su]

over to Curt.

Comment 5

[Brian 'netdragon' Bober]

I fixed this bug, but I still have to do more testing to make sure it doesn't 
cause any additional problems and to see if all the discrepancies have been 
removed. What I did was I saved the portion of the string that was after the 
portion of the string after the list box's index (gdwIndexLastSelected) in 
mozilla\xpinstall\wizard\windows\setup\dialogs.c

Comment 6

[Brian 'netdragon' Bober]

*SIGH* From comment #3, it still becomes out of sync. Have to fix that...

Comment 7

[Brian 'netdragon' Bober]

Personal Note: 

DLGS.H has the identifiers that aren't named in dialogs.c (such as 1092)

http://lxr.mozilla.org/seamonkey/source/xpinstall/wizard/windows/setup/dialogs.c#266

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/userinput/commondialogboxlibrary/aboutcommondialogboxes/openandsaveasdialogboxes.asp

There was talk about using explorer-style selection box, but before we ever do
that, we need to first of all make sure people can choose non-existant
directories without a hassle and also make sure that it is supported on all
target systems (i.e. international, Win NT 3.x, etc).

See also bug 51989. I'm going to fix this bug and that bug first. We don't plan
on fixing the issue I just mentioned immediately as it is rather risky and needs
to be done with caution and a lot of testing.

Comment 8

[Brian 'netdragon' Bober]

Unfortunately, there is no way I can tell to know when the listbox changes 
because of the selection in the disk combobox. This is quite a dilemma.

I think we are going to have to redo the directory picker. :-/

I found this KB Article:
http://support.microsoft.com/default.aspx?scid=KB;en-us;q179497

People in #mozilla said there are many directory picker examples online and 
some include fallbacks for compatibility.

Comment 9

[Brian 'netdragon' Bober]

Attachment: patch

This is the fix. Notice the attempt here is to not munge the extra path info
the user enters into the Edit Box even when changing directories in the
listbox. I also fixed the drive issue.

I talked to a lot of computer-savvy people (developers, network admins, etc)
and they agreed installers should not munge the data when you double-click on a
new folder in a drive selection or directory selection listbox.

Notice the new directory choosers using SHBrowseForFolder often make you create
the directory one-by-one. That can be a pain in the neck. In this case, you can
just navigate to the parent directory you want, press OK and *presto*, its all
created at once. If you remove a portion of the "mozilla.org" portion or
whatever, the rest of your extra data is maintained.

Superior to what is out there, I only wish it used a more up-to-date common
dialog.

Screenshots to come....

Comment 10

[Brian 'netdragon' Bober]

Attachment: Screen Shot 1: Starting point

Comment 11

[Brian 'netdragon' Bober]

Attachment: Screen Shot 2: User selects "Communications"

Comment 12

[Brian 'netdragon' Bober]

Attachment: Screen Shot 3: User selects drive F:\

Comment 13

[Brian 'netdragon' Bober]

Attachment: Screen Shot 4: User erases extra path info

Comment 14

[Brian 'netdragon' Bober]

Attachment: Screen Shot 5: User erases all text in edit box

Comment 15

[Brian 'netdragon' Bober]

Attachment: Screen Shot 6: User selects drive C:\

Comment 16

[Brian 'netdragon' Bober]

Attachment: HTML file showing all images.

Comment 17

[Sean Su]

sorry for taking so to get to this patch.  I'll try to finish the review by tonight.

Comment 18

[Brian 'netdragon' Bober]

Sean: Have you had a chance to try out the patch yet?

Comment 19

[Sean Su]

brian, sorry.  have not had a chance. I got swamped that night with problems
with a gre patch that's due by 01/21 midnight, but your patch is on the top of
my things to look at when I get the time.

Comment 20

[Sean Su]

Comment on attachment 110390 [details] [diff] [review]
patch

Brian, nice job on this patch.

However, if I happen to select a drive letter that does not have a preselected
subpath, the resulting path will have an extra '\' after the drive letter.
  ie. if I select my CDRom drive letter (E:), where the opened folder in E: is
the root, it will show "E:\\mozilla.org\Mozilla" in the Directories edit box.
*notice the extra '\' after the ':'

That should be easy to fix.

Comment 21

[Brian 'netdragon' Bober]

Attachment: New patch. Fixed issue sean mentioned

Fixed that.

Comment 22

[Brian 'netdragon' Bober]

Comment on attachment 114818 [details] [diff] [review]
New patch. Fixed issue sean mentioned

Asking Sean Su for review+

Comment 23

[Brian 'netdragon' Bober]

Attachment: New patch

Comment 24

[Brian 'netdragon' Bober]

Comment on attachment 115458 [details] [diff] [review]
New patch

Sean said you are good at finding any multibyte issues ::Crossing fingers:: ;-)

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 115458 [details] [diff] [review]
New patch

>+static HWND hBrowseDlg;                 // Browse dialog HWND for use in functions not called from

Please change this to ghBrowseDlg or other g* name to clarify things later.

>+void ExtractListBoxPath(void) 
  ...

>+    SendDlgItemMessage(hBrowseDlg, lst2, LB_GETTEXT, 0, (LPARAM)szBufIndex);
>+    strcpy(gListBoxPath, szBufIndex);

Why not just get the drive letter directly into gListBoxPath?

>+    //Convert the drive letter to upper case
>+    _strupr(gListBoxPath);

Not that it matters in the case of a drive letter, but "CharUpper()" is the
habit you want to get into. Will index 0 ever be anything *but* a single drive
letter, such as a network drive name? In that case you MUST use CharUpper.

But why do you need to uppercase it at all?

>+        if (dwLoop < dwIndex) AppendBackSlash(gListBoxPath, sizeof(gListBoxPath));

Please make this two lines.

>+void ModifyEditBox(void)
>+{
>+    char szPath[MAX_BUF];
>+    char szExtraPath[MAX_BUF];
>+    char * szStart;
>+
>+/*  We want to carry over the extra data contained within IDC_EDIT_DESTINATION when the 
>+    user selects something within lst2. We don't want to carry over anything more or anything 
>+    less. We don't want to munge anything. Not carrying it over would be munging the data. */

dialogs.c is already a horrible mix of comment styles but this doesn't match
either of the leading contenders for multi-line comments. Please pick either
C++ "//" style or indent the openning "/*" at the same level as the code and
use a leading "*" on each line with the closing "*/" on a line of its own. And
shorten the lines to <= 80 chars.

[I'm amused by the pre-existing comment around 2400 that has two lines of //
bracketing four lines of /*...*/]

>+    GetDlgItemText(hBrowseDlg, IDC_EDIT_DESTINATION, szPath, sizeof(szPath) - 1);

You don't need the -1 here and a couple places later, GetDlgItemText() counts
the null. If you're worried about an overflow string being truncated with no
null termination then you need to explicitly add a null to the end of the
buffer, not just leave room for one.

>+        /* The strstr check above left off the trailing backslash in case the user
>+           added text to the edit box but forgot a backslash before it, i.e.
>+           "F:mozilla.org\mozilla". Therefore, we need to get rid of the leading 
>+           backslash for szStart if extant */
>+        if (*szStart == '\\') szStart++; 

Same nits about comment and if styles.

>+    ExtractListBoxPath();
>+    strcpy(szPath, gListBoxPath);
>+    if (*szExtraPath != '\0') 
>+    {
>+        AppendBackSlash(szPath, sizeof(szPath));
>+        lstrcat(szPath, szExtraPath);
>+    }
>+    SetDlgItemText(hBrowseDlg, IDC_EDIT_DESTINATION, szPath);
>+}

Couldn't you avoid the need for szPath and extra strcpys by operating directly
on gListBoxPath?

>-    case LB_SETCURSEL:
>-      gdwIndexLastSelected = (DWORD)wParam;
>+/*  Examining the messages showed that this is sent only after the data is updated. Unfortunately,

Comment style again.

Comment 26

[Brian 'netdragon' Bober]

Attachment: New patch

> Please change this to ghBrowseDlg or other g* name to clarify things later.
fixed
>Why not just get the drive letter directly into gListBoxPath?
fixed
>Not that it matters in the case of a drive letter, but "CharUpper()" is the
>habit you want to get into. Will index 0 ever be anything *but* a single drive

>letter, such as a network drive name? In that case you MUST use CharUpper.
I don't know 100% if it will be anything but a single drive letter, therefore I
used CharUppr. I'm pretty sure network drive names have just letters.
>But why do you need to uppercase it at all?
Convention
>Please make this two lines.
Done


Fixed comment and if styles.



>You don't need the -1 here and a couple places later, GetDlgItemText() counts
>the null. If you're worried about an overflow string being truncated with no
>null termination then you need to explicitly add a null to the end of the
>buffer, not just leave room for one.
Fixed. I wasn't sure if it included the null character and didn't want to cause
a buffer overrun. Its true that if the entire string was used and it didn't
include the null automatically or in the size passed to it, that it would have
caused the string to have no null character. Thanks for pointing that out.
>Couldn't you avoid the need for szPath and extra strcpys by operating directly

>on gListBoxPath?
No, because gListBoxPath only holds the path selected, not the extra path
information. Using it for both and truncating it later would be confusing for
someone reading the code.

Comment 27

[Brian 'netdragon' Bober]

This has not been reviewed again and its now 5 months later.

Comment 28

[Sean Su]

Comment on attachment 118434 [details] [diff] [review]
New patch

r=ssu

Comment 29

[Matthias Versen [:Matti]]

Seamonkey and Firefox are using a new NSIS based installer. resolving this old bug, please reopen if you still get this with the new installer