Re: New uninstaller

[Dmitry Timoshkov]

"Jonathan Ernst"  wrote:
> With some help I'll be able to improve it (more unicodification, etc.)
> when I'll come back.

You can't really convert to unicode partially, you have to do it in one go.
That not only will simplify things a lot, but also will save you a lot of time.
> - for (i=0; i < numentries; i++)
> - {
> -
>    len = WideCharToMultiByte(CP_UNIXCP, 0, entries[i].descr, -1, NULL, 0, NULL, NULL); 
> -
>    descr = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> -
>    WideCharToMultiByte(CP_UNIXCP, 0, entries[i].descr, -1, descr, len, NULL, NULL); 
> -
>    printf("%s|||%s\n", entries[i].key, descr);

There is no need to allocate len * sizeof(WCHAR) bytes, len works just fine.
> +int wmain(int argc, char *argv[])

wmain takes 'WCHAR *argv[]' list.

All remaining problems are caused by the fact that now argv[] array
is passed in unicode but you still handle argv[] strings as ASCII and
pass them around to internal functions which accept 'char *' not 'WCHAR *'
strings.

New uninstaller

[Jonathan Ernst]

Le lundi 28 février 2005 à 09:10 +0800, Dmitry Timoshkov a écrit :
> "Jonathan Ernst"  wrote:
> > With some help I'll be able to improve it (more unicodification, etc.)
> > when I'll come back.
>
> You can't really convert to unicode partially, you have to do it in one go.
> That not only will simplify things a lot, but also will save you a lot of time.

Ok, I will continue to work on it then. I'm sorry to require that much
help, but my first idea was just to add internationalization to the
uninstaller, then I was asked to move from window to dialog and then
from ascii to unicode and that's another cup of tea ;-)
> > - for (i=0; i < numentries; i++)
> > - {
> > -
> >    len = WideCharToMultiByte(CP_UNIXCP, 0, entries[i].descr, -1, NULL, 0, NULL, NULL); 
> > -
> >    descr = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > -
> >    WideCharToMultiByte(CP_UNIXCP, 0, entries[i].descr, -1, descr, len, NULL, NULL); 
> > -
> >    printf("%s|||%s\n", entries[i].key, descr);
>
> There is no need to allocate len * sizeof(WCHAR) bytes, len works just fine.

Ok Fixed.
> > +int wmain(int argc, char *argv[])
>
> wmain takes 'WCHAR *argv[]' list.

Fixed as well.
> All remaining problems are caused by the fact that now argv[] array
> is passed in unicode but you still handle argv[] strings as ASCII and
> pass them around to internal functions which accept 'char *' not 'WCHAR *'
> strings.

I tried to fix every char->WCHAR I though was worth it; please have a
second look at it.

The problem now with unicode is that when I try to strstr my token with
the element of the list (entries[i].descr) it works only with the first
letter (before to use unicode it was working very well).
You can see the same problem (one letter .descr) when using the
unintaller with --list.

The second problem is that since I use unicode for the argvs, whatever
argv I give I get the --list branch.

Thanks everyone for your help.

New uninstaller

[Dmitry Timoshkov]

"Jonathan Ernst"  wrote:
> I tried to fix every char->WCHAR I though was worth it; please have a
> second look at it.
>
> The problem now with unicode is that when I try to strstr my token with
> the element of the list (entries[i].descr) it works only with the first
> letter (before to use unicode it was working very well).
> You can see the same problem (one letter .descr) when using the
> unintaller with --list.
>
> The second problem is that since I use unicode for the argvs, whatever
> argv I give I get the --list branch.

Your main mistake is that you are trying to handle ASCII strings with "old"
APIs, you can't do that. Unicode requires a completely different set of APIs,
and win32 provides one.
> -
>    /*
> -
>    TELL ME: IS IT STILL NEEDED WITH UNICODE ???
>    len = WideCharToMultiByte(CP_UNIXCP, 0, entries[i].descr, -1, NULL, 0, NULL, NULL); 

As I have said it already, you need to convert unicode strings to the encoding
of the underlying system before printing strings on console. CP_UNIXCP is supposed
to do exactly that task.
> -
>    descr = HeapAlloc(GetProcessHeap(), 0, len);
> -
>    WideCharToMultiByte(CP_UNIXCP, 0, entries[i].descr, -1, descr, len, NULL, NULL); */
> -
>    printf("%s|||%s\n", entries[i].key, entries[i].descr);
> -
>    /*HeapFree(GetProcessHeap(), 0, descr);*/

You have to HeapFree a HeapAlloc'ed memory block.

It's better to use W versions of APIs even if it's not strictly required.
> - if (RegOpenKeyExA(HKEY_LOCAL_MACHINE, REGSTR_PATH_UNINSTALL, 0, KEY_READ, &hkeyUninst) != ERROR_SUCCESS)

RegOpenKeyExW in this case.
>  {
> - MessageBox(0, "Uninstall registry key not available (yet), nothing to do !", appname, MB_OK);
> - return 0;
> -
>    MessageBox(0, sRegistryKeyNotAvailable, sAppName, MB_OK);

MessageBoxW in this case.
>  if (!entries)
> - entries = HeapAlloc(GetProcessHeap(), 0, sizeof(uninst_entry));
> - entries = HeapAlloc(GetProcessHeap(), 0, sizeof(uninst_entry));

Quite strange formatting.
>  strcpy(key_app, REGSTR_PATH_UNINSTALL);
>  strcat(key_app, "\\");
>  p = key_app+strlen(REGSTR_PATH_UNINSTALL)+1;

You shouldn't use string APIs supposed to handle ASCII strings. Use lstrcpyW,lstrcatW
and lstrlenW in this and all other cases.

sizeOfSubKeyName = 255;
-    for ( i=0;
-   RegEnumKeyExA( hkeyUninst, i, subKeyName, &sizeOfSubKeyName,
-      NULL, NULL, NULL, NULL ) != ERROR_NO_MORE_ITEMS;
-   ++i )
+    for (i=0; RegEnumKeyExA( hkeyUninst, i, subKeyName, &sizeOfSubKeyName, NULL, NULL, NULL, NULL ) != ERROR_NO_MORE_ITEMS; ++i)

RegEnumKeyExW in this case.
> -
>    static const WCHAR DisplayNameW[] = {'D','i','s','p','l','a','y','N','a','m','e',0};
> -
>    strcpy(p, subKeyName);

lstrcpyW
> -
>    RegOpenKeyExA(HKEY_LOCAL_MACHINE, key_app, 0, KEY_READ, &hkeyApp);

RegOpenKeyExW
> -
>    res = CreateProcess(NULL, entries[i].command, NULL, NULL, FALSE, 0, NULL, NULL, &si, &info);

CreateProcessW
> -
>    prevsel = SendMessage(hList, LB_GETCURSEL, 0, 0);

SendMessageW

and many,many,many others. Make sure to pass correct arguments to all the changed APIs.

For better understanding of what is going on add the following as the very first
line in main.c:

#define __WINESRC__

New uninstaller

[Jonathan Ernst]

Le mardi 08 mars 2005 à 18:04 +0800, Dmitry Timoshkov a écrit :
[...]
> and many,many,many others. Make sure to pass correct arguments to all the changed APIs.

I fixed/changed many things regarding unicode. It seems that now both
outputing to the console and case insensitive find as you type feature
(re-)work.

I fixed most warnings, but there are still 4 lines that I couldn't fix
(173, 206, 215, 429). Can someone help me ?

Last but not least, since these changes the uninstaller crashes (very)
often. I have an unhandled exception when retrieving some apps from the
uninstall key and I don't know what cause this:

err:seh:EXC_DefaultHandling Unhandled exception code c0000005 flags 0
addr 0x77ecf5d4
> For better understanding of what is going on add the following as the
> very first line in main.c:
>
> #define __WINESRC__

When adding this line the compilation fails with this message:

main.c:194: error: `STARTUPINFO' undeclared (first use in this function)

Are some include(s) missing  ?

...

Do you think I'm near the target ?

New uninstaller

[Dmitry Timoshkov]

"Jonathan Ernst"  wrote:
> I fixed/changed many things regarding unicode. It seems that now both
> outputing to the console and case insensitive find as you type feature
> (re-)work.
>
> I fixed most warnings, but there are still 4 lines that I couldn't fix
> (173,

cast entries[numentries-1].command to LPBYTE as you do 3 lines earlier.
> 206,

wrong STARTUPINFO declaration, you need a unicode one with W suffix.
> 215,

you can't use sprintf to print to a WCHAR string, use win32 API wsprintfW
instead.
> 429). Can someone help me ?
>      slen  = strlenW(String),
>      plen  = strlenW(Pattern);

better use official API lstrlenW.
>     /* find start of pattern in string */
>     while (toupper(*start) != toupper(*Pattern))

You can't use toupper with unicode strings. I can't suggest a win32 equivalent
from the top of my head, you need to find out.
>     pptr = (WCHAR *)Pattern;

There is no need for a cast here.
>     while (toupper(*sptr) == toupper(*pptr))

same as above.
> Last but not least, since these changes the uninstaller crashes (very)
> often. I have an unhandled exception when retrieving some apps from the
> uninstall key and I don't know what cause this:
>
> err:seh:EXC_DefaultHandling Unhandled exception code c0000005 flags 0
> addr 0x77ecf5d4

Very likely a common error happening often in the process of unicodification:
buffer overflow due to not a correct memory allocation, or a not correct
initialization of a variable pointing to a buffer length (size of buffer in
bytes vs. WCHARs).
> > For better understanding of what is going on add the following as the
> > very first line in main.c:
> >
> > #define __WINESRC__
>
> When adding this line the compilation fails with this message:
>
> main.c:194: error: `STARTUPINFO' undeclared (first use in this function)
>
> Are some include(s) missing  ?

That's the point. I'd prefer that you could figure out that on your own.
Hint: look at the headers, and you will see that all those STARTUPINFO,
lstrcpy, RegOpenKey are not the real API names, but just macros depending
whether UNICODE is defined or not. In Wine using API macros is deprecated,
personally I'd prefer to not use them in programs/* either.
> Do you think I'm near the target ?

Definitely very close.

New uninstaller

[Jonathan Ernst]

Le mardi 08 mars 2005 à 23:29 +0800, Dmitry Timoshkov a écrit :
> "Jonathan Ernst"  wrote:
>
> [...]

Thanks, fixed.
> You can't use toupper with unicode strings. I can't suggest a win32 equivalent
> from the top of my head, you need to find out.

It seems that CharUpperW works well.

[...]
> > Last but not least, since these changes the uninstaller crashes (very)
> > often. I have an unhandled exception when retrieving some apps from the
> > uninstall key and I don't know what cause this:
> >
> > err:seh:EXC_DefaultHandling Unhandled exception code c0000005 flags 0
> > addr 0x77ecf5d4
>
> Very likely a common error happening often in the process of unicodification:
> buffer overflow due to not a correct memory allocation, or a not correct
> initialization of a variable pointing to a buffer length (size of buffer in
> bytes vs. WCHARs).

Thanks for the hint. I found the allocation problem.

[...]

Fixed

Now everything works; thanks for your step by step help; I hope that
what I'me learning now will let me make more additions to Wine !

But:

It is even possible to uninstall more than one app at a time, but I wish
that when an uninstaller has finished to uninstall an application the
list is updated, but it's not. What's and where's a clean way to do it ?

Please find attached main.c's diff.

Jonathan

P.S. Do I really have to split this new uninstaller in different patches
as nearly everything changed ?

New uninstaller

[Dmitry Timoshkov]

"Jonathan Ernst"  wrote:
> Thanks for the hint. I found the allocation problem.

Here is another one:
> -
>                    int len = GetWindowTextLengthW(GetDlgItem(hwnd, IDC_FILTER));
> -
>                    if(len > 0)
> -
>                    {
> -
>                        sFilter = (WCHAR*)GlobalAlloc(GPTR, len + 1);
> -
>                        GetDlgItemTextW(hwnd, IDC_FILTER, sFilter, len + 1);

And this one:
> - /* Load MessageBox's strings */
> - LoadStringW(hInst, IDS_APPNAME, sAppName, sizeof(sAppName));
> - LoadStringW(hInst, IDS_ABOUTTITLE, sAboutTitle, sizeof(sAboutTitle));
> - LoadStringW(hInst, IDS_ABOUT, sAbout, sizeof(sAbout));
> - LoadStringW(hInst, IDS_REGISTRYKEYNOTAVAILABLE, sRegistryKeyNotAvailable, sizeof(sRegistryKeyNotAvailable));
> - LoadStringW(hInst, IDS_UNINSTALLFAILED, sUninstallFailed, sizeof(sUninstallFailed));

LoadStringW takes number of WCHARs, not bytes.

Everything else seems to be good enough.
> P.S. Do I really have to split this new uninstaller in different patches
> as nearly everything changed ?

It's up to Alexandre to decide, just send your work to wine-patches.

New uninstaller

[Mike McCormack]

> > P.S. Do I really have to split this new uninstaller in different patches
> > as nearly everything changed ?
>
> It's up to Alexandre to decide, just send your work to wine-patches.

I would suggest that you do split it into seperate patches.  If one of 
your patches causes a regression, it's easier to find the regression if 
the changes are small.  Secondly, it's easier to review and verify small 
patches.  Thirdly, since small patches are easier to check, they're 
easier to get accepted.

How about creating three seperate patches that:
1. Change to use a dialog
2. Change to use unicode
3. internationalize the code

This seems slower, but it will actually be faster, since you'll only 
need to perfect one patch at a time.

Mike

uninstaller: what needs to be fixed ?

[Jonathan Ernst]

Hello,

I noticed Alexandre committed patch #1/3 of the new uninstaller; thanks for that.
Could you please tell me why #2 (find-as-you-type) and #3 (internationalization) stayed out ?

Thanks a lot for your help.
Jonathan

uninstaller: what needs to be fixed ?

[Alexandre Julliard]

Jonathan Ernst  writes:
> Hello,
>
> I noticed Alexandre committed patch #1/3 of the new uninstaller; thanks for that.
> Could you please tell me why #2 (find-as-you-type) and #3 (internationalization) stayed out ?

They don't apply cleanly, you have to resubmit them relative to CVS.

[AppDB] Supermaintainers can't delete posts

[Scott Ritchie]

Just another bug report - supermaintainers can't delete postings like
normal maintainers can.

Thanks,
Scott Ritchie

[AppDB] Supermaintainers can't delete posts

[Jonathan Ernst]

Le vendredi 11 mars 2005 à 01:12 -0800, Scott Ritchie a écrit :
> Just another bug report - supermaintainers can't delete postings like
> normal maintainers can.
>
> Thanks,
> Scott Ritchie

Are you sure ?

I just tried it and it works on my local setup.
I click on "Edit" and then I have a delete button I can click on.

What error do you get when clicking on this button ?

Thanks

[AppDB] Supermaintainers can't delete posts

[Scott Ritchie]

On Fri, 2005-03-11 at 10:51 +0100, Jonathan Ernst wrote:
> Le vendredi 11 mars 2005 à 01:12 -0800, Scott Ritchie a écrit :
> > Just another bug report - supermaintainers can't delete postings like
> > normal maintainers can.
> >
> > Thanks,
> > Scott Ritchie
>
> Are you sure ?
>
> I just tried it and it works on my local setup.
> I click on "Edit" and then I have a delete button I can click on.
>
> What error do you get when clicking on this button ?
>
> Thanks

I'm talking about the posts by users on the version's page, not the
notes and howtos and stuff.

Thanks,
Scott

[AppDB] Supermaintainers can't delete posts

[Jonathan Ernst]

Le vendredi 11 mars 2005 à 04:00 -0800, Scott Ritchie a écrit :
> On Fri, 2005-03-11 at 10:51 +0100, Jonathan Ernst wrote:
> > Are you sure ?
> >
> > I just tried it and it works on my local setup.
> > I click on "Edit" and then I have a delete button I can click on.
> >
> > What error do you get when clicking on this button ?
> >
> > Thanks
>
> I'm talking about the posts by users on the version's page, not the
> notes and howtos and stuff.
>
> Thanks,
> Scott

That's what I'm talking about too.
=> what error do you get ?

thanks.

[AppDB] Supermaintainers can't delete posts

[Scott Ritchie]

On Fri, 2005-03-11 at 13:47 +0100, Jonathan Ernst wrote:
> Le vendredi 11 mars 2005 à 04:00 -0800, Scott Ritchie a écrit :
> > On Fri, 2005-03-11 at 10:51 +0100, Jonathan Ernst wrote:
> > > Are you sure ?
> > >
> > > I just tried it and it works on my local setup.
> > > I click on "Edit" and then I have a delete button I can click on.
> > >
> > > What error do you get when clicking on this button ?
> > >
> > > Thanks
> >
> > I'm talking about the posts by users on the version's page, not the
> > notes and howtos and stuff.
> >
> > Thanks,
> > Scott
>
> That's what I'm talking about too.
> => what error do you get ?
>
> thanks.

I don't get a button at all, only "Post new" and "Reply to this"

-Scott

[AppDB] Supermaintainers can't delete posts

[Jonathan Ernst]

Le vendredi 11 mars 2005 à 14:56 -0800, Scott Ritchie a écrit :
[...]
> I don't get a button at all, only "Post new" and "Reply to this"
>
> -Scott

Ok thanks, I sent a patch to fix this as well (will be committed soon I
guess).