Re: [PATCH] Implement OleCreatePropertyFrame (try 2)

[Geoffrey Hausheer]

On Wed, Jan 13, 2010 at 10:38 PM, Piotr Caban wrote:
> Hi,
>
> There're still some things that needs to be changed.

Thank you for the continued feedback.
> In pixels_to_dialog_units:
> -
>
> *basex = ((float)size.cx/26+1)/2;
> -
>
> *x_pixels = MulDiv(*x_pixels, 4, *basex);
>
> I proposed doing the computations on floats so the dialog size is more
> accurate. If it was working for you correctly than it's probably better
> to keep it the way I proposed. The cast in the patch you've sent doesn't
> change anything.
> If there are reasons to make the computations on integers it's better to
> use GdiGetCharDimensions function.

My understanding is that your method is to get the average font width.
 I have no idea if that is similar to how MapDialogRect works, so I
have no idea if it more correct or not.  So I haven't any idea if the
accuracy of using float is really more accurate with respect to
Windows or not.  Looking through the dialog code in user32,
GetDialogBaseUnits just returns the value of GdiGetCharDimensions
(which isn't right here), so I don't see a better way than to use a
heuristic.
> I think that pixels_to_dialog_units function should be deleted and it's
> code move to OleCreatePropertyFrameIndirect.

It was originally meant to implement the inverse of MapDialogRect.
Now that it is precomputing the base units, it probably doesn't make
sense in its current form.
> - /* This was expected to work but didn't:
> - * LONG baseunits = GetDialogBaseUnits();
> - * basex = LOWORD(baseunits);
> - * basey = HIWORD(baseunits);
> - */
>
> This comment is not correct. GetDialogBaseUnits is somehow strange and
> should be avoided.

The comment is correct in that it is how MSDN says it should work, but
it obviously doesn't.  I think it is valuable to make it clear that
there is a reason for the heuristic.
> About PropertyPageSite interface:
> You have implemented AddRef and Release functions but you don't really
> care about reference counter. The object should be freed when the
> counter reaches 0.

Well, I didn't implement proper COM objects because I didn't see the
need.  Memory is allocated and freed in OlePropertyFrameIndirect.  But
COM objects must present AddRef/Release functions, so I have them.  Is
the preference here to build the proper COM object and allocate/free
individually, or can I implement the AddRef/Release functions as stubs
(which is basically what they are now)
> +typedef struct {
> - struct {
> - DLGTEMPLATE dlg;
> - WORD menu;
> - WORD class;
> - WORD caption;
> - WORD font;
> - } dialog;
> - OLEPropertyPageSite pps;
> - IPropertyPage  *propPage;
>
> +} OLEPropertyFrame;
> You don't need font field.

But I do need the WORD to be present, as MSDN clearly states that you
must reserve 4 WORDS after the DLGTEMPLATE.  Since its general use is
for 'font' I figured I'd name it properly.  If it is really
bothersome, I could call it 'reserved' or something equivalent.

I probably won't get to this for several days.  My development
computer will be out of commission for a while.

Thanks again,
.Geoff