RFC: Richedit: Test Patch for EM_SETMARGINS

[James McKenzie]

On 1/16/11 9:55 AM, Nikolay Sivov wrote:
> On 1/16/2011 19:39, James McKenzie wrote:
>> All:
>>
>> I would like comments on this patch.
> Ok.
>
>>   static HWND new_window(LPCTSTR lpClassName, DWORD dwStyle, HWND parent) {
>>     HWND hwnd;
>> -  hwnd = CreateWindow(lpClassName, NULL, dwStyle|WS_POPUP|WS_HSCROLL|WS_VSCROLL
>> +/*  hwnd = CreateWindow(lpClassName, NULL, dwStyle|WS_POPUP|WS_HSCROLL|WS_VSCROLL
>>                         |WS_VISIBLE, 0, 0, 200, 60, parent, NULL,
>> -                      hmoduleRichEdit, NULL);
>> +                      hmoduleRichEdit, NULL); */
>> +  /* Set screen to VGA proportions for testing purposes, will be removed from final patch release */
>> +    hwnd = CreateWindow(lpClassName, NULL, dwStyle|WS_POPUP|WS_HSCROLL|WS_VSCROLL
>> +                        |WS_VISIBLE, 0, 0, 640, 480, parent, NULL,
>> +                        hmoduleRichEdit, NULL);
>>     ok(hwnd != NULL, "class: %s, error: %d\n", lpClassName, (int) GetLastError());
>>     return hwnd;
> Commented code?
>
>> +static inline int is_win9xA()
>> +{
>> +    static WCHAR arialW[]={'A','r','i','a','l',0};
>> +    SetLastError(0xdeadbeef);
>> +    lstrcmpW(arialW, arialW);
>> +    return (GetLastError() == ERROR_CALL_NOT_IMPLEMENTED);
>> +}
> I think we don't care much about 9x now.
We still do tests on Windows95 to prevent regressions, or am I 
incorrect?  This code could be removed then.
>> +    hDC = GetDC(hwndRichEdit);
>> +    ok (hDC, "Creation of hdc failed \n");
>> +    if (!hDC)
>> +    {
>> +        DeleteObject(testFont1A);
>> +        DeleteObject(testFont2A);
>> +        DeleteObject(testFont1W);
>> +        DeleteObject(testFont2W);
>> +        DeleteObject(testFont3W);
>> +        DeleteObject(testFont4W);
>> +        DeleteDC(hDC);
>> +        DestroyWindow(hwndRichEdit);
>> +        return;
>> +    }
> DeleteObject() with uninitialized handles?
Should I go ahead and just 'return' then?
>> +    /* Calculate the twips per pixel */
>> +    ry = GetDeviceCaps(hDC, LOGPIXELSY);
>> +
>> +    static const WCHAR arialW[] = {'A','r','i','a','l',0};
>> +    static const WCHAR courier[] = {'C','o','u','r','i','e','r',0};
>> +    static const WCHAR msSansSerif [] = {'M','S',' ','S','a','n','s',' ','S','e','r','i','f',0};
>> +
> Variable declaration in code?
I can fix this.
>
>> +    todo_wine {
>> +    SendMessageA(hwndRichEdit, EM_SETMARGINS, EC_LEFTMARGIN, MAKELONG (5,0));
>> +    SendMessageA(hwndRichEdit, EM_GETRECT, 0, (LPARAM)&new_rect);
>> +    ok(new_rect.left == old_rect.left + 5, "The left border of the rectangle is wrong.  The value of it is %d.\n", \
>> +    new_rect.left);
>> +    }
> todo_wine usually contains only test calls.
Ok.  I missed this one.
>
>> +    /*Set test font to be Courier font */
>> +    SendMessageW(hwndRichEdit, WM_SETFONT, (WPARAM)testFont1W, MAKELPARAM(TRUE, 0));
>> +    /*Verify font is set */
>> +    SendMessageW(hwndRichEdit, EM_GETCHARFORMAT, SCF_DEFAULT, (LPARAM)&CharFont1Unicode);
>> +    GetObjectW(testFont1W, sizeof(LOGFONTW),&sentLogFont);
> Do you really need to verify that?
No.  I will remove this code.
>> +    /* Perhttp://msdn.microsoft.com/en-us/library/bb761649%(VS.85).asp  if the lparam is
>> +       EC_USEFONTINFO the Left and Right Margins should be set to a narrow width if the
>> +       font has been set */
> They don't use permanent links I believe.
Can I modify this to state the page name?
>
>> +    ret = GetTextMetricsW (hDC,&tmw);
>> +    ok (ret, "GetTextMetricsW failed\n");
>> +    ok (7 == tmw.tmAveCharWidth, "Average Character Width for MS Sans Serif is %d\n", tmw.tmAveCharWidth);
>> +    ok (14 == tmw.tmMaxCharWidth, "Maximum Character Width for MS Sans Serif is %d\n", tmw.tmMaxCharWidth);
>> +    ok (150== CharFont1Unicode.yHeight /*Windows*/ || -195 == CharFont1Unicode.yHeight /*Wine*/, \
>> +       "Character height for MS Sans Serif is not 195 but is %d\n", abs(CharFont1Unicode.yHeight));
>> +    ok (10 == (abs(CharFont1Unicode.yHeight) * ry) / 1440 /*Windows*/ || 13 == (abs(CharFont1Unicode.yHeight) * ry) / 1440 /*Wine*/, \
>> +       "Character Height of MS San Serif character set is not 12 but %d\n", abs(CharFont1Unicode.yHeight) * ry / 1440);
>> +    ok (0 == CharFont1Unicode.wWeight /*Windows*/ || FW_NORMAL == CharFont1Unicode.wWeight /*Wine*/, "Average Character Weight for MS"\
>> +        " Sans Serif is %d\n", CharFont1Unicode.wWeight);
> I don't like that. Looks like it's necessary to figure out why values 
> differ comparing to native.
>
Ok.  I questioned this as well.
>> +    todo_wine {
>> +        SendMessageW(hwndRichEdit, EM_SETMARGINS, EC_USEFONTINFO, MAKELONG(EC_USEFONTINFO, EC_USEFONTINFO));
>> +    }
> Hm.
This is one of the test cases that was recommended and it appears to be 
the only one that 'works'.

>
>> +  if (winetest_debug>  1) {
>>     test_WM_CHAR();
>>     test_EM_FINDTEXT();
>>     test_EM_GETLINE();
>> @@ -7126,6 +7643,8 @@ START_TEST( editor )
>>     test_WM_GETDLGCODE();
>>     test_zoom();
>>     test_dialogmode();
>> +  }
>> +  test_em_setmargins();
> Why?
>
> Another question is how screen resolution affects that test.
I will look at this as well.  I'll have to figure out how to change the 
resolution on the test bot and or my local copy of WindowsXP SP2 (not 
upgradable, no Internet connection to that system.

Thank you for the comments and suggestions, Nikolay.

James McKenzie

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110116/4cc495fc/attachment.htm>