Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose cancel state of FileDialog and DirectoryDialog #1026

Merged

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Feb 2, 2024

The FileDialog and DirectoryDialog implementations do currently not provide any way to identify that the dialog was canceled. The dialogs return a null string in case no file or folder was selected, but is not possible to identify whether this was intended (via a cancel action) or if there was some unexpected error while the dialog was open (such as too long file paths on Windows).

This change introduces the openDialog() method as an alternative to the existing open() method. This new method returns an optional String, which is only present if a file or directory was successfully selected. An empty Optional indicates a cancellation by the user and an actual error is realized as an SWTException. The dialogs evaluate the operating system's response code for the system's dialog control to identify the dialogs result state.

This PR is a result of the proposal by @laeubi in #1005 (comment). Supercedes and thus closes #1005.

Copy link
Contributor

github-actions bot commented Feb 2, 2024

Test Results

   299 files  ±0     299 suites  ±0   6m 18s ⏱️ - 1m 50s
 4 099 tests ±0   4 091 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 209 runs  ±0  12 134 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit e6bb8b0. ± Comparison against base commit 0532901.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some suggestions/thoughts ...

@HeikoKlare HeikoKlare force-pushed the filedialog-open-with-cancel branch 2 times, most recently from bff9b11 to 31937f7 Compare February 2, 2024 11:49
@laeubi
Copy link
Contributor

laeubi commented Feb 2, 2024

Thinking more, maybe the result class is not required and one could have a singel method

Optional<String askForFile/Folder()

that in case of error throws an exception with a meaningful message (e.g. path is to long, IO exception seems suitable if SWT Exception seems not well suited) and an empty optional on cancel, then all cases should be covered.

@HeikoKlare HeikoKlare force-pushed the filedialog-open-with-cancel branch 2 times, most recently from 4306e90 to 9641b01 Compare February 2, 2024 15:13
@HeikoKlare
Copy link
Contributor Author

Thanks for all the input, @laeubi! I tend to completely agree with your point of view.

In particular, I took a look at the kinds of "errors" that may happen and found that the currently used OS APIs only seem to provide "OK" and "Cancel", and maybe something else if there really is a error (that may then reasonable be thrown as an SWTException). I took the according information from here:
Windows: https://learn.microsoft.com/en-us/windows/win32/api/shobjidl_core/nf-shobjidl_core-imodalwindow-show
Linux/GTK: https://docs.gtk.org/gtk3/method.NativeDialog.run.html
Mac: https://developer.apple.com/documentation/appkit/nssavepanel/1525357-runmodal
And on Windows, I manually tried to produce errors that have been delivered as such in the "old" file dialog implementation (for the result types, see here), but all these kinds of errors are now already handled by the dialog itself, so that you cannot even close the dialog pressing "Ok" with, e.g., an invalid file name. My contribution was driven by the fact that our RCP product still uses SWT with the "old" Windows file dialog implementation, where we had to explicitly handle different kinds of error cases, but this does not seem to be necessary with the current implementation anyway.

So I changed the implementation to only return an Optional result and throw an exception in other cases in 9641b01.
One drawback is that this complicates the realization of the existing open() method. To be API preserving, it has to distinguish between existing SWTExceptions and those potentially thrown due to an error result (i.e., not OK and not Cancel) of the dialog. So either this information has to be wrapped in a result object of some private method or it has to be extracted from the SWTException. I followed the latter approach to not bloat up the code, but it is actually a bit unclean.

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks fine for me just a bit unsure about the naming but maybe others like to give advice.

@@ -182,7 +212,12 @@ public String open() {
fileDialog.Release();
}

return directoryPath;
if (hr == COM.S_OK) {
return Optional.of(directoryPath);
Copy link
Contributor

@laeubi laeubi Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Optional.of(directoryPath);
return Optional.ofNullable(directoryPath);

could it be null? or maybe the case of empty should also result in a Optional.empty()....

*
* @since 3.125
*/
public Optional<String> askSelectDirectory() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Optional<String> askSelectDirectory() {
public Optional<String> openDirectory() {

What do you think about something like this? askSelect sounds a bit strange and that way probabbly if it also starts with "open" users that are used to the old method might discover the new one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think something that starts with open so that it's sorted to be adjacent in documentation or in proposals....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Maybe just openDialog()? Since the call does not open a file/directory, but a dialog (to choose one).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with openDialog() as well

@HeikoKlare HeikoKlare force-pushed the filedialog-open-with-cancel branch 4 times, most recently from 8bd21ca to eeb8cd9 Compare February 5, 2024 12:42
@HeikoKlare
Copy link
Contributor Author

HeikoKlare commented Feb 5, 2024

I have manually checked the behavior on Windows 11, Ubuntu 22.04 (Gnome 42) and MacOS Sonoma with this simply snippet (also with DirectoryDialog respectively):

System.out.println(new FileDialog(new Shell(new Display())).openDialog());

They all printed an empty Optional when cancelling the dialog and the selected path in case of a successful selection.

@HeikoKlare HeikoKlare marked this pull request as ready for review February 5, 2024 12:50
@HeikoKlare HeikoKlare force-pushed the filedialog-open-with-cancel branch 2 times, most recently from 50e5058 to 5096e6f Compare February 6, 2024 16:07
@HeikoKlare HeikoKlare force-pushed the filedialog-open-with-cancel branch 2 times, most recently from f87f799 to 7fa0257 Compare March 6, 2024 13:45
The FileDialog and DirectoryDialog implementations do currently not
provide any way to identify that the dialog was canceled. The dialogs
return a null string in case no file or folder was selected, but is not
possible to identify whether this was intended (via a cancel action) or
if there was some unexpected error while the dialog was open (such as
too long file paths on Windows).

This change introduces the `openDialog()` method as an alternative to
the existing `open()` method. This new method returns an optional
String, which is only present if a file or directory was successfully
selected. An empty Optional indicates a cancellation by the user and an
actual error is realized as an SWTException. The dialogs evaluate the
operating system's response code for the system's dialog control to
identify the dialogs result state.
@HeikoKlare HeikoKlare force-pushed the filedialog-open-with-cancel branch from 7fa0257 to e6bb8b0 Compare March 6, 2024 15:21
@HeikoKlare HeikoKlare merged commit 974992c into eclipse-platform:master Mar 7, 2024
13 checks passed
@HeikoKlare HeikoKlare deleted the filedialog-open-with-cancel branch March 7, 2024 14:35
@HeikoKlare HeikoKlare linked an issue Mar 7, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose Canceled State in FileDialog
3 participants