Replace format floppy disk with standard format drive dialog #108
Conversation
src/wfcomman.c
Outdated
|
|
||
| FormatDiskette(hwndFrame,FALSE); | ||
| // Don't use modal dialog box, set hWndParent = NULL | ||
| DialogBox(hAppInstance, (LPTSTR)MAKEINTRESOURCE(FORMATSELECTDLG), NULL, (DLGPROC)FormatSelectDlgProc); |
There was a problem hiding this comment.
In order for the dialog to be positioned like the others, we need to pass hwndFrame as the third parameter to DialogBox.
There was a problem hiding this comment.
@craigwi Good point, and I actually originally wrote the code that way. I later changed the parameter to NULL to avoid making this dialog modal. Really, I was more concerned about the dialog created by SHFormatDrive being modal, because formatting can be a long-running operation. SHFormatDrive requires a parent HWND (says MSDN), so I used the first dialog (the one you commented on) as its parent. Therefore, to prevent a situation where both dialogs were modal, I used NULL in the line you mentioned.
I see a few options:
- Just change the line you called out and make hWndParent=hwndFrame. That will solve the positioning issue, but will make the dialogs modal.
- Make your suggested change, and also change the call to SHFormatDrive so that it its parent HWND is NULL, to avoid the modal concern. This goes against MSDN's guidance, so I'm not sure what the side effects may be, although it seems to work.
- Make your suggested change, and also change the call to SHFormatDrive so that it its parent HWND is a hidden window. This avoids the modal problem while still complying with MSDN's recommendation. This is actually what explorer.exe seems to do, and it seems the proper thing to do, but I had avoided this approach before since involved extra window management.
What do you think?
There was a problem hiding this comment.
I suggest making the FORMATSELECTDLG modeless by calling CreateDialog instead of DialogBox. You can also save the hwnd returned to activate it so there is only one. I tried this out and the basic cases seem to work.
There are several other examples of modeless dialogs (e.g., CancelDlg) that you can use as a pattern.
I would like to see the FileCopy dialog becomes modelsss in this way. It has always annoyed me that it was modal.
There was a problem hiding this comment.
Good suggestion; thanks! I had forgotten about CreateDialog. It has been a while since I've written C/Win32 code! I'll look into making that change.
|
Completely agree on this and, eventually, copy and move dialog boxes being modeless. Disabling the use of the entire file manager, especially where time consuming file operations are happening doesn't really make a lot of sense. |
src/wfdlgs3.c
Outdated
| } | ||
| else | ||
| { | ||
| EndDialog(hDlg, IDOK); |
There was a problem hiding this comment.
According to the documentation (referenced at https://stackoverflow.com/questions/13788680/enddialog-vs-destroywindow), this needs to be DestroyWindow(). Also see the function DestroyCancelWindow() in wfdlgs3.c.
src/wfdlgs3.c
Outdated
| return TRUE; | ||
| } | ||
| case IDCANCEL: | ||
| EndDialog(hDlg, IDCANCEL); |
src/wfcomman.c
Outdated
| return TRUE; | ||
| else | ||
| { | ||
| SetActiveWindow(hwndFormatSelect); |
There was a problem hiding this comment.
Call ShowWindow(hwndFormatSelect, SW_SHOW) before SetActiveWindow(). The result is the dialog comes back on screen if hidden, but still model. Seems like the right behavior.
There was a problem hiding this comment.
Thinking through this... hwndFormatSelect is intentionally hidden while the SHFormatDrive dialog is visible. So, if the user currently has the SHFormatDrive dialog up (say they are formatting a drive) and they then click the "Format" menu item again, do we want to show hwndFormatSelect? If we do show it, then both hwndFormatSelect and SHFormatDrive will be visible at the same time, but hwndFormatSelect won't be usable, since it is the parent of the modal SHFormatDrive dialog. I think that is the behavior you are describing above, but I just want to be sure we are on the same page. I'm concerned about user confusion if we show a previously hidden dialog that isn't usable.
|
Good work! Made a couple of other comments on the code. Does the old FormatDiskette method still work? I know it is referenced in wfcopy.c. |
|
@craigwi Thanks, and I appreciate the comments! I'll submit a new PR once we work through the above question on hiding hwndFormatSelect. Regarding FormatDiskette, it should still work. I left the relevant code in place since it was called elsewhere. That said, I wasn't able to test it before or after my changes, since I don't have a floppy disk drive. |
|
You also need to add: || (hwndFormatSelect && to bDialogMessage() in winfile.c. This makes the keyboard (e.g., tab) work with modeless dialogs. As a note to others, to find this stuff I looked for all references to CancelInfo.bModal. |
|
@craigwi @matthewjustice whether the old code works is a great question, and I think I might be able to test this. Since I had already restored my Window Server 2008 R2 VM for win7 compat testing, I believe I can use that, as In fairly certain that VMware emulates virtual floppy disks. I'll report back with results, Insh'Allah. Oh, and this would be a nice opportunity to see if the new Window version of format disk is still able to format floppy disks. |
|
Thanks @craigwi for your patience and insight! I just made an update in commit f2dd108. Status of various issues:
Also, in my testing today I noticed another issue. If running non-elevated on Windows 7, a UAC prompt appears when SHFormatDrive is called. Once the UAC prompt is accepted, the SHFormatDrive dialog appears and works, but it is effectively modal. The main File Manager UI cannot be interacted with. Workaround: run File Manager elevated. I don't see this as a blocker, but I wanted to mention it. |
|
Hi @matthewjustice, just a heads up: you will need to create your new dialog in all language files that exist at the time you finish your PR. Right now that is two with several more coming soon. I don't expect you to provide all of the translations, but we would need to queue up the right folks post commit to finish them. |
|
EDIT: oops wrong tab |
|
So, does this mean that Win10 allows you to access format disk feature without UAC where as Windows 7 does? I wonder if Microsoft has documented the reasoning for this change. |
|
@craigwi - Thanks for the heads up. Looks like I'll need to move my English dialog text to winfile_en-US.dlg and any other English strings should be loaded from res_en-US.rc. As for the Simplified Chinese translation, I can add placeholders in res_zh-CN.rc and lang\winfile_zh-CN.dlg and perhaps @MouriNaruto can assist with translation. Does that sound like what you had in mind? Also, for this PR I'm looking for your feedback on the hidden window discussed above; please see my earlier post. Thanks! |
|
@NazmusLabs - Good question. In Windows 7 bringing up the format dialog from explorer.exe does not invoke UAC, but making the same call from winfile.exe does invoke UAC. Whereas on Windows 10 I don't see the UAC prompt in either scenario. I suspect it has more to do with changes to how UAC works, or default settings for UAC between versions of Windows, but I haven't had a chance to verify that assumption. |
|
@matthewjustice Okay, in order to ensure it's the differences in OS and not the UAC configuration you may have, I should give your build of Winfile a test on my Server 2008R2 VM and see what happens versus on my Win10 PC. |
|
My two last commits (d793d5f and 59c7b29) bring in the localization changes. For some reason, GitHub shows the overall diff of winfile_en-US.dlg as a complete replacement of the file, but if you look at the individual commits you'll see that I pulled in the copy from master as-is, and then added my dialog. I think that the GitHub diff tool is just having issues comparing the changes. Since I don't know Simplified Chinese, I did not include any localization changes, so the following updates need to be made later:
|
|
I'm not sure how to address the merge issue. Also note that there are new lang files to be handled. |
|
The last two commits (064ef57 and dcc01e3) bring in the ja-JP changes and resolve the merge conflict. I think this PR is ready to go, pending @craigwi's feedback on the hidden window discussed above... we need to come to an agreement on whether that suggested call to ShowWindow is desirable. My recommendation is that we leave the window hidden (as the PR current stands); my comments on this are above. Once this PR is accepted, the following localization changes need to be made:
|
|
Regarding whether to re-show the hidden window or not: when the formatting process has started (i.e., the Shell format dialog is showing), going back to WinFile and choosing Disk.Format again should do something. Just activating the hidden window without showing anything is kind of odd. Activating the Shell format dialog would be best since that is what is happening. If we can't activate the Shell format dialog, I think at least we should show and activate the disk selection dialog even though you would have to close the Shell format dialog to use it. Nice work! |
|
@craigwi Thanks for the confirmation of desired behavior. I agree that the ideal behavior would be to make the SHFormatDrive window active if it is opened. However, the HWND for that dialog isn't readily available. We could enumerate windows or maybe use a windows hook to find it, but I don't think the extra code complexity is worth it, at least for now. So I went with your original suggestion (see commit a52df7f). Maybe activation of the SHFormatDrive dialog is something for future consideration. |
|
Added dialog to the Polish translation, removed some trailing spaces and committed. |
|
Just wanted to mention, I had done some testing and I get an UAC prompt in Win10, just as you noted seeing it in Win7. Perhaps your UAC settings are different from default? You only get UAC for internal drives. No UAC for external drives in WIn10. Haven't checked external drives in Win7 |
|
@NazmusLabs - I think you found the difference. I was testing a internal drive on Windows 7 and an external drive for Windows 10! |
|
@matthewjustice thanks for confirming! |
For issue #62, the suggestion was made that File Manager display the standard Windows format dialog box when the user select the "Format Disk" menu option, which was originally designed for floppy disks. Some testing revealed that the "Format Disk" menu is disabled on modern systems without floppy disks, unless a removable storage USB device is present, in which case the menu is enabled. Even with the menu enabled, the old UI doesn't allow for formatting of non-floppy disks. The standard Windows dialog for formatting disks is invoked from SHFormatDrive, which unfortunately is deprecated. However, since explorer.exe relies on it, I went ahead and made this PR which invokes SHFormatDrive.
SHFormatDrive requires that the caller supply the drive that is to be formatted. Since we don't know ahead of time which drive the user may wish to format, another dialog was introduced to first allow the user to select a drive, and then invoke SHFormatDrive. This new dialog mimics the look and feel of the old UI. This new UI lists the drives for the user to select, but filters out CD/DVD drives and network shares.
The old UI:
The new UI:
Notes:
Known issue:
Workaround screenshot:
