RFC: Richedit: Test Patch for EM_SETMARGINS

[James McKenzie]

All:

I would like comments on this patch.

Thank you.

James McKenzie

[Nikolay Sivov]

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.
> - 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?
> - /* 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?
> - 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.
> - /*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?
> - /* 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.
> - 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.
> - todo_wine {
> -
>    SendMessageW(hwndRichEdit, EM_SETMARGINS, EC_USEFONTINFO, MAKELONG(EC_USEFONTINFO, EC_USEFONTINFO));
> - }

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

[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

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

[James McKenzie]

On 1/16/11 10:24 AM, André Hentschel wrote:
> see http://test.winehq.org/data/

Will do.  I don't want to break Windows9x functionality though.
> > - /* 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..."

Much better idea.
> - 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

Same style as used in the rest of the file.  I like curly braces on 
separate lines (I hope that is what you are referring to.)
> > > > - 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.

I interpreted it that way as well.  I have a system here set to 120 DPI 
and use small fonts for the default font setting.

James McKenzie

[Nikolay Sivov]

On 1/16/2011 20:17, James McKenzie wrote:
> > > - 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'.

There's no test in this line, maybe that's why it 'works'.

[James McKenzie]

On 1/16/11 10:27 AM, Nikolay Sivov wrote:
> On 1/16/2011 20:17, James McKenzie wrote:
> > > > - 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'.
>
> There's no test in this line, maybe that's why it 'works'.

No, USEFONTINFO is equal to 65,535 (0xFFFF) and in the other cases sets 
the margin to either an overflow or underflow condition.  This test does 
not move the margins.  Microsoft's documentation is lacking in how to 
use this feature and I could not find a good case on the Internet.  
Perhaps I used wrong search criteria or no one is using this capability.

James McKenzie

[Nikolay Sivov]

On 1/16/2011 20:53, James McKenzie wrote:
> On 1/16/11 10:27 AM, Nikolay Sivov wrote:
> > On 1/16/2011 20:17, James McKenzie wrote:
> > > > > - 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'.
> >
> > There's no test in this line, maybe that's why it 'works'.
>
> No, USEFONTINFO is equal to 65,535 (0xFFFF) and in the other cases 
> sets the margin to either an overflow or underflow condition.  This 
> test does not move the margins.  Microsoft's documentation is lacking 
> in how to use this feature and I could not find a good case on the 
> Internet.  Perhaps I used wrong search criteria or no one is using 
> this capability.

Ok, let's put it another way. There's no wine test entry in this line.

[James McKenzie]

On 1/16/11 10:58 AM, Nikolay Sivov wrote:
> On 1/16/2011 20:53, James McKenzie wrote:
> > On 1/16/11 10:27 AM, Nikolay Sivov wrote:
> > > On 1/16/2011 20:17, James McKenzie wrote:
> > > > > > - 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'.
> > >
> > > There's no test in this line, maybe that's why it 'works'.
> >
> > No, USEFONTINFO is equal to 65,535 (0xFFFF) and in the other cases 
> > sets the margin to either an overflow or underflow condition.  This 
> > test does not move the margins.  Microsoft's documentation is lacking 
> > in how to use this feature and I could not find a good case on the 
> > Internet.  Perhaps I used wrong search criteria or no one is using 
> > this capability.
>
> Ok, let's put it another way. There's no wine test entry in this line.

Got it.  Thank you again, Nikolay for your feedback.  Now to figure out 
how to change the screen dpi.

James McKenzie

[Charles Davis]

On 1/16/11 10:53 AM, James McKenzie wrote:
> On 1/16/11 10:27 AM, Nikolay Sivov wrote:
> > On 1/16/2011 20:17, James McKenzie wrote:
> > > > > - 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'.
> >
> > There's no test in this line, maybe that's why it 'works'.
>
> No, USEFONTINFO is equal to 65,535 (0xFFFF) and in the other cases sets
> the margin to either an overflow or underflow condition.  This test does
> not move the margins.  Microsoft's documentation is lacking in how to
> use this feature and I could not find a good case on the Internet. 
> Perhaps I used wrong search criteria or no one is using this capability.

He means there's no ok() macro call inside the todo_wine block. todo
blocks only have an effect on ok() macros.

Chip

[James McKenzie]

On 1/16/11 10:58 AM, Charles Davis wrote:
> On 1/16/11 10:53 AM, James McKenzie wrote:
> > On 1/16/11 10:27 AM, Nikolay Sivov wrote:
> > > On 1/16/2011 20:17, James McKenzie wrote:
> > > > > > - 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'.
> > >
> > > There's no test in this line, maybe that's why it 'works'.
> >
> > No, USEFONTINFO is equal to 65,535 (0xFFFF) and in the other cases sets
> > the margin to either an overflow or underflow condition.  This test does
> > not move the margins.  Microsoft's documentation is lacking in how to
> > use this feature and I could not find a good case on the Internet.
> > Perhaps I used wrong search criteria or no one is using this capability.
>
> He means there's no ok() macro call inside the todo_wine block. todo
> blocks only have an effect on ok() macros.

I will remove the todo_wine from this call.  It feeds back a 'fixme' for 
now.  I'll provide a 'cleaner' patch later today after working on it 
some more.

James McKenzie

[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.
>
> Another question is how screen resolution affects that test.

Thank you and Ge for this.  Changing the resolution from 96 to 120 
definitely affects the test results.  Plus something else broke the test 
so now it is reporting errors.  Sigh, back to testing some more.

James McKenzie

[Dmitry Timoshkov]

James McKenzie  wrote:
> - ok (16 == sentLogFontA.lfHeight, "Height not set to 12 for System Font. Height is %d\n", sentLogFontA.lfHeight);

12?
> - ok (96 == ry, "DPI for screen does not equal 96 it is %d\n", ry);
> - ok (-240 == CharFont1ANSI.yHeight /* Wine */|| 195 == CharFont1ANSI.yHeight /* Windows */, "y Height of System Character is "
> -
>    "not -240 or 195 but is %d\n", CharFont1ANSI.yHeight);
> - ok (16 == abs(CharFont1ANSI.yHeight) * ry / 1440 /*Wine*/|| 13 == abs(CharFont1ANSI.yHeight) * ry / 1440 /*Windows*/, \
> -
>    "Character Height of System character set is not 16 but %d\n", abs(CharFont1ANSI.yHeight) * ry / 1440);
> - ok (0 == CharFont1ANSI.yOffset, "Character Offset for System character set is not zero but %x\n", CharFont1ANSI.yOffset);
> - ok (7 == tma.tmAveCharWidth, "Average Character Width for System character set is %d\n", tma.tmAveCharWidth);
> - ok (15 == tma.tmMaxCharWidth /*Wine*/ || 14 == tma.tmMaxCharWidth /*Windows*/, "Maximum Character Width for System character set " \
> -
>    "is %d\n", tma.tmMaxCharWidth);

Please use normal not reversed comparisons here and everywhere else, that
style improves nothing.

Also comments like /*Wine*/ and /*Windows*/ suggest that the tests deserve
at least a todo_wine around them.

Testing a return value of SendMessage() everywhere is also highly recommended
to make sure that the call really has any affect.

[James McKenzie]

On 1/17/11 1:07 AM, Dmitry Timoshkov wrote:
> James McKenzie  wrote:
> > - ok (16 == sentLogFontA.lfHeight, "Height not set to 12 for System Font. Height is %d\n", sentLogFontA.lfHeight);
>
> 12?

Fixed in the next test patch.
> > - ok (96 == ry, "DPI for screen does not equal 96 it is %d\n", ry);
> > - ok (-240 == CharFont1ANSI.yHeight /* Wine */|| 195 == CharFont1ANSI.yHeight /* Windows */, "y Height of System Character is "
> > -
> >    "not -240 or 195 but is %d\n", CharFont1ANSI.yHeight);
> > - ok (16 == abs(CharFont1ANSI.yHeight) * ry / 1440 /*Wine*/|| 13 == abs(CharFont1ANSI.yHeight) * ry / 1440 /*Windows*/, \
> > -
> >    "Character Height of System character set is not 16 but %d\n", abs(CharFont1ANSI.yHeight) * ry / 1440);
> > - ok (0 == CharFont1ANSI.yOffset, "Character Offset for System character set is not zero but %x\n", CharFont1ANSI.yOffset);
> > - ok (7 == tma.tmAveCharWidth, "Average Character Width for System character set is %d\n", tma.tmAveCharWidth);
> > - ok (15 == tma.tmMaxCharWidth /*Wine*/ || 14 == tma.tmMaxCharWidth /*Windows*/, "Maximum Character Width for System character set " \
> > -
> >    "is %d\n", tma.tmMaxCharWidth);
>
> Please use normal not reversed comparisons here and everywhere else, that
> style improves nothing.

Actually this did catch an error where I used equal (=) instead of equal 
to (==) and that is a couple of best coding practice books that I use.  
However, for the final patch, this will be removed in its entirety.  
There is a difference between what Windows is returning and what Wine is 
returning but that is not the purpose of this patch.

James McKenzie