[v2] Add a GUI for dxdiag program
Matteo Bruni
matteo.mystral at gmail.com
Thu Sep 1 13:21:32 CDT 2016
2016-08-27 10:26 GMT+02:00 Ruslan Kabatsayev <b7.10110111 at gmail.com>:
> This adds a dialog with only System tab, reflecting what save-to-file
> functionality currently provides. It can also save the report as file
> from the GUI in a way similar to its Windows counterpart.
> This version now also shows DMI information which current dxdiagn.dll doesn't
> provide, but may do so in the future. Also some spacing issues are fixed.
>
> Signed-off-by: Ruslan Kabatsayev <b7.10110111 at gmail.com>
> ---
> programs/dxdiag/Makefile.in | 3 +-
> programs/dxdiag/dxdiag.rc | 64 +++++++++++
> programs/dxdiag/dxdiag_private.h | 9 +-
> programs/dxdiag/main.c | 20 ++--
> programs/dxdiag/main_dialog.c | 220 ++++++++++++++++++++++++++++++++++++++
> programs/dxdiag/resource.h | 56 ++++++++++
> 6 files changed, 356 insertions(+), 16 deletions(-)
> create mode 100644 programs/dxdiag/main_dialog.c
> create mode 100644 programs/dxdiag/resource.h
Hi Ruslan,
first of all, I think this patch should be splitted. For example, you
could split the "save all information" button functionality, together
with the common dialog include and dll linkage, to a separate patch.
Aside from that, I have some additional comments. I'm not exactly a
Win32 GUI expert so I might certainly have missed something
significant. Anyway...
> diff --git a/programs/dxdiag/Makefile.in b/programs/dxdiag/Makefile.in
> index 437cc5e..d971a55 100644
> --- a/programs/dxdiag/Makefile.in
> +++ b/programs/dxdiag/Makefile.in
> @@ -1,10 +1,11 @@
> MODULE = dxdiag.exe
> APPMODE = -mwindows -municode
> -IMPORTS = dxguid ole32 oleaut32 user32
> +IMPORTS = dxguid ole32 oleaut32 user32 comdlg32
Keep it sorted alphabetically.
>
> C_SRCS = \
> information.c \
> main.c \
> + main_dialog.c \
> output.c
Formatting.
> +IDD_SYSTEM_TAB DIALOG 0, 0, 387, 176
> +STYLE DS_SETFONT | WS_CHILDWINDOW
> +FONT 8, "Ms Shell Dlg"
> +{
> + GROUPBOX "System Information", IDC_SYSINFO_GROUP, 3, 65, 381, 136, 0, WS_EX_LEFT
> + LTEXT "If you know what area is causing the problem, click the appropriate tab above. Otherwise, you can use the ""Next Page"" button below to visit each page in sequence.", IDC_SYSTEMTAB_LABEL1, 9, 37, 344, 17, SS_LEFT, WS_EX_LEFT
> + LTEXT "This tool reports information about the DirectX components and drivers installed on your system. It lets you test functionality and diagnose problems.", IDC_SYSTEMTAB_LABEL2, 9, 11, 370, 17, SS_LEFT, WS_EX_LEFT
This text seems way too similar to the text in the original
application. I'm not sure it adds much, maybe just dropping it
entirely is fine?
> + RTEXT "Current Date/Time:", IDC_SYSTEMTAB_DATETIME_LABEL, 10, 74, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_DATETIME_VALUE, 153, 74, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "Computer name:", IDC_SYSTEMTAB_PCNAME_LABEL, 10, 84, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_PCNAME_VALUE, 153, 84, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "Operating System:", IDC_SYSTEMTAB_OS_LABEL, 10, 94, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_OS_VALUE, 153, 94, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "Language:", IDC_SYSTEMTAB_LANG_LABEL, 10, 104, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_LANG_VALUE, 153, 104, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "System Manufacturer:", IDC_SYSTEMTAB_VENDOR_LABEL, 10, 114, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_VENDOR_VALUE, 153, 114, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "System Model:", IDC_SYSTEMTAB_MODEL_LABEL, 10, 124, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_MODEL_VALUE, 153, 124, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "BIOS:", IDC_SYSTEMTAB_BIOS_LABEL, 10, 134, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_BIOS_VALUE, 153, 134, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "Processor:", IDC_SYSTEMTAB_CPU_LABEL, 10, 144, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_CPU_VALUE, 153, 144, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "Memory:", IDC_SYSTEMTAB_RAM_LABEL, 10, 154, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_RAM_VALUE, 153, 154, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "Page File:", IDC_SYSTEMTAB_PAGEFILE_LABEL, 10, 164, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_PAGEFILE_VALUE, 153, 164, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + RTEXT "DirectX Version:", IDC_SYSTEMTAB_DXVERSION_LABEL, 10, 174, 140, 9, SS_RIGHT, WS_EX_LEFT
> + LTEXT "", IDC_SYSTEMTAB_DXVERSION_VALUE, 153, 174, 227, 9, SS_LEFT, WS_EX_LEFT
> +
> + AUTOCHECKBOX "Check for WHQL digital signatures", IDC_WHQL_CHECKBOX, 10, 186, 369, 9, 0, WS_EX_LEFT
> }
Similarly here. Not sure about these, I'd drop the unused entries though.
> - WINE_FIXME("Information dialog is not implemented\n");
> + DialogBoxW(hInst,(WCHAR*)MAKEINTRESOURCE(IDD_MAIN),NULL,main_dlg_proc);
Space after the commas. Also you don't need to cast, just use
MAKEINTRESOURCEW() (see for example notepad/dialog.c).
> +extern HINSTANCE hInstance;
Please avoid camelcase or hungarian notation.
> +extern struct dxdiag_information * dxdiag_info;
The '*' should be next to the variable name, as in the preexisting
dxdiag sources.
> +void set_main_dialog_tab(int new_tab_index)
> +{
> + RECT tab_rect;
> + POINT top_left, bottom_right;
> + const HWND new_tab = main_dialog_tabs[new_tab_index];
> + int tab = 0;
> + for(;tab<MAIN_DIALOG_TAB_COUNT;++tab)
> + ShowWindow(main_dialog_tabs[tab], SW_HIDE);
Please leave a blank line between variable declarations and code.
Stick to consistently having a whitespace between the '<' and its
operands and after the ';'. Also, but this is probably just my
preference, I'd initialize tab to 0 in the "for" instead.
> +void main_dialog_insert_tab(int index, int caption_id, int template_id, DLGPROC dlg_proc)
> +{
> + WCHAR tab_caption[MAX_STRING_LEN];
> + const TCITEMW tab_item = {.pszText = tab_caption, .mask = TCIF_TEXT};
Designated initializers aren't allowed in C89.
> + LoadStringW(hInstance, caption_id, tab_caption, sizeof tab_caption/sizeof tab_caption[0]);
Parentheses around the sizeof operand.
> +void populate_system_tab(void)
> +{
> + const HWND page = main_dialog_tabs[SYSTEM_TAB_INDEX];
> + const struct system_information zeros = {0};
> + const struct system_information*const si = dxdiag_info ? &dxdiag_info->system_info : &zeros;
Spaces.
> +void save_info_dialog(HWND parent)
> +{
> + static const WCHAR filter[] = {'T','e','x','t',' ','f','i','l','e',' ','(','*','.','t','x','t',')',0, '*','.','t','x','t',0,
> + 'X','M','L',' ','f','i','l','e',' ','(','*','.','x','m','l',')',0, '*','.','x','m','l',0,0};
> + OPENFILENAMEW ofn = {0};
> + WCHAR filename[MAX_PATH];
> + filename[0] = 0;
You can initialize filename right in the definition, like:
WCHAR filename[MAX_PATH] = {0};
Actually, do you need to initialize it at all?
> + ofn.lStructSize = sizeof ofn;
> + ofn.hwndOwner = parent;
> + ofn.lpstrFilter = filter;
> + ofn.lpstrFile = filename;
> + ofn.nMaxFile = MAX_PATH;
> + ofn.Flags = OFN_EXPLORER|OFN_HIDEREADONLY|OFN_OVERWRITEPROMPT;
Spaces.
> +void update_dxdiag_info(void)
> +{
> + const HWND checkbox = GetDlgItem(main_dialog, IDC_WHQL_CHECKBOX);
> + const HWND save_all_button = GetDlgItem(main_dialog, IDC_SAVE_INFO);
> + const BOOL check_whql = SendMessageW(checkbox, BM_GETCHECK, 0L, 0L) == BST_CHECKED;
> + free_dxdiag_information(dxdiag_info);
> + dxdiag_info = collect_dxdiag_information(check_whql);
> + /* update the dialog before showing error message */
> + populate_tabs();
> + if(!dxdiag_info)
Nitpick, space after "if".
> +INT_PTR CALLBACK system_tab_proc(HWND hsystab, UINT msg, WPARAM wparam, LPARAM lparam)
> +{
> + switch(msg)
> + {
> + case WM_COMMAND:
> + switch(HIWORD(wparam))
> + {
> + case BN_CLICKED:
> + switch(wparam)
> + {
> + case IDC_WHQL_CHECKBOX:
> + {
> + update_dxdiag_info();
> + return TRUE;
> + }
> + }
This looks pretty ugly. You don't need a separate block inside the
case so you can just drop the inner braces.
Actually I don't think you're ever going to have other buttons in the
system tab so the switch seems unnecessary.
More information about the wine-devel
mailing list