August 2025 - Wine-GitLab - list.winehq.org

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > + if (changed) > + { > + save_cfg_data(NULL, path, SINGLE, opt.optno); > + InitializeDialog(hwnd); > + } > free( value ); > } > > +LRESULT CALLBACK SaveWindowProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam) > +{ > + switch (uMsg) > + { > + case WM_CREATE: > + { > + RECT rcClient; > + HWND tag, edit, btnSave, btnLoad; This seems like unnecessarily complicated UI to me. Why not just have Save and Load buttons in the main window that bring up standard Open/Save dialogs? (And maybe a "Save Defaults" that writes it to the default location. That could be added first, with the extra dialogs added in a later commit.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113179

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > + WideCharToMultiByte(CP_UTF8, 0, opt.title, -1, title, sizeof(title), NULL, NULL); > + > if (opt.type == TYPE_STRING && opt.constraint_type != CONSTRAINT_NONE) > { > CHAR buffer[255]; > WCHAR *p; > > + BOOL is_exist = load_from_file(path, opt.type, title, buffer); > + BOOL is_correct = FALSE; > + > for (p = opt.constraint.strings; *p; p += lstrlenW(p) + 1) > + { > + CHAR param[256]; > SendMessageW( control,CB_ADDSTRING,0, (LPARAM)p ); > + WideCharToMultiByte(CP_UTF8, 0, p, -1, param, sizeof(param), NULL, NULL); > + if (!strcmp(param, buffer)) This will access buffer uninitialized if the option is not in the file. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113178

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > + BOOL is_exist = load_from_file(path, opt.type, title, &b); > + > + if (is_exist) > + { > + sane_option_set_value( i, &b, NULL ); > + } > + > sane_option_get_value( i, &b ); > + > if (b) > + { > SendMessageA(control,BM_SETCHECK,BST_CHECKED,0); > - > + continue; > + } > + SendMessageA(control,BM_SETCHECK,BST_UNCHECKED,0); This could be simplified to `SendMessageA(control,BM_SETCHECK,b ? BST_CHECKED : BST_UNCHECKED,0);` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113177

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > + option->type = opt->type; > + option->value.int_val = pos; > + free(sf); > + } > + } > +} > + > +static BOOL save_cfg_data(HWND hwnd, WCHAR file_path[128], int type, int optno) > +{ > + ScannerOption *options = malloc((cntOptionsExist + 1) * sizeof(ScannerOption)); > + if (!options) { > + ERR("Failed to allocate memory for options array\n"); > + return FALSE; > + } > + > + switch(type) This reads to me like 2 entirely different functions with different interfaces that have been combined into 1 with a switch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113176

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > + sf = calloc( opt->size, sizeof(int) ); > + sane_option_get_value(opt->optno, sf ); > + if (opt->constraint.range.quant) > + pos = *sf / opt->constraint.range.quant; > + else > + pos = MulDiv( *sf, 100, 65536 ); > + option->type = opt->type; > + option->value.int_val = pos; > + free(sf); > + } > + } > +} > + > +static BOOL save_cfg_data(HWND hwnd, WCHAR file_path[128], int type, int optno) > +{ > + ScannerOption *options = malloc((cntOptionsExist + 1) * sizeof(ScannerOption)); This is never freed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113175

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > return FALSE; > } > > +static void set_value(struct option_descriptor* opt, ScannerOption* option) This is called `set_value` but it gets the value? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113174

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > if (rc != TWCC_SUCCESS) > { > ERR("Unable to read number of options\n"); > - return FALSE; > + optcount = gOptCount; What situation causes this failure, and why is using a previous cached value OK? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113172

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/cfg.h: > +{ > + int type; > + CHAR name[64]; > + int optno; > + union > + { > + int int_val; > + BOOL bool_val; > + CHAR str_val[255]; > + int fixed_val; > + } value; > + BOOL is_enabled; > +} ScannerOption; > + > +BOOL save_to_file(WCHAR* path, ScannerOption *option); > +BOOL load_from_file(WCHAR* path, int type, CHAR* name, void* value); This interface seems like it would be inefficient - each setting saved is a complete read and write of the file, and each setting loaded is a complete read. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113173

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > } > > control_len += len + padding; > + optionsExist[cntOptionsExist] = opt.optno; `optionsExist` is not bounds-checked. `cntOptionsExist` is never reset to 0 if `create_options_page` is called multiple times. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113171

1 0

Re: [PATCH v13 0/1] MR8778: sane.ds: Adding a window for loading and saving the scanner configuration
by Esme Povirk (＠madewokherd) Aug. 19, 2025

Aug. 19, 2025

Esme Povirk (@madewokherd) commented about dlls/sane.ds/ui.c: > > +#include "cfg.h" > + > WINE_DEFAULT_DEBUG_CHANNEL(twain); > > #define ID_BASE 0x100 > #define ID_EDIT_BASE 0x1000 > #define ID_STATIC_BASE 0x2000 > +#define ID_APPLY_NOW 0x3021 > + > +#define ID_SAVE_CFG 0x8337 > +#define ID_EDIT_FILENAME 0x8338 > +#define ID_BUTTON_SAVE 0x8339 > +#define ID_BUTTON_LOAD 0x8340 > + > +#define CSIDL_LOCAL_APPDATA 0x001c Why not include the header that defines this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8778#note_113170

1 0