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 #1005

Closed

Conversation

HeikoKlare
Copy link
Contributor

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

This change introduces a wasCanceled() method to the FileDialog implementations for all three operating systems. The dialogs evaluate the operating system's response code for the system's dialog control and store it to be retrieved by the added method.

A "better" solution would be to let the open() method somehow return a cancel operation as a its result, but such a solution would require an API change, while the proposed one only adds a method and thus keeps the API stable.

I have already tested the extension on all three operating system by both canceling and properly selecting a file in a dialog via the following simple snippet:

public static void main(String[] args) {
	FileDialog dialog = new FileDialog(new Shell(new Display()));
	dialog.open();
	System.out.println(dialog.wasCanceled());
}

Copy link
Contributor

github-actions bot commented Jan 25, 2024

Test Results

   299 files  ±0     299 suites  ±0   6m 9s ⏱️ -12s
 4 100 tests ±0   4 092 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 212 runs  ±0  12 137 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit 3857a6b. ± Comparison against base commit 4aefc7a.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare force-pushed the filedialog-cancelled branch 2 times, most recently from b192f9e to e61c959 Compare January 26, 2024 10:03
@HeikoKlare HeikoKlare marked this pull request as ready for review January 26, 2024 10:29
@HeikoKlare HeikoKlare force-pushed the filedialog-cancelled branch 3 times, most recently from bb3a30a to 6aff4a7 Compare January 30, 2024 17:28
The FileDialog implementations do currently not provide any way to
identify that the dialog was canceled. The dialogs return a null string
in case no file was selected, but is not possible to identify whether
this was intended (via a cancel action) or if there was some unexpected
error (such as too long file paths on Windows).

This change introduces a `wasCanceled()` method to the FileDialog
implementations for all three operating systems. The dialogs evaluate
the operating system's response code for the system's dialog control and
store it to be retrieved by the added method.
@HeikoKlare HeikoKlare force-pushed the filedialog-cancelled branch from 6aff4a7 to 3857a6b Compare February 1, 2024 15:47
@laeubi
Copy link
Contributor

laeubi commented Feb 1, 2024

@HeikoKlare just a though but if there is an ERROR I would expect it to be throwing an exception instead of silently ignore, it seems not so useful to have check for cancel just to tell the user that "something" went wrong....

@HeikoKlare
Copy link
Contributor Author

Maybe my description is a bit misleading: the wasCanceled() method returns true if, and only if, the user pressed the "Cancel" button. In any other error case (and also in case the user pressed "Cancel"), the open() method returns null.
Unfortunately, the latter is existing behavior specified in the API (documentation for open()). I agree that throwing an exception in case of an actual error would make sense, but implementing that would break the existing API. This is why I propose to add this method to be able to distinguish error from cancel event at all.
Did I understand you concern correctly, @laeubi?

@laeubi
Copy link
Contributor

laeubi commented Feb 1, 2024

At least the GTK impl also throws error NO_MORE_HANDLES so it seems not completely true that it always returns null on error.

But even then, instead of a new wasCanceled() method I would rather like a new method that return an OpenResult that has:

  • file
  • canceled
  • error

or as an alternative have a String open(OpenStatus status), where OpenStatus is a DTO with:

  • canceled
  • error

so one can get more information if open failed versus canceled.

Also note that there is also a DirectoryDialog :-)

@HeikoKlare
Copy link
Contributor Author

At least the GTK impl also throws error NO_MORE_HANDLES so it seems not completely true that it always returns null on error.

That's right. So we have to distinguish two kinds of errors here:

  1. There can be errors in SWT code around the dialog instantiation (invalid thread access, no more handles etc.). They will throw an SWTException, as documented at the method (even though the list of error types seems to be incomplete).
  2. There can be an error response code from the OS, encapsulating that something went wrong while the dialog was open (and all event handling was completely given to the OS for that time). I would expect this to usually be errors concerning the dialog content's semantics, such as invalid paths (e.g., too long paths on Windows), but I have to admit that I am not completely sure about the kinds of errors that can occur here.

But even then, instead of a new wasCanceled() method I would rather like a new method that return an OpenResult

That's a really good idea and definitely far better than the proposal in this PR. I will provide a PR with a proposal to discuss how that should like.

Also note that there is also a DirectoryDialog :-)

Thanks! I missed that (because our product only requires cancel state identification for the FileDialog). I will address that in the PR proposing an alternative solution (as metnioned above).

@HeikoKlare HeikoKlare marked this pull request as draft February 2, 2024 07:02
@HeikoKlare HeikoKlare deleted the filedialog-cancelled branch November 3, 2024 13:40
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.

2 participants