RFC: Richedit: Test Patch for EM_SETMARGINS

[André Hentschel]

Am 16.01.2011 18:17, schrieb 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.
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.

see http://test.winehq.org/data/

>>> +    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.
>>> +    /* Per http://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?

remove the url, maybe better something like "msdn's documentation for functionxyz..."

>>
>>> +    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'.

i bet Nikolay sighed about the curly brackets

>>
>>> +  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.

to be clear: i guess it's about the dpi and not the resolution like 1024x768.

-- 

Best Regards, André Hentschel