[v2] Add a GUI for dxdiag program
Ruslan Kabatsayev
b7.10110111 at gmail.com
Thu Sep 1 14:56:16 CDT 2016
Hello,
On Thu, Sep 1, 2016 at 9:21 PM, Matteo Bruni <matteo.mystral at gmail.com> wrote:
> 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.
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
More information about the wine-devel
mailing list