Re: [PATCH 2/2] conhost: Popup Attributes should match Character Attributes when a new screen buffer is created

[Jacek Caban]

On 7/25/21 7:42 AM, Hugh McMaster wrote:
> Hi Jacek,
>
> On Fri, 23 Jul 2021 at 22:24, Jacek Caban wrote:
> > Hi Hugh,
> >
> > On 7/22/21 3:36 PM, Hugh McMaster wrote:
> > > @@ -104,6 +104,7 @@ static struct screen_buffer *create_screen_buffer( struct console *console, int
> > >            screen_buffer->win.right  = width - 1;
> > >            screen_buffer->win.bottom = height - 1;
> > >            screen_buffer->attr       = FOREGROUND_BLUE|FOREGROUND_GREEN|FOREGROUND_RED;
> > > -
> > >    screen_buffer->popup_attr = screen_buffer->attr;
> >
> > This part doesn't seem right. For windowed consoles this part is
> > overwritten when we read the config, but it's not the case for pseudo
> > consoles (or when we use Unix console). This breaks the attached test.
> > diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c
> > index 6ee00aa49a6..6e53355f6e5 100644
> > --- a/dlls/kernel32/tests/console.c
> > +++ b/dlls/kernel32/tests/console.c
> > @@ -4352,6 +4352,7 @@ static void test_AllocConsole(void)
> >
> > static void test_pseudo_console_child(HANDLE input, HANDLE output)
> > {
> > - CONSOLE_SCREEN_BUFFER_INFOEX sb_infoex;
> > CONSOLE_SCREEN_BUFFER_INFO sb_info;
> > CONSOLE_CURSOR_INFO cursor_info;
> > DWORD mode;
> >
> > @@ -4402,6 +4403,13 @@ static void test_pseudo_console_child(HANDLE input, HANDLE output)
> >      ok(sb_info.srWindow.Bottom == 29, "srWindow.Bottom = %u\n", sb_info.srWindow.Bottom);
> >      ok(sb_info.dwMaximumWindowSize.X == 40, "dwMaximumWindowSize.X = %u\n", sb_info.dwMaximumWindowSize.X);
> >      ok(sb_info.dwMaximumWindowSize.Y == 30, "dwMaximumWindowSize.Y = %u\n", sb_info.dwMaximumWindowSize.Y);
> > -
> >
> > ok(sb_info.wAttributes == 0x7, "wAttributes = %#x\n", sb_info.wAttributes);
> > -
> > -
> >
> > sb_infoex.cbSize = sizeof(sb_infoex);
> > -
> >
> > ret = GetConsoleScreenBufferInfoEx(output, &sb_infoex);
> > -
> >
> > ok(ret, "GetConsoleScreenBufferInfo failed: %u\n", GetLastError());
> > -
> >
> > ok(sb_infoex.wAttributes == 0x7, "wAttributes = %#x\n", sb_infoex.wAttributes);
> > -
> >
> > ok(sb_infoex.wPopupAttributes == 0xf5, "wPopupAttributes = %#x\n", sb_infoex.wPopupAttributes);
> >
> > ret = GetConsoleCursorInfo(output, &cursor_info);
> > ok(ret, "GetConsoleCursorInfo failed: %u\n", GetLastError());
>
> What is this testing? Currently, it just tells us what the color
> attributes and popup attributes are. There's nothing unexpected here.
>
> I ran a test using the --pseudo-console flag while creating a new
> screen buffer [1] and found the popup attributes take on the same
> value as the color attributes, in line with the other tests and patch.
>
> How should I be testing this?

Just run the test with no arguments, the test will create a pseudo 
console launch a child process attached to the pseudo console. It's a 
bit tricky, because you will not see printfs from failures on Wine, but 
you will still see it failing or not. The test shows that a fresh new 
pseudo console does not have default popup attributes equal to 
attributes (like in your patch).

Thanks,

Jacek