Hi all,
Changelog:
Nog <nog(a)sdf.lonestar.org>
Don't enable the 'hot' state of a toolbar button when it's not
enabled.
--- dlls/comctl32/toolbar.c Sun Jul 29 09:08:56 2001
+++ dlls/comctl32/toolbar.c Sun Jul 29 11:41:50 2001
@@ -4123,13 +4123,13 @@
if (nHit >= 0)
{
btnPtr = &infoPtr->buttons[nHit];
- btnPtr->bHot = TRUE;
infoPtr->nHotItem = nHit;
/* only enabled buttons show hot effect */
if(infoPtr->buttons[nHit].fsState & TBSTATE_ENABLED)
{
+ btnPtr->bHot = TRUE;
InvalidateRect(hwnd, &btnPtr->rect,
TOOLBAR_HasText(infoPtr, btnPtr));
}
> Patrik Stridvall <ps(a)leissner.se> writes:
>
> > Does the Windows API really define exactly what a handle is?
>
> typedef void* HANDLE seems pretty clear to me.
I was refering some documentation that said that
application should always expect a handle to be
a pointer.
But never mind. Documentation and reality is often
different things.
> > However your suggestion that any Winelib application that
> cares whether
> > they are 32-bit or 64-bit are broken is a far too strict
> IMHO and I see
> > no reason to require porters to fix such things.
>
> An application that assumes that everything is 32-bit is broken, yes.
> I don't see why handles are any different than other types; if
> anything they should be less of a problem since they are supposed to
> be opaque, so their size should not matter for the application.
>
> Of course most handles will fit in 32-bit, so we will still support
> apps that do invalid casts and truncate the handles; but this doesn't
> mean it is correct behavior, or that apps that do this should be able
> to compile without warnings.
OK, since you agree we should accept handles truncated to 32-bit,
it still leaves the problem. Should API calls truncate the handles
themselves ie take 32-bit integers as arguments or should they not
ie take 64-bit pointers as arguments.
There is no particular reason to not accept both and make the
actual definition of handle be dependent on some define
and leave the choice on whether they want warnings or not
to the Winelib developers, not leave them without a choice.
Actually we might also consider giving them the option to
accept 64-bit integers.
In short, the Winelib developers should decide what their
application wants (warning wise), we shouldn't.
> > So therefor I suggest that we should do the following
> > TRACE("(%p)", (LPVOID) handle);
>
> You must be joking.
No, I'm not. However reality forced me to change the
solution somewhat. See below.
> I said I didn't want any typecast, and here you
> suggest to have one on every single debugging output.
Well, we have (or should have) a debugstr_[aw]n? on every single
string in the debugging output, because we have to. We might not like it,
but we have it because we must.
> Let me repeat it
> once more just in case: *NO* *TYPECASTS*.
I know that you said that and I have said that I support that whenever
possible, which it fortunately is in the normal cases.
> The whole point of STRICT and pointer handles is to be able to use
> compiler type checks, and this is only possible without typecasts.
Agreed, but there is no self value in having no exceptions on a rule.
Remember no type cast is a means no a goal.
Debugging output is hardly a critical piece of code so if we can
gain something from it we should consider making an exception.
But never mind, it turns out that we we needn't make a exception
either. See below.
> Not
> to mention that your solution will probably still get a warning like
> "converting integer to pointer of different size" on a 64-bit
> platform.
Ah, thats a good point. Practice often stand in the way of a good theory.
:-(
Unfurtunately I think you are right, it is more than probable that the
compiler
will generate such a warning, I if not now so sometime in the future.
Casting to a 32-bit integer to 64-bit pointer is almost always bogus,
so there is no reason for the compiler not give a warning epecially
since you can do a double cast (32-bit integer to 64-bit integer to
64-bit pointer) if you really mean it.
Fortunately the problem is solvable
#ifdef __alpha__
#ifdef STRICT
#define HANDLE2POINTER(handle) (handle)
#else
#define HANDLE2POINTER(handle) ((void *) (unsigned long) (handle))
#endif
#ifdef STRICT
#define HANDLE2POINTER(handle) (handle)
#else
#define HANDLE2POINTER(handle) ((void *) (handle))
#endif
#endif
TRACE("(%p)", HANDLE2POINTER(handle));
This solves your problem (at least for 32-bit developers)
< The whole point of STRICT and pointer handles is to be able to use
< compiler type checks, and this is only possible without typecasts.
Francois problem
< * Ignore gcc's warnings saying '%x format, pointer argument'. Fixing the
format
< at this stage would only introduce tons of warnings for the developpers
not
< participating in the switch (i.e. compiling with STRICT off).
as well as my problem.
Of course we might consider making it a debug output only construct instead
Like s/HANDLE2POINTER/debugptr_handle/ and define it in debugtools.h.
Non-debugging parts of Wine shouldn't need to use it either since as I said
earlier just pass them long or must use an explict cast to a non-void
pointer.
Ah, wait a minute we will have "converting integer to pointer of different
size"
on 64-bit platforms for the actual cast to an object.
Of course easily solved with a similar
#ifdef __alpha__
#define HANDLE2OBJECT(type, handle) ((type *) (unsigned long) (handle))
#else
#define HANDLE2OBJECT(type, handle) ((type *) (handle))
#endif
but perhaps we should have the HANDLE2POINTER (debugptr_handle) in
debugtools.h all the same, nobody else is intrested in a void pointer
anyway.
Of course, nothing forces us to define handles as the same both
internally and externally. At least Alpha AFAIK will be binary
compatible all the same, so we might not even need this.
Oh well having to always using HANDLE2OBJECT is a small price
to pay all the same and makes it possible for us to do
adjustments on other 64-bit platforms if need be.
And yes, I'm serious. My inital solutions wasn't so good,
but thanks to your feedback I have managed to refine it to
a much better solution, that I think actually works in reality.
And yes, I still consider the 64-bit problem a real problem,
you just tries to dismiss the problem by simply saying,
that is the Winelib developers problem. Sure we could do
that but, remember that Winelib is meant to help developers
port application not make it more difficult by trying
to enforce unnessary restriction on the code.
Sure Win64 should use 64-bit pointers as handles that is
without questions, but forcing it on Win32 on 64-bit
is totally unnessary, especially since I now finally
after your feedback managed to find a solution that
solve all your stated problems (remember that no type
casts is a means not a goal and actually there will be
no type casts on 32-bit (-DSTRICT) either).
But perhaps you can see any flaws in this refined solution as well?
Patrik Stridvall <ps(a)leissner.se> writes:
> Does the Windows API really define exactly what a handle is?
typedef void* HANDLE seems pretty clear to me.
> However your suggestion that any Winelib application that cares whether
> they are 32-bit or 64-bit are broken is a far too strict IMHO and I see
> no reason to require porters to fix such things.
An application that assumes that everything is 32-bit is broken, yes.
I don't see why handles are any different than other types; if
anything they should be less of a problem since they are supposed to
be opaque, so their size should not matter for the application.
Of course most handles will fit in 32-bit, so we will still support
apps that do invalid casts and truncate the handles; but this doesn't
mean it is correct behavior, or that apps that do this should be able
to compile without warnings.
> So therefor I suggest that we should do the following
> TRACE("(%p)", (LPVOID) handle);
You must be joking. I said I didn't want any typecast, and here you
suggest to have one on every single debugging output. Let me repeat it
once more just in case: *NO* *TYPECASTS*.
The whole point of STRICT and pointer handles is to be able to use
compiler type checks, and this is only possible without typecasts. Not
to mention that your solution will probably still get a warning like
"converting integer to pointer of different size" on a 64-bit
platform.
--
Alexandre Julliard
julliard(a)winehq.com
> Patrik Stridvall <ps(a)leissner.se> writes:
>
> > However for the debugging output I disagree. I orginally thought
> > as Francois (and you it seems) that handles are pointers and should
> > be treated as such. But then I realized that when we
> implement Win32 on
> > 64-bit platforms we can't have handles as pointers even
> internally if
> > we wan't to support x86 emulation and even exposing a 64-bit
> > handle to Winelib application can only cause trouble since they
> > might store it is a 32-bit memory location. Sure we will probably be
> > able to handle 64-bit pointers truncated to 32-bit by storing all
> > objects in the lower 32-bit part of memory, but fear it might
> > cause trouble all the same.
>
> Handles are pointers in the Windows API, and they should be pointers
> inside of Wine. On a 64-bit platforms pointers will be 64-bit, and so
> will handles.
Does the Windows API really define exactly what a handle is?
It is just a identifier for an object that can be passed to
various APIs but should not be used directly.
> If you want to run 32-bit code on such a platform you
> need a conversion for pointers anyway,
> so you might as well use the
> same conversion for handles.
> Not to mention that some handles are
> actually pointers (like HMODULE).
That is an internal Wine/Winelib problem and shouldn't need to
concern porters of Winelib applications.
> So no, there is absolutely no reason
> to force handles to be 32 bits.
Handles are some sort of values as for as whether they are integers or
pointers any Winelib application that cares are IMHO broken.
I guess you agree on that point.
However your suggestion that any Winelib application that cares whether
they are 32-bit or 64-bit are broken is a far too strict IMHO and I see
no reason to require porters to fix such things.
Note that it might no be so easy to fix it either. Perhaps the application
uses some external lib that provides storage of some user defined 32-bit
data and use it to store the handles. Now it will suddenly not work anymore,
and fixing it is not trival. Hint you are on a 64-bit platform that requires
64-bit pointers and you have 32-bit storage. The obvious solution an index
of an array might not always work so well because of memory management
requirements and beside the maintainer of the project might not accept
a special patch for the 64-bit version.
Anyway, never mind. See below instead.
Sure pointer face in some meaning the same problems,
but that sort of problems is more obvious and there is
no reason to add more to the burden.
> > The real reason that we should support compiling Wine with -DSTRICT
> > is not because handles becomes pointers but because callbacks gets
> > the correct prototype.
>
> No, it's because handles become pointers; you cannot do the -DSTRICT
> typechecking with integers anyway.
True, you can't do typechecking with integers but since Wine is
primarily compiled 32-bit platform that will use 32-bit pointers,
we will get the benefit of typechecking at the platform, which
happends the only one Wine currently works on.
Sure future developers on 64-bit will not find the errors then they
compile, but everybody else compiling for a 32-bit platform will,
so I don't see the problem.
> Callbacks are a very minor issue;
> the real point is to fix all incorrect handle manipulations, like
> implicit 16-bit handle conversions or arithmetic on handle values.
The ones of the above that can be found automatically will be fixed
by the 32-bit developers. So the actual problem is what?
Anyway, this discusion is getting a little carried away (mostly my fault).
While I do not currenly agree that we really should use 64-bit
handles on 64-bit platforms, your objections have made me think
some more and made me come to the following conclusions.
In order to not "close the door to the future" whether this
is Win32 on 64-bit or Win64, the Wine code shouldn't internally
not care (give errors or warnings or be semantically wrong)
whether the header files:
1. Defines handles as integers or pointers
2. Defines handles as 32-bit or 64-bit
(regardless of whether we have 32-bit or 64-bit pointers)
This is not so a for the normal (non-debugging) code.
OK, we might have to force memory allocation of
objects for the lower 32-bit of memory if we have
64-bit pointers but that is probably not so large
problem. After all the Winelib application itself can
allocate memory on the full 64-bit memory range
and Winelib itself are not likely to need more that
4Gb memory for objects.
However, I see no really good solution for the debug output.
Neither mine nor your previously suggested solution have
the above properties.
However the solution below have, I think
TRACE("(%p)", (LPVOID) handle);
eventhough it might display unnessary information
on 64-bit platforms, but perhaps we should worry
about that in the future, if it indeed is a problem
at all. After all it is only a few more bytes in
the debug output.
So therefor I suggest that we should do the following
TRACE("(%p)", (LPVOID) handle);
It will fix the problem that Francois speaks of in bug report #90.
< * Ignore gcc's warnings saying '%x format, pointer argument'. Fixing the
format
< at this stage would only introduce tons of warnings for the developpers
not
< participating in the switch (i.e. compiling with STRICT off).
It will allow us to fix the debug output one by one (or at least
seperately per type) since it will allow us compile without related
warnings even without -DSTRICT as well as to prepare us for a possible
future where handles might be 32-bit integers on 64-bit.
> On Sat, 28 Jul 2001, Patrik Stridvall wrote:
> [...]
> > The real reason that we should support compiling Wine with -DSTRICT
> > is not because handles becomes pointers but because callbacks gets
> > the correct prototype.
> >
> > I case not you haven't realized the obvious, it doesn't matter if
> > the handles are 32-bit integer or 32-bit pointers as long as
> > all handles are the same. The code either
>
> No, I think you have missed something: when compiling with -DSTRICT
> on all handles are not the same.
Note I said same but meant compatible. Wrong choice of words.
> For instance you cannot do
> hDesk=hAccel. That's because with -DSTRICT on you have:
>
> typedef struct HDESK__ { int unused; } *HDESK;
> typedef struct HACCEL__ { int unused; } *HACCEL;
>
> Thus HDESK and HACCEL are incompatible pointer types. This
> brings us
> more type safety which is the whole point of -DSTRICT.
Oh sure, but the point is that if code, that are semanticly
correct, cares whether you are strict or relax
the type, the code it wrong. Perhaps I was I little unclear on
that point. Being strict obviously makes it easier to find
semantic errors, but that was not the point at all.
> About 64bits Windows I don't know yet how we are going to do that.
> But you have to check exactly what Windows 64bit does, not just
> extrapolate from the way Unix 64bit does things. AFAIK the
> approach used
> by windows is quite different from that which Unices took.
Yes, unfortunately, but I'm primarily worried about Win32 on
64-bit platforms.
> > If we do that can just do
> > s/DECLARE_OLD_HANDLE/DECLARE_HANDLE/
> > on the handle types on by one and fix the warnings.
>
> That's precisely the plan, and it is just the opposite of what you are
> doing with your patch (you are turning DECLARE_HANDLE back into
> DECLARE_OLD_HANDLE).
The reason I did that was that they can't be fixed as long as
HANDLE/HGLOBAL is the wrong types. When they become the right
types the warnings will automatically disappear and we can
do s/DECLARE_OLD_HANDLE/DECLARE_HANDLE/. But there is no
reason to have a lot of warnings that can't be fixed
otherwise until then.
> Yes you get a number of warnings if you turn
> -DSTRICT on, most of them should disappear once the conversion is
> complete; fixing them before that is a waste of time.
Since we can't fix before that why show them at all?
People that want to fix for example output messages can do,
s/DECLARE_OLD_HANDLE/DECLARE_HANDLE/ in their tree,
fix them and submit the patches (without the
s/DECLARE_OLD_HANDLE/DECLARE_HANDLE/).
> This is the
> reason why -DSTRICT is not the default yet. Read bug report #90,
> Francois explains everything there.
OK. I have read bug report #90 now.
> And don't forget the number one rule of the -DSTRICT conversion is:
> *no* *typecasts*. If you have to add typecasts something is wrong.
No typecasts where the handle is used in conjuction with other handles,
certainly not, no argument there. The code some expect all handles to
be the of same type.
However for the debugging output I disagree. I orginally thought
as Francois (and you it seems) that handles are pointers and should
be treated as such. But then I realized that when we implement Win32 on
64-bit platforms we can't have handles as pointers even internally if
we wan't to support x86 emulation and even exposing a 64-bit
handle to Winelib application can only cause trouble since they
might store it is a 32-bit memory location. Sure we will probably be
able to handle 64-bit pointers truncated to 32-bit by storing all
objects in the lower 32-bit part of memory, but fear it might
cause trouble all the same.
Beside doing TRACE("(%p)", handle) on 64-bit platforms will needlessly
display the always the same presumably zero upper 32-bit part
of the handle. That why I decided to should use
TRACE("(0x%08lx)", (DWORD) handle) instead to avoid such issues.
In short, the code should IMHO support both 32-bit pointers as well
and 32-bit integers as handles presuming of course that all handles
use the same.
The real reason that we should support compiling Wine with -DSTRICT
is not because handles becomes pointers but because callbacks gets
the correct prototype.
I case not you haven't realized the obvious, it doesn't matter if
the handles are 32-bit integer or 32-bit pointers as long as
all handles are the same. The code either
1. Just pass them around as the same type of handles of other types
of handles (HANDLE/HGLOBAL).
2. Tries to access the underlying object, in such case a void pointer
will not do use any good, we must use a cast either after calls to
functions like GlobalLock or alone.
Patrik Stridvall <ps(a)leissner.se> writes:
> However for the debugging output I disagree. I orginally thought
> as Francois (and you it seems) that handles are pointers and should
> be treated as such. But then I realized that when we implement Win32 on
> 64-bit platforms we can't have handles as pointers even internally if
> we wan't to support x86 emulation and even exposing a 64-bit
> handle to Winelib application can only cause trouble since they
> might store it is a 32-bit memory location. Sure we will probably be
> able to handle 64-bit pointers truncated to 32-bit by storing all
> objects in the lower 32-bit part of memory, but fear it might
> cause trouble all the same.
Handles are pointers in the Windows API, and they should be pointers
inside of Wine. On a 64-bit platforms pointers will be 64-bit, and so
will handles. If you want to run 32-bit code on such a platform you
need a conversion for pointers anyway, so you might as well use the
same conversion for handles. Not to mention that some handles are
actually pointers (like HMODULE). So no, there is absolutely no reason
to force handles to be 32 bits.
> Beside doing TRACE("(%p)", handle) on 64-bit platforms will needlessly
> display the always the same presumably zero upper 32-bit part
> of the handle. That why I decided to should use
> TRACE("(0x%08lx)", (DWORD) handle) instead to avoid such issues.
Sorry but this is wrong. Handles are pointers and must be printed as
pointers. Assuming that handles are always exactly 32-bit is very
shortsighted.
> The real reason that we should support compiling Wine with -DSTRICT
> is not because handles becomes pointers but because callbacks gets
> the correct prototype.
No, it's because handles become pointers; you cannot do the -DSTRICT
typechecking with integers anyway. Callbacks are a very minor issue;
the real point is to fix all incorrect handle manipulations, like
implicit 16-bit handle conversions or arithmetic on handle values.
--
Alexandre Julliard
julliard(a)winehq.com
Patrik Stridvall <ps(a)leissner.se> writes:
> Concerning some parts of my warnings patch that was
> partly rejected I disagree with the rejection.
> The patch in question is in warnings_rejected.diff.
> Note the field "u" is a union. But perhaps you know
> and rejected it for other reasons?
I know it is a union; I thought it was one of these cases that would
get fixed by -DSTRICT, but now I see that it is a slightly different
problem. I'll apply it.
> If you accept all the strict_*.diff patches, perhaps we
> should consider adding -DSTRICT as a permanent flag?
> It removes the HOOKPROC/TIMERPROC warnings that is not
> possible to fix otherwise.
>
> If we do that can just do
> s/DECLARE_OLD_HANDLE/DECLARE_HANDLE/
> on the handle types on by one and fix the warnings.
That's precisely the plan, and it is just the opposite of what you are
doing with your patch (you are turning DECLARE_HANDLE back into
DECLARE_OLD_HANDLE). Yes you get a number of warnings if you turn
-DSTRICT on, most of them should disappear once the conversion is
complete; fixing them before that is a waste of time. This is the
reason why -DSTRICT is not the default yet. Read bug report #90,
Francois explains everything there.
And don't forget the number one rule of the -DSTRICT conversion is:
*no* *typecasts*. If you have to add typecasts something is wrong.
--
Alexandre Julliard
julliard(a)winehq.com
Alexandre Julliard:
> I think GetClipBox is correct, the problem is
> that the WM_ERASEBKGND
> handler should not be using it, but
> GetClientRect. You can try this:
Hello Alexandre,
thank you for your reccomendation. Unfortunately the DefWindowProc
improvement does not help, because the default handler newer gets
called in my app. The application (Progress _prowin.exe version 7)
uses custom dialog boxes (forms) and also custom controls. The
controls handle WM_ERASEBKGND themeselves. I have no access to
the sources. What I know about GetClipRect, I know from the trace. I agree
that your method (GetClientRect in the handler) is much better, but I
cannot use it.
I also agree that Windows should return the parent area in GetClipBox
as Wine does. Unfortunalelly, the Windows do not do that while the
_prowin.exe depends on it. I enclose a simple C example
which I hacked to demonstrate this. The example displays two red
children in a white parent. The value returned by GetClipBox is
wsprintf-ed.
If you run the binary in the Windows, you will see two properly sized
red rectangles on a white background. If you run the same binary in
Wine, you will see only one large red child but no white parent
background. If either the CS_PARENTDC or WS_CLIPSIBLINGS gets removed
from my example, both Windows and Wine give identical results.
Regards
Ladislav Sladecek
