Request for Patchreview

[André Hentschel]

Can someone please review my patch http://www.winehq.org/pipermail/wine-patches/2009-June/074195.html.
Its about the Installer-Button in Appwiz.cpl

Thanks
Best Regards André Hentschel

[Juan Lang]

Hi André,
> Can someone please review my patch
> http://www.winehq.org/pipermail/wine-patches/2009-June/074195.html.

I have a couple minor comments:

+
+   IDS_INSTALL_FILTER, "Installation
Program\0*Instal*.exe;*Setup*.exe;*.msi\0Programs\0*.com;*.cmd;*.exe\0All
Files (*.*)\0*.*\0\0"

Nit:  the blank line isn't needed.

Nit:  "Installation Program" should be "Installation Programs".

The filter looks strange to me.  At least in Windows, it isn't usual
to see a wildcard in the name part of the filter other than *.
Usually it's something like *.extension.  If you think it's important
to do it this way, at least change it to Install*.exe, rather than
Instal*.exe.  Also, are .com and .cmd installers common in Windows?
Usually I see .exe, and sometimes .msi.

In my opinion, something like the following would be better:
"Installation Programs (*.exe;*.msi)\0*.exe;*.msi\0Programs
(*.exe)\0*.exe\0Microsoft Installer Packages (*.msi)\0*.msi\0All
Files(*.*)\0*.*\0\0"
--Juan

[André Hentschel]

Juan Lang schrieb:
> Hi André,
> > Can someone please review my patch
> > http://www.winehq.org/pipermail/wine-patches/2009-June/074195.html.
>
> I have a couple minor comments:
>
> +
> +   IDS_INSTALL_FILTER, "Installation
> Program\0*Instal*.exe;*Setup*.exe;*.msi\0Programs\0*.com;*.cmd;*.exe\0All
> Files (*.*)\0*.*\0\0"
>
> Nit:  the blank line isn't needed.
>
> Nit:  "Installation Program" should be "Installation Programs".

Thanks for reply. I will correct that.
> The filter looks strange to me.  At least in Windows, it isn't usual
> to see a wildcard in the name part of the filter other than *.
> Usually it's something like *.extension.  If you think it's important
> to do it this way, at least change it to Install*.exe, rather than
> Instal*.exe.  Also, are .com and .cmd installers common in Windows?
> Usually I see .exe, and sometimes .msi.

First thing, i tested it on some Win-Versions, and it took everything like Instal*.exe and *Setup*.exe
Really, name a exe-file Instalblahblah.exe and the installer-dialog will show it.

Second thing, the .com and .cmd belongs to normal Program-selection, not to the installers. but we can reduce it to .exe, of course.
> In my opinion, something like the following would be better:
> "Installation Programs (*.exe;*.msi)\0*.exe;*.msi\0Programs
> (*.exe)\0*.exe\0Microsoft Installer Packages (*.msi)\0*.msi\0All
> Files(*.*)\0*.*\0\0"

Thats apart of the way Windows does. As i said, i spent some time testing the Windows-filter, because it didnt show every .exe

[Juan Lang]

> First thing, i tested it on some Win-Versions, and it took everything like
> Instal*.exe and *Setup*.exe
> Really, name a exe-file Instalblahblah.exe and the installer-dialog will
> show it.
>
> Second thing, the .com and .cmd belongs to normal Program-selection, not to
> the installers. but we can reduce it to .exe, of course.

Fair enough.  We tend to mimic Windows behavior.  I hadn't checked
this myself.  Note that it still should be Install*.exe rather than
Instal*.exe, at least in English.
--Juan

[André Hentschel]

Juan Lang schrieb:
> > First thing, i tested it on some Win-Versions, and it took everything like
> > Instal*.exe and *Setup*.exe
> > Really, name a exe-file Instalblahblah.exe and the installer-dialog will
> > show it.
> >
> > Second thing, the .com and .cmd belongs to normal Program-selection, not to
> > the installers. but we can reduce it to .exe, of course.
>
> Fair enough.  We tend to mimic Windows behavior.  I hadn't checked
> this myself.  Note that it still should be Install*.exe rather than
> Instal*.exe, at least in English.
> --Juan

My tests worked with Instal*.exe files on english Windows, too.
I bet it is because of the instal~1.exe writing. (8.3 and 6##.3)
So i correct some things: http://www.winehq.org/pipermail/wine-patches/2009-June/074266.html

Best Regards André Hentschel

[Juan Lang]

Thanks André.
> My tests worked with Instal*.exe files on english Windows, too.
> I bet it is because of the instal~1.exe writing. (8.3 and 6##.3)

Ah, right.  Very good, leave this as it is then.

Sorry for the sloppy review, there are a couple more things I'd like
to ask about:
> My tests worked with Instal*.exe files on english Windows, too.
> I bet it is because of the instal~1.exe writing. (8.3 and 6##.3)
> So i correct some things:
> http://www.winehq.org/pipermail/wine-patches/2009-June/074266.html

+static WCHAR FileNameBufferW[MAX_PATH];
+static WCHAR FileTitleBufferW[MAX_PATH];
+static const WCHAR openW[] = {'o','p','e','n',0};

Why are these static?  openW is probably fine as a static, as it's
const, but the other two are never used beyond the InstallProgram
scope, and therefore shouldn't be static.  As a general rule, you
should make variables' scope as narrow as possible.

One minor enhancement along these lines:
+    SHELLEXECUTEINFOW sei;

this isn't used in the outer block, only in the if
(GetOpenFileNameW(&ofn)) block.  Therefore it shouldn't be declared
until the block in which it's used.

Style:
+    if (GetOpenFileNameW(&ofn)) {
the existing code puts braces on a newline.  Please do the same.

Finally,
+    return;
an empty return statement is superfluous.  Please omit it.
--Juan

[Dmitry Timoshkov]

"André Hentschel" <nerv(a)dawncrow.de> wrote:
> Fair enough.  We tend to mimic Windows behavior.  I hadn't checked
> this myself.  Note that it still should be Install*.exe rather than
> Instal*.exe, at least in English.
> --Juan
>
> My tests worked with Instal*.exe files on english Windows, too.
> I bet it is because of the instal~1.exe writing. (8.3 and 6##.3)

There is no such a thing as '6##.3' file names under Windows.
'~1' is just a part of short file name, and a long file name
should still match the 'Install*.exe' wildcard.

-- 
Dmitry.

[Ben Klein]

2009/6/16 Dmitry Timoshkov <dmitry(a)codeweavers.com>:
> "André Hentschel" <nerv(a)dawncrow.de> wrote:
> > Fair enough.  We tend to mimic Windows behavior.  I hadn't checked
> > this myself.  Note that it still should be Install*.exe rather than
> > Instal*.exe, at least in English.
> > --Juan
> >
> > My tests worked with Instal*.exe files on english Windows, too.
> > I bet it is because of the instal~1.exe writing. (8.3 and 6##.3)
>
> There is no such a thing as '6##.3' file names under Windows.
> '~1' is just a part of short file name, and a long file name
> should still match the 'Install*.exe' wildcard.

I wish application devs believed this too. I've had issues with (even
Microsoft) apps using short filenames, at worst hard-coding "PROGRA~1"
for "Program Files", and the app failing to run because on my system
it happened to be some other number ... :)

[André Hentschel]

Dmitry Timoshkov schrieb:
> "André Hentschel" <nerv(a)dawncrow.de> wrote:
> > Fair enough.  We tend to mimic Windows behavior.  I hadn't checked
> > this myself.  Note that it still should be Install*.exe rather than
> > Instal*.exe, at least in English.
> > --Juan
> >
> > My tests worked with Instal*.exe files on english Windows, too.
> > I bet it is because of the instal~1.exe writing. (8.3 and 6##.3)
>
> There is no such a thing as '6##.3' file names under Windows.
> '~1' is just a part of short file name, and a long file name
> should still match the 'Install*.exe' wildcard.

Please believe me, on W2K you can rename some file to instalWine.exe and it will be displayed as Installer.
so it must bei something like instal*.exe
I must admit, the upper case version Instal*.exe is because i am german. Will change this.