[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    RE: Last (?) DGA 2 patch
From:       Patrik Stridvall <ps () leissner ! se>
Date:       1999-12-30 21:03:45
[Download RAW message or body]

> As my 'for review' patch did not raise any outcry on the WineDevel
> mailing list (and as I am leaving for a few days for Y2K :-) ), I post
> my patch on wine-patches.

I haven't had time to look it (as I promised), but I have done so now.
First of all, on the whole, looks good, no objection to it being applied.

Nether the less, there are some minor issue that perhaps should be
fixed in the future. Note that I might be a little pendantic,
so keep that in mind then reading what I have to say below.

In windows/x11drv/event.c:

You seem to have done approximately as I have suggested concerning the OO
design.
However since you only needed one entry in the X11DRV event table you took
a shortcut and used a function pointer in stead of a jumptable.
Personally I would have ues a jumptable, since it would have made the
code easier to read IMHO. In any case the readbillity is event further
decreased by the fact that you have named the funtion pointer
X11DRV_EVENT_ProcessEvent which most people would assume it
is a function, not a function pointer, because how the that kind
of name is normally used. Most people, I believe, would assume
that X11DRV_EVENT_ProcessEvent is the X11DRV implementation,
not the pointer to the implementation of the current "inheritance".

IMHO it is EVENT_ProcessEvent that should be called
X11DRV_EVENT_ProcessEvent
along with most (all?) of the other EVENT_*. Note that the fault that they
aren't called that is mine, since I didn't do the renaming when I
moved the code to the X11DRV because of external dependencies and
I never got around doing it before I forgot about it.

In short, use a jumptable despite the fact the you only have one entry,
it is much more readable. The renaming can wait even though 

In windows/{,d}input.c:

You have replaced GetSystemMetric(SM_C{X,Y}SCREEN)
with MONITOR_Get{Width,Height}(&MONITOR_PrimaryMonitor).

This is understandable given how GetSystemMetric is
implemented, it is not syncronised with the currrent screen mode.

However if under Windows GetSystemMetric(SM_C{X,Y}SCREEN) is
syncronized with the current DirectX screen mode it should
be in Wine too. I am not sure if it is though.

In short, you have made a hack to solve the problem only
in a special case, not in the general case. Note that
GetSystemMetric(SM_C{X,Y}SCREEN) is used at 33 places
in the current CVS.

[prev in list] [next in list] [prev in thread] [next in thread]