Re: [PATCH 3/4] kernel32/tests: Add tests for GetCurrentConsoleFont

[Sebastian Lackner]

On 21.10.2015 02:43, Hugh McMaster wrote:
> - if (osvi.dwMajorVersion >= 6) /* Windows Vista or higher */
> - {
> -
>    COORD c;
> -
>    c = GetConsoleFontSize(std_output, cfi.nFont);
> -
>    todo_wine ok(cfi.dwFontSize.X == c.X, "got %d, expected %d\n", cfi.dwFontSize.X, c.X);
> -
>    todo_wine ok(cfi.dwFontSize.Y == c.Y, "got %d, expected %d\n", cfi.dwFontSize.Y, c.Y);
> - }
> - else
> - {
> -
>    CONSOLE_SCREEN_BUFFER_INFO csbi;
> -
>    short int width, height;
> -
>    GetConsoleScreenBufferInfo(std_output, &csbi);
> -
>    width = csbi.srWindow.Right - csbi.srWindow.Left + 1;
> -
>    height = csbi.srWindow.Bottom - csbi.srWindow.Top + 1;
> -
>    ok(cfi.dwFontSize.X == width, "got %d, expected %d\n", cfi.dwFontSize.X, width);
> -
>    ok(cfi.dwFontSize.Y == height, "got %d, expected %d\n", cfi.dwFontSize.Y, height);
> - }

I haven't reviewed the whole patch, but wanted to point that we usually do not test or
implement Windows version specific behavior, unless an application depends on it. Often
only one of both is implemented, and the other version is marked as broken.

If you decide to implement the pre-Vista behavior (which would make sense, because Wine
still uses WinXP as default version), it seems like no wineserver changes are required
at all (at least as long as font.index is always 0).

[PATCH 3/4] kernel32/tests: Add tests for GetCurrentConsoleFont

[Hugh McMaster]

On Wed, 21 Oct 2015 02:54:55 +0200 Sebastian Lackner wrote:
> On 21.10.2015 02:43, Hugh McMaster wrote:
> > - if (osvi.dwMajorVersion>= 6) /* Windows Vista or higher */
> > - {
> > -
> >    COORD c;
> > -
> >    c = GetConsoleFontSize(std_output, cfi.nFont);
> > -
> >    todo_wine ok(cfi.dwFontSize.X == c.X, "got %d, expected %d\n", cfi.dwFontSize.X, c.X);
> > -
> >    todo_wine ok(cfi.dwFontSize.Y == c.Y, "got %d, expected %d\n", cfi.dwFontSize.Y, c.Y);
> > - }
> > - else
> > - {
> > -
> >    CONSOLE_SCREEN_BUFFER_INFO csbi;
> > -
> >    short int width, height;
> > -
> >    GetConsoleScreenBufferInfo(std_output, &csbi);
> > -
> >    width = csbi.srWindow.Right - csbi.srWindow.Left + 1;
> > -
> >    height = csbi.srWindow.Bottom - csbi.srWindow.Top + 1;
> > -
> >    ok(cfi.dwFontSize.X == width, "got %d, expected %d\n", cfi.dwFontSize.X, width);
> > -
> >    ok(cfi.dwFontSize.Y == height, "got %d, expected %d\n", cfi.dwFontSize.Y, height);
> > - }
>
> I haven't reviewed the whole patch, but wanted to point that we usually do not test or
> implement Windows version specific behavior, unless an application depends on it. Often
> only one of both is implemented, and the other version is marked as broken.
>
> If you decide to implement the pre-Vista behavior (which would make sense, because Wine
> still uses WinXP as default version), it seems like no wineserver changes are required
> at all (at least as long as font.index is always 0).

You are correct. No wineserver changes are required if implementing the WinXP version of the function.

But I don't believe it is useful to avoid implementing the correct return values for Vista and higher. The
return values aren't broken in the sense that it is only one Windows version returning a different value.
In this case, it is Vista, 7, 8/8.1, 10.

I have no problem implementing the XP version in kernel32 -- in fact, I'll send a patch to do that -- but I'd
like other opinions (Alexandre?) on the overall issue, as it seems to be a special case.

Thanks for the feedback.

--
Hugh McMaster

[PATCH 3/4] kernel32/tests: Add tests for GetCurrentConsoleFont

[Alexandre Julliard]

Hugh McMaster  writes:
> The return values aren't broken in the sense that it is only one Windows
> version returning a different value. In this case, it is Vista, 7,
> 8/8.1, 10. I have no problem implementing the XP version in kernel32 -
> - in fact, I'll send a patch to do that -- but I'd like other opinions
>
> (Alexandre?) on the overall issue, as it seems to be a special case.

There are many differences between Windows versions, we can't afford to
have a version check for every difference. So it should be done only
when strictly necessary, i.e. there's an actual app that checks the
version and expects different behaviors based on that.