Re: shell32: remove unneeded parsing of SHELLEXECUTEINFO.lpFile [try 2]

[Ilya Basin]

Hi! Sorry for being annoying, will you accept my patch?
http://www.winehq.org/pipermail/wine-patches/2010-March/086009.html

shell32: remove unneeded parsing of SHELLEXECUTEINFO.lpFile [try 2]

[Paul Vriens]

On 03/22/2010 07:59 AM, Ilya Basin wrote:
> Hi! Sorry for being annoying, will you accept my patch?
> http://www.winehq.org/pipermail/wine-patches/2010-March/086009.html

Hi Ilya,

That's not up to us.

Whenever you sent a patch to wine-patch everybody is free to comment (or 
sent a reply that they agree in some specific cases). It's still however 
up to Alexandre whether a patch is accepted and committed.

I can't spot anything obviously wrong with your patch. I must admit that 
this SHELL32_execute function is huge so it could be worthwhile (in the 
future) to split things out of there.

The only thing I'm wondering is why this big piece was added in the 
first place (2004) but that in itself is of course no guarantee it was 
needed.

Is it possible to create more tests or is everything possible already 
catered for by that one test?

Re[2]: shell32: remove unneeded parsing of SHELLEXECUTEINFO.lpFile [try 2]

[Ilya Basin]

PV> The only thing I'm wondering is why this big piece was added in the
PV> first place (2004) but that in itself is of course no guarantee it was 
PV> needed.

PV> Is it possible to create more tests or is everything possible already
PV> catered for by that one test?
Add a new verb that has '%*' in it. I put the reply from M.Fuchs in
the bottom. It seems that it was related to .lnk handling.

But currently I'm more worried by
http://bugs.winehq.org/show_bug.cgi?id=19385
( the 'wine start' launcher does not open MS Office documents that
have spaces in their path )

If the <verb>\command doesn't contain '%', e.g.
'"C:\PROGRA~1\MICROS~2\OFFICE11\WINWORD.EXE" /n /dde'
it's argyfied incorrectly

Now there's no test verb without '%', I think I should add it
and add related tests.

MF> well you see it's a long time ago and I don't know the exact reason for this code. But it may help if I show you the first occurence of this code snippet I could get hold of in the ReactOS repository find at http://svn.reactos.org/svn/reactos/trunk/reactos/lib/shell32/shlexec.c?revis... .
MF>  
MF>  
MF>   927     /* The following code is needed for example to resolve a shortcut
MF>   928        to control panel applet "Keyboard", since this is accessed using
MF>   929        "rundll32.exe shell32.dll,Control_RunDLL %1,%*" with a command line
MF>   930        parameter received from ISF_ControlPanel_fnGetDisplayNameOf(). */
MF>   931     if (!*szCommandline) {
MF>   932   if (*szApplicationName == '"') {
MF>   933       LPCSTR src = szApplicationName + 1;
MF>   934       LPSTR dst = fileName;
MF>   935 
MF>   936       while(*src && *src!='"')
MF>   937     *dst++ = *src++;
MF>   938 
MF>   939       *dst = '\0';
MF>   940 
MF>   941       if (*src == '"')
MF>   942     for(++src; isspace(*src); )
MF>   943         ++src;
MF>   944 
MF>   945       strcpy(szCommandline, src);
MF>   946   }
MF>   947   else
MF>   948   {
MF>   949       LPSTR space, s;
MF>   950       char buffer[MAX_PATH], xlpFile[MAX_PATH];
MF>   951 
MF>   952       LPSTR beg = szApplicationName;
MF>   953       for(s=beg; space=strchr(s, ' '); s=space+1) {
MF>   954     int idx = space-szApplicationName;
MF>   955     strncpy(buffer, szApplicationName, idx);
MF>   956     buffer[idx] = '\0';
MF>   957 
MF>   958     if (SearchPathA(sei->lpDirectory, buffer, ".exe", sizeof(xlpFile), xlpFile, NULL))
MF>   959     {
MF>   960         /* separate out command from parameter string */
MF>   961         LPCSTR p = space + 1;
MF>   962 
MF>   963         while(isspace(*p))
MF>   964       ++p;
MF>   965 
MF>   966         strcpy(szCommandline, p);
MF>   967         *space = '\0';
MF>   968 
MF>   969         break;
MF>   970     }
MF>   971       }
MF>   972 
MF>   973       strcpy(fileName, szApplicationName);
MF>   974   }
MF>   975     } else
MF>   976   strcpy(fileName, szApplicationName);
MF>   977 
MF>   978     lpFile = fileName;
MF>   979 
MF>   980     if (szCommandline[0]) {
MF>   981         strcat(szApplicationName, " ");
MF>   982         strcat(szApplicationName, szCommandline);
MF>   983     }
MF>   984 
MF>   985     retval = execfunc(szApplicationName, NULL, sei, FALSE);
MF>   986     if (retval > 32)
MF>   987     {
MF>   988   /* Now, that we have successfully launched a process, we can free the PIDL.
MF>   989   It may have been used before for %I command line options. */
MF>   990   if (tmpPidl)
MF>   991       SHFree(tmpPidl);
MF>   992 
MF>   993         TRACE("execfunc: retval=%d sei->hInstApp=%p\n", retval, sei->hInstApp);
MF>   994         return TRUE;
MF>   995     }
MF>  
MF> Please note the comment at the top, which may lead you to a valid test case for this algorithm in SHELL_execute(). This text remained there until revision 7522, where it was replaced by the current text "separate out command line arguments from executable file name". The code itself was extended to handle more cases and ported to UNICODE strings until now.
MF> May be it is now superflous because of some other code changes (just a guess: FindExecutable may be a candidate). But may it is still necessary to correctly handle some rare cases when executing shell/explorer related calls with complicated command line arguments.
MF>  
MF>

MF> Regards,
MF>  
MF>     Martin