Well, I finally solved all of my issues with my patch (yay!). It will
be ready to submit to wine-patches as soon as I reinstall wine to
finish up my testing.. Something borked it and now it won't run even
winecfg... Go figure. So anyways, with any luck, my testing will go
without a hitch, and I can get my first patch ever done in C for the
wine project committed. Wish me luck.
Tom
On 4/19/07, Tom Spear <[EMAIL PROTECTED]> wrote:
I rewrote the patch based on your suggestions, and I'm having a little
difficulty with certain things. I did a little more testing, and it
turns out that the uninstaller I was using removes the local machine
uninstall entry on its own. So I started futzing with the code some
to try to figure a way to get the root key to be properly passed to
UninstallProgram. The only thing I can come up with is some form of
using entries[sel].key. I can only test at work though, so I have no
way of knowing until tomorrow if that would even work. However,
assuming that it will, is there a way to strip PathUninstallW from
entries[sel].key so that the only thing left is the root key? If so
then I can probably use something along the lines of (pseudocode):
root_key = strip(entries[sel].key, PathUninstallW);
UninstallProgram(root_key);
Thanks for the help
Tom
On 4/19/07, James Hawkins <[EMAIL PROTECTED]> wrote:
> On 4/19/07, Tom Spear <[EMAIL PROTECTED]> wrote:
> > Try applying the patch before you comment on it. See below..
> >
>
> I read all your reply comments, and nothing you said leads me to
> believe that I need to apply this patch to review it.
>
> > On 4/19/07, James Hawkins <[EMAIL PROTECTED]> wrote:
> > > On 4/19/07, Tom Spear <[EMAIL PROTECTED]> wrote:
> > > > Apparently the last patch had multiple patches in the file because I
> > > > didnt delete it before creating the new one..
> > > >
> > > > So here we go again.
> > > >
> > > > Check HKCU as well as HKLM for uninstall entries, using a for loop
> > > >
> > >
> > > @@ -69,7 +69,7 @@
> > > /**
> > > * Used to output program list when used with --list
> > > */
> > > -static void ListUninstallPrograms(void)
> > > +static void ListUninstallPrograms()
> > >
> > > The parameter list was correct as is. If a function takes no
> > > parameters, the correct list is 'void'. Don't change parts of the
> > > code that don't have to do with your patch, especially when you don't
> > > know what you're changing.
> >
> > That one was an oops, I'll change it back...
> >
> > > if (i < numentries)
> > > - UninstallProgram();
> > > + for (x=0; x<2; x++)
> > > + UninstallProgram(rootkey);
> > >
> > > Why are you calling UninstallProgram twice with the same key? Besides
> > > that, using magic numbers (2) is bad coding.
> >
> > That is another oops. I should have double checked this as well. The
> > use of the 'magic number' in this case is that there will never be
> > more than 2 keys (EVER) to check. However I will make a constant
> > array instead and check x against the number of values in this array
> > instead.
> >
>
> Engineers at a certain software company decided that machines would
> not have more than 640k of RAM anytime soon, leading to at least a
> decade of headaches for developers. Case in point, putting
> self-imposed limits in your code is a bad design habit.
>
> > > + HKEY keys[2];
> > > + keys[0] = HKEY_CURRENT_USER;
> > > + keys[1] = HKEY_LOCAL_MACHINE;
> > >
> > > HKEY keys[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
> > >
> > > + for (x=0; x<2; x++)
> > >
> > > You're hardcoding this value to 2, which is another bad magic number.
> > > What if, hypothetically, more root keys are added to the list?
> >
> > see above
> >
> > > for (x = 0; x < sizeof(keys); x++)
> > >
> > > + rootkey[0] = HKEY_CURRENT_USER;
> > > + rootkey[1] = HKEY_LOCAL_MACHINE;
> > >
> > > Same thing as above.
> >
> > see above
> >
> > > + sizeOfSubKeyName = 255;
> > >
> > > What is 255?
> >
> > I dont know, but if you look at the file (pre patch) that is already
> > there, diff just deletes and readds it because it moved further down
> > in the file. However, to answer your question, I checked the MS
> > website:
> >
>
> The old code is clearly in the patch, and no one implied that you
> wrote it. The comment still stands though. The 255 needs to be
> replaced with a const or define that clarifies what the value
> represents.
>
> > http://support.microsoft.com/kb/256986
> >
> > <quote>
> > The maximum size of a key name is 255 characters.
> >
> > The following table lists the data types that are currently defined
> > and that are used by Windows. The maximum size of a value name is as
> > follows:
> > • Windows Server 2003 and Windows XP: 16,383 characters
> > • Windows 2000: 260 ANSI characters or 16,383 Unicode characters
> > • Windows Millennium Edition/Windows 98/Windows 95: 255 characters
> > </quote>
> >
> > > + HKEY rootkey[2];
> > > + rootkey[0] = HKEY_CURRENT_USER;
> > > + rootkey[1] = HKEY_LOCAL_MACHINE;
> > >
> > > Same.
> > same
> > >
> > > + /* FIXME: UninstallProgram should figure out
> > > which root key to uninstall
> > > + * from, as opposed to just going with the
> > > first value in rootkey. The
> > > + * entry gets removed this way as well, but
> > > it is not apparent by this code.
> > > + */
> > > + UninstallProgram(*rootkey);
> > >
> > > What is the point of adding rootkey if you're not going to use it?
> > > UninstallProgram(HKEY_X) is a lot shorter. The point is, don't add
> > > something until you're going to use it. On top of that, this new code
> > > is basically UninstallProgram(HKEY_CURRENT_USER) when the original
> > > code was (parameter added for convenience)
> > > UninstallProgram(HKEY_LOCAL_MACHINE). That's not right.
> >
> > Well, unfortunately there isnt a way to call UninstallProgram without
> > a parameter if it is declared that way.
>
> Please reread my comment. rootkey is superfluous, not the parameter
> to UninstallProgram.
>
> > So I could either hardcode it
> > to HKEY_LOCAL_MACHINE, since it requires a parameter now, or I could
> > leave it as *rootkey to be fixed later.
>
> Since rootkey shouldn't be in the patch (should only be added when
> it's used), the only option remaining is to call
> UninstallProgram(HKEY_LOCAL_MACHINE).
>
> > I tried to find a way to for
> > loop it, but because it is inside DlgProc, I get an error when
> > compiling. So I left it alone.. As the comment says, the code works
> > as is, but it is not apparent by the code. If someone has a
> > suggestion on how to get that specific line of code to look proper (so
> > you can tell if it is doing UninstallProgram on HKLM or HKCU, please
> > feel free..
> >
> > The reason we need the parameter added to UninstallProgram is because of:
> >
> > /* delete the application's uninstall entry */
> > RegOpenKeyExW(HKEY_LOCAL_MACHINE, PathUninstallW, 0, KEY_READ, &hkeyUninst);
> >
> > being in the original file. I think it's better to have whichever
> > root key is being used passed to the RegOpenKeyExW.....
> >
>
> I never had any objections with the new parameter to UninstallProgram.
>
> --
> James Hawkins
>
--
Thanks
Tom
Check out this new 3D Instant Messenger called IMVU. It's the best I
have seen yet!
http://imvu.com/catalog/web_invitation.php?userId=1547373&from=power-email
--
Thanks
Tom
Check out this new 3D Instant Messenger called IMVU. It's the best I
have seen yet!
http://imvu.com/catalog/web_invitation.php?userId=1547373&from=power-email
