[PATCH v2 1/3] conhost: Save console settings as global defaults on first run

[Hugh McMaster]

Hi Jacek,

On Wed, 20 Apr 2022 at 02:06, Jacek Caban wrote:
>
> On 4/19/22 13:26, Hugh McMaster wrote:
> > On Windows, HKCU\Console holds global default settings, while subkeys hold
> > any app-specific settings that differ from the defaults.
> >
> > Wine's conhost.exe implementation currently saves all console settings to
> > an app-specific subkey on the first run, while global defaults are only
> > saved to HKCU\Console if the user selects the Default pop-up menu option.
> > This is the opposite of the behaviour on Windows.
> >
> > Signed-off-by: Hugh McMaster
> > ---
> >   programs/conhost/window.c | 12 +++++-------
> >   1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/programs/conhost/window.c b/programs/conhost/window.c
> > index fb02ef9fd94..130be396820 100644
> > --- a/programs/conhost/window.c
> > +++ b/programs/conhost/window.c
> > @@ -817,9 +817,9 @@ static int WINAPI get_first_font_sub_enum( const LOGFONTW *lf, const TEXTMETRICW
> >
> >               fc->done = 1;
> >
> > -            /* since we've modified the current config with new font information,
> > -             * set this information as the new default.
> > -             */
> > +            /* Save default font and console configuration to the registry */
> > +            load_config( NULL, &config );
> > +
> >               load_config( fc->console->window->config_key, &config );
>
>
> The new call seems to be no-op, the second load_config() will replace
> the result of the first one.

Sorry about that. I did this patch quickly by hand and clearly didn't
test it before sending.

>
> >               config.cell_width  = fc->console->active->font.width;
> >               config.cell_height = fc->console->active->font.height;
> > @@ -827,10 +827,8 @@ static int WINAPI get_first_font_sub_enum( const LOGFONTW *lf, const TEXTMETRICW
> >                       fc->console->active->font.face_len * sizeof(WCHAR) );
> >               config.face_name[fc->console->active->font.face_len] = 0;
> >
> > -            /* Force also its writing back to the registry so that we can get it
> > -             * the next time.
> > -             */
> > -            save_config( fc->console->window->config_key, &config );
> > +            save_config( NULL, &config );
> > +
>
>
> While you mention first run in the commit message, note that this is
> used in set_output_info() code path, so it will modify registry in
> response to SetCurrentConsoleFontEx() call. What's the desired effect of
> that? An app-specific key seems more appropriate for that case, but
> maybe we shouldn't write to registry at all?

SetCurrentConsoleFontEx() only alters the active session settings; it
does not modify console registry settings. However, the call to
SetCurrentConsoleFontEx() could end up modifying the registry settings
if CreateFontIndirectW() fails, which is extremely unlikely since it
selects a font as close as possible to the LOGFONT parameters.

On Windows, passing an invalid font face name via CONSOLE_FONT_INFOEX
generally results in the existing font remaining unchanged. On Windows
10 1809 or more recent, the font seems to change to a default font,
Courier New. See [1] for some tests.

Wine behaves similarly here. On my system, a simple test program set
the font from Liberation Mono to DejaVu Sans Mono.

If you're concerned, it might be better to create a new code path for
set_output_info(). The call could check the returned value of
set_console_font() and, if it failed (however unlikely that is), make
a second call using the current active font (or get the first valid
font).

That proposal probably didn't make a lot of sense, so I might prepare
a patch to make it clear. Let me know if you have a preference for
using the active font as a fallback or getting the first valid font.

I was planning on making updates to the default font code later, but
it seems better to fix this now.

Hugh

[1] https://testbot.winehq.org/JobDetails.pl?Key=112939