Re: kernel32/tests: Add initial tests for GetConsoleFontInfo (v3)

[Sebastian Lackner]

I am aware that this patch is already committed, but since you are probably
still working on the follow-up patches, here some remarks:

On 26.04.2016 14:06, Hugh McMaster wrote:
> - GetCurrentConsoleFont(std_output, FALSE, &cfi[0]);
> - index = cfi[0].nFont;
> - orig_font = GetConsoleFontSize(std_output, index);
> -

You might want to do a memset here. Currently some of the tests pass,
although Wine has no implementation yet.
> - SetLastError(0xdeadbeef);
> - ret = pGetConsoleFontInfo(std_output, FALSE, num_fonts, cfi);
> - todo_wine ok(ret, "got %d, expected non-zero\n", ret);
> - todo_wine ok(GetLastError() == 0xdeadbeef, "got %u, expected 0xdeadbeef\n", GetLastError());
> -
> - ok(cfi[index].dwFontSize.X == win_width, "got %d, expected %d\n", cfi[index].dwFontSize.X, win_width);
> - ok(cfi[index].dwFontSize.Y == win_height, "got %d, expected %d\n", cfi[index].dwFontSize.Y, win_height);

Could you add an additional test that (cfi[i].nFont == i) holds?
If GetConsoleFontInfo would return elements in arbitrary order, the
code above would not work.
> -
> - for (i = 0; i < num_fonts; i++)
> - {
> -
>    tmp_font = GetConsoleFontSize(std_output, cfi[i].nFont);
> -
>    tmp_w = (double)orig_font.X / tmp_font.X * win_width;
> -
>    tmp_h = (double)orig_font.Y / tmp_font.Y * win_height;
> -
>    ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> -
>    ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);
> - }

It wouldn't hurt to HeapFree() cfi here.
> +}
> -
>
> START_TEST(console)
>  {
>      static const char font_name[] = "Lucida Console";
> @@ -2925,4 +2998,5 @@ START_TEST(console)
>      test_GetCurrentConsoleFont(hConOut);
>      test_GetConsoleFontSize(hConOut);
>      test_GetLargestConsoleWindowSize(hConOut);
> - test_GetConsoleFontInfo(hConOut);
>
> }

kernel32/tests: Add initial tests for GetConsoleFontInfo (v3)

[Hugh McMaster]

On Wednesday, 27 April 2016 2:41 AM, Sebastian Lackner wrote:
> I am aware that this patch is already committed, but since you are probably
> still working on the follow-up patches, here some remarks:
> On 26.04.2016 14:06, Hugh McMaster wrote:
> > - GetCurrentConsoleFont(std_output, FALSE, &cfi[0]);
> > - index = cfi[0].nFont;
> > - orig_font = GetConsoleFontSize(std_output, index);
> You might want to do a memset here. Currently some of the tests pass,
> although Wine has no implementation yet.

Do you mean to clear cfi[0] after calling GetCurrentConsoleFont?
> > - SetLastError(0xdeadbeef);
> > - ret = pGetConsoleFontInfo(std_output, FALSE, num_fonts, cfi);
> > - todo_wine ok(ret, "got %d, expected non-zero\n", ret);
> > - todo_wine ok(GetLastError() == 0xdeadbeef, "got %u, expected 0xdeadbeef\n", GetLastError());
> > -
> > - ok(cfi[index].dwFontSize.X == win_width, "got %d, expected %d\n", cfi[index].dwFontSize.X, win_width);
> > - ok(cfi[index].dwFontSize.Y == win_height, "got %d, expected %d\n", cfi[index].dwFontSize.Y, win_height);
> Could you add an additional test that (cfi[i].nFont == i) holds?
> If GetConsoleFontInfo would return elements in arbitrary order, the
> code above would not work.

I'm not sure I understand what you're asking. Should I run the loop again?

The TRUE/FALSE argument doens't seem to have an effect on the order of elements returned.
If you set a different font size, cfi[new font index].dwFontSize.X == win_width. The remaining dwFontSize
members are set relative to that font size.
> > - for (i = 0; i < num_fonts; i++)
> > - {
> > -
> >    tmp_font = GetConsoleFontSize(std_output, cfi[i].nFont);
> > -
> >    tmp_w = (double)orig_font.X / tmp_font.X * win_width;
> > -
> >    tmp_h = (double)orig_font.Y / tmp_font.Y * win_height;
> > -
> >    ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> > -
> >    ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);
> > - }
> It wouldn't hurt to HeapFree() cfi here.

Yes, sorry, I missed that. I split the patch and forgot about the HeapFree().
I'll add it when I send tests with the TRUE argument.

kernel32/tests: Add initial tests for GetConsoleFontInfo (v3)

[Sebastian Lackner]

On 27.04.2016 15:24, Hugh McMaster wrote:
> On Wednesday, 27 April 2016 2:41 AM, Sebastian Lackner wrote:
> > I am aware that this patch is already committed, but since you are probably
> > still working on the follow-up patches, here some remarks:
> > On 26.04.2016 14:06, Hugh McMaster wrote:
> > > - GetCurrentConsoleFont(std_output, FALSE, &cfi[0]);
> > > - index = cfi[0].nFont;
> > > - orig_font = GetConsoleFontSize(std_output, index);
> > You might want to do a memset here. Currently some of the tests pass,
> > although Wine has no implementation yet.
>
> Do you mean to clear cfi[0] after calling GetCurrentConsoleFont?

Yes, or even better the whole memory. HeapAlloc does not initialize it.
> > > - SetLastError(0xdeadbeef);
> > > - ret = pGetConsoleFontInfo(std_output, FALSE, num_fonts, cfi);
> > > - todo_wine ok(ret, "got %d, expected non-zero\n", ret);
> > > - todo_wine ok(GetLastError() == 0xdeadbeef, "got %u, expected 0xdeadbeef\n", GetLastError());
> > > -
> > > - ok(cfi[index].dwFontSize.X == win_width, "got %d, expected %d\n", cfi[index].dwFontSize.X, win_width);
> > > - ok(cfi[index].dwFontSize.Y == win_height, "got %d, expected %d\n", cfi[index].dwFontSize.Y, win_height);
> > Could you add an additional test that (cfi[i].nFont == i) holds?
> > If GetConsoleFontInfo would return elements in arbitrary order, the
> > code above would not work.
>
> I'm not sure I understand what you're asking. Should I run the loop again?

No, you can add it into the loop below. To make a bit more clear what I mean:
You initialize "index" based on the nFont value, but here you access based on
the array index. Its not immediately obvious that the array index and nFont
value is the same.
> The TRUE/FALSE argument doens't seem to have an effect on the order of elements returned.
> If you set a different font size, cfi[new font index].dwFontSize.X == win_width. The remaining dwFontSize
> members are set relative to that font size.
> > > - for (i = 0; i < num_fonts; i++)
> > > - {
> > > -
> > >    tmp_font = GetConsoleFontSize(std_output, cfi[i].nFont);
> > > -
> > >    tmp_w = (double)orig_font.X / tmp_font.X * win_width;
> > > -
> > >    tmp_h = (double)orig_font.Y / tmp_font.Y * win_height;
> > > -
> > >    ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> > > -
> > >    ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);
> > > - }
> > It wouldn't hurt to HeapFree() cfi here.
>
> Yes, sorry, I missed that. I split the patch and forgot about the HeapFree().
> I'll add it when I send tests with the TRUE argument.