[PATCH v2 1/3] conhost: Save console settings as global defaults on first run
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 <hugh.mcmaster(a)outlook.com> --- 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 ); 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 ); + return 0; } } -- 2.35.2
On Windows, app-specific subkeys of HKCU\Console only hold console settings that differ from the global defaults. Signed-off-by: Hugh McMaster <hugh.mcmaster(a)outlook.com> --- programs/conhost/window.c | 139 +++++++++++++++++++++++++++----------- 1 file changed, 98 insertions(+), 41 deletions(-) diff --git a/programs/conhost/window.c b/programs/conhost/window.c index 130be396820..8dbb92732fe 100644 --- a/programs/conhost/window.c +++ b/programs/conhost/window.c @@ -280,69 +280,127 @@ static void load_config( const WCHAR *key_name, struct console_config *config ) TRACE( "%s\n", debugstr_config( config )); } -static void save_registry_key( HKEY key, const struct console_config *config ) +#define CMP(x) (config->x != defconfig.x) + +static void save_registry_key( HKEY key, const struct console_config *config, BOOL save_all ) { + struct console_config defconfig; DWORD val, width, height, i; WCHAR color_name[13]; TRACE( "%s\n", debugstr_config( config )); + if (!save_all) + load_config( NULL, &defconfig ); + for (i = 0; i < ARRAY_SIZE(config->color_map); i++) { - wsprintfW( color_name, L"ColorTable%02d", i ); - val = config->color_map[i]; - RegSetValueExW( key, color_name, 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(color_map[i])) + { + wsprintfW( color_name, L"ColorTable%02d", i ); + val = config->color_map[i]; + RegSetValueExW( key, color_name, 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } } - val = config->cursor_size; - RegSetValueExW( key, L"CursorSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(cursor_size)) + { + val = config->cursor_size; + RegSetValueExW( key, L"CursorSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->cursor_visible; - RegSetValueExW( key, L"CursorVisible", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(cursor_visible)) + { + val = config->cursor_visible; + RegSetValueExW( key, L"CursorVisible", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->edition_mode; - RegSetValueExW( key, L"EditionMode", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(edition_mode)) + { + val = config->edition_mode; + RegSetValueExW( key, L"EditionMode", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - RegSetValueExW( key, L"FaceName", 0, REG_SZ, (BYTE *)&config->face_name, - (lstrlenW(config->face_name) + 1) * sizeof(WCHAR) ); + if (save_all || lstrcmpW( config->face_name, defconfig.face_name )) + { + RegSetValueExW( key, L"FaceName", 0, REG_SZ, (BYTE *)&config->face_name, + (lstrlenW(config->face_name) + 1) * sizeof(WCHAR) ); + } - val = config->font_pitch_family; - RegSetValueExW( key, L"FontPitchFamily", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(font_pitch_family)) + { + val = config->font_pitch_family; + RegSetValueExW( key, L"FontPitchFamily", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - width = MulDiv( config->cell_width, USER_DEFAULT_SCREEN_DPI, GetDpiForSystem() ); - height = MulDiv( config->cell_height, USER_DEFAULT_SCREEN_DPI, GetDpiForSystem() ); - val = MAKELONG( width, height ); - RegSetValueExW( key, L"FontSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(cell_width) || CMP(cell_height)) + { + width = MulDiv( config->cell_width, USER_DEFAULT_SCREEN_DPI, GetDpiForSystem() ); + height = MulDiv( config->cell_height, USER_DEFAULT_SCREEN_DPI, GetDpiForSystem() ); + val = MAKELONG( width, height ); + + RegSetValueExW( key, L"FontSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->font_weight; - RegSetValueExW( key, L"FontWeight", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(font_weight)) + { + val = config->font_weight; + RegSetValueExW( key, L"FontWeight", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->history_size; - RegSetValueExW( key, L"HistoryBufferSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(history_size)) + { + val = config->history_size; + RegSetValueExW( key, L"HistoryBufferSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->history_mode; - RegSetValueExW( key, L"HistoryNoDup", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(history_mode)) + { + val = config->history_mode; + RegSetValueExW( key, L"HistoryNoDup", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->insert_mode; - RegSetValueExW( key, L"InsertMode", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(insert_mode)) + { + val = config->insert_mode; + RegSetValueExW( key, L"InsertMode", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->menu_mask; - RegSetValueExW( key, L"MenuMask", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(menu_mask)) + { + val = config->menu_mask; + RegSetValueExW( key, L"MenuMask", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->popup_attr; - RegSetValueExW( key, L"PopupColors", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(popup_attr)) + { + val = config->popup_attr; + RegSetValueExW( key, L"PopupColors", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->quick_edit; - RegSetValueExW( key, L"QuickEdit", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(quick_edit)) + { + val = config->quick_edit; + RegSetValueExW( key, L"QuickEdit", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = MAKELONG(config->sb_width, config->sb_height); - RegSetValueExW( key, L"ScreenBufferSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(sb_width) || CMP(sb_height)) + { + val = MAKELONG(config->sb_width, config->sb_height); + RegSetValueExW( key, L"ScreenBufferSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = config->attr; - RegSetValueExW( key, L"ScreenColors", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(attr)) + { + val = config->attr; + RegSetValueExW( key, L"ScreenColors", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } - val = MAKELONG( config->win_width, config->win_height ); - RegSetValueExW( key, L"WindowSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + if (save_all || CMP(win_width) || CMP(win_height)) + { + val = MAKELONG( config->win_width, config->win_height ); + RegSetValueExW( key, L"WindowSize", 0, REG_DWORD, (BYTE *)&val, sizeof(val) ); + } } static void save_config( const WCHAR *key_name, const struct console_config *config ) @@ -365,12 +423,11 @@ static void save_config( const WCHAR *key_name, const struct console_config *con } else { - /* FIXME: maybe only save the values different from the default value ? */ - save_registry_key( app_key, config ); + save_registry_key( app_key, config, FALSE ); RegCloseKey( app_key ); } } - else save_registry_key( key, config ); + else save_registry_key( key, config, TRUE ); RegCloseKey(key); } -- 2.35.2
Hi Hugh, On 4/19/22 13:26, Hugh McMaster wrote:
+#define CMP(x) (config->x != defconfig.x)
Please avoid macros like that, it doesn't really make the code easier to read. Thanks, Jacek
On Windows, app-specific console settings are always saved to the registry unless the user cancels the console properties dialog. Signed-off-by: Hugh McMaster <hugh.mcmaster(a)outlook.com> --- programs/conhost/conhost.h | 4 --- programs/conhost/conhost.rc | 12 ------- programs/conhost/window.c | 69 +++++-------------------------------- 3 files changed, 9 insertions(+), 76 deletions(-) diff --git a/programs/conhost/conhost.h b/programs/conhost/conhost.h index 9f2a90cd510..4464f51032f 100644 --- a/programs/conhost/conhost.h +++ b/programs/conhost/conhost.h @@ -186,7 +186,6 @@ static inline unsigned int get_bounded_cursor_x( struct screen_buffer *screen_bu #define IDD_OPTION 0x0100 #define IDD_FONT 0x0200 #define IDD_CONFIG 0x0300 -#define IDD_SAVE_SETTINGS 0x0400 /* dialog boxes controls */ #define IDC_OPT_CURSOR_SMALL 0x0101 @@ -217,6 +216,3 @@ static inline unsigned int get_bounded_cursor_x( struct screen_buffer *screen_bu #define IDC_CNF_WIN_HEIGHT_UD 0x0308 #define IDC_CNF_CLOSE_EXIT 0x0309 #define IDC_CNF_EDITION_MODE 0x030a - -#define IDC_SAV_SAVE 0x0401 -#define IDC_SAV_SESSION 0x0402 diff --git a/programs/conhost/conhost.rc b/programs/conhost/conhost.rc index e7c4416b634..820220b9d60 100644 --- a/programs/conhost/conhost.rc +++ b/programs/conhost/conhost.rc @@ -114,18 +114,6 @@ FONT 8, "MS Shell Dlg" COMBOBOX IDC_CNF_EDITION_MODE, 119, 69, 75, 60, CBS_DROPDOWNLIST|WS_VSCROLL|WS_TABSTOP } -IDD_SAVE_SETTINGS DIALOG 20, 20, 210, 60 -STYLE DS_MODALFRAME | WS_POPUP | WS_CAPTION -CAPTION "Console parameters" -FONT 8, "MS Shell Dlg" -{ - AUTORADIOBUTTON "Retain these settings for later sessions", IDC_SAV_SAVE, 10, 8, 200, 10, WS_TABSTOP - AUTORADIOBUTTON "Modify only current session", IDC_SAV_SESSION, 10, 22, 200, 10, WS_TABSTOP - - DEFPUSHBUTTON "OK", IDOK, 100, 43, 50, 14 - PUSHBUTTON "Cancel", IDCANCEL, 155, 43, 50, 14 -} - LANGUAGE LANG_NEUTRAL, SUBLANG_NEUTRAL #define WINE_FILEDESCRIPTION_STR "Wine conhost" diff --git a/programs/conhost/window.c b/programs/conhost/window.c index 8dbb92732fe..0b546acaa39 100644 --- a/programs/conhost/window.c +++ b/programs/conhost/window.c @@ -1759,34 +1759,6 @@ static INT_PTR WINAPI config_dialog_proc( HWND dialog, UINT msg, WPARAM wparam, return TRUE; } -/* dialog proc for choosing how to handle modification to the console settings */ -static INT_PTR WINAPI save_dialog_proc( HWND dialog, UINT msg, WPARAM wparam, LPARAM lparam ) -{ - switch (msg) - { - case WM_INITDIALOG: - SendMessageW( dialog, WM_NEXTDLGCTL, (WPARAM)GetDlgItem( dialog, IDC_SAV_SESSION ), TRUE ); - SendDlgItemMessageW( dialog, IDC_SAV_SESSION, BM_SETCHECK, BST_CHECKED, 0 ); - return FALSE; - - case WM_COMMAND: - switch (LOWORD(wparam)) - { - case IDOK: - EndDialog( dialog, - (IsDlgButtonChecked(dialog, IDC_SAV_SAVE) == BST_CHECKED) ? - IDC_SAV_SAVE : IDC_SAV_SESSION ); - break; - case IDCANCEL: - EndDialog( dialog, IDCANCEL ); break; - } - break; - default: - return FALSE; - } - return TRUE; -} - static void apply_config( struct console *console, const struct console_config *config ) { if (console->active->width != config->sb_width || console->active->height != config->sb_height) @@ -1904,19 +1876,17 @@ static BOOL config_dialog( struct console *console, BOOL current ) PROPSHEETPAGEW psp; WNDCLASSW wndclass; WCHAR buff[256]; - BOOL modify_session = FALSE; - BOOL save = FALSE; InitCommonControls(); memset( &di, 0, sizeof(di) ); di.console = console; - if (!current) - { + + if (current) + current_config( console, &di.config ); + else load_config( NULL, &di.config ); - save = TRUE; - } - else current_config( console, &di.config ); + prev_config = di.config; wndclass.style = 0; @@ -1982,35 +1952,14 @@ static BOOL config_dialog( struct console *console, BOOL current ) TRACE( "%s\n", debugstr_config(&di.config) ); - if (!save) - { - switch (DialogBoxW( GetModuleHandleW( NULL ), MAKEINTRESOURCEW(IDD_SAVE_SETTINGS), - console->win, save_dialog_proc )) - { - case IDC_SAV_SAVE: - save = TRUE; - modify_session = TRUE; - break; - case IDC_SAV_SESSION: - modify_session = TRUE; - break; - default: - ERR( "dialog failed\n" ); - /* fall through */ - case IDCANCEL: - modify_session = FALSE; - save = FALSE; - break; - } - } - - if (modify_session) + if (current) { apply_config( console, &di.config ); update_window( di.console ); } - if (save) - save_config( current ? console->window->config_key : NULL, &di.config ); + + save_config( current ? console->window->config_key : NULL, &di.config ); + return TRUE; } -- 2.35.2
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 <hugh.mcmaster(a)outlook.com> --- 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.
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? Thanks, Jacek
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
participants (2)
-
Hugh McMaster -
Jacek Caban