2016-08-27 10:26 GMT+02:00 Ruslan Kabatsayev b7.10110111@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@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.
Hello,
On Thu, Sep 1, 2016 at 9:21 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2016-08-27 10:26 GMT+02:00 Ruslan Kabatsayev b7.10110111@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@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.
Hmm, and disable the Save button until that 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.
What a trick. I seemed to indent it correctly, but now I see the original file had tabs instead of spaces :/
+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?
Not sure. There'll be lots of empty space on the tab. I tried to mimic the original dialog closely enough but still not copying.
- 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.
The unused entries are not really unused: they are used in the original (GUI-less) version when creating the report file. They are just not filled (i.e. are created empty) by dxdiagn.dll.
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).
I did at first, but with gcc 5 I got ugly warnings.
+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};
This has different semantics from my code. My code only assigns zero to first character, yours variant initializes the whole array (see C90, 6.5.7 Initialization).
Actually, do you need to initialize it at all?
In fact, maybe not. I was first thinking of the possibility of GetSaveFileNameW somehow failing so that subsequent use of filename would result in UB, but now I see that this scenario is guarded by the if clause.
Regards, Ruslan
2016-09-01 21:56 GMT+02:00 Ruslan Kabatsayev b7.10110111@gmail.com:
Hello,
On Thu, Sep 1, 2016 at 9:21 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
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.
Hmm, and disable the Save button until that patch?..
That's okay, yes.
Also you don't need to cast, just use MAKEINTRESOURCEW() (see for example notepad/dialog.c).
I did at first, but with gcc 5 I got ugly warnings.
Ah, interesting. Does notepad also generate those warnings?
On Thu, Sep 1, 2016 at 11:03 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
Also you don't need to cast, just use MAKEINTRESOURCEW() (see for example notepad/dialog.c).
I did at first, but with gcc 5 I got ugly warnings.
Ah, interesting. Does notepad also generate those warnings?
No, it doesn't. I see now, I didn't notice the difference between what I did and your suggestion (MAKEINTRESOURCE vs MAKEINTRESOURCEW). It's only the former which triggers the warning (and from winuser.h, this seems intentional).
-
Matteo Bruni -
Ruslan Kabatsayev