Skip to content

Check if output file exists when exporting #2298

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

andromeda-224
Copy link
Collaborator

Prerequisites

  • Reviewed the checklist

  • Reviewed feedback from the "Sonar Cloud" bot. Note that you have to wait
    for the "CI / Unit Tests") to complete first. Failed Unit tests can be
    debugged by adding the label "verbose logging" to the GitHub PR.

Description of the Change

When exporting, add a warning to the user when the file in the output textfield already exists in the following scenarios:

  1. When file path is pasted into the output
  2. Whenever the filepath in output is manually modified

Alternate Designs

Other considerations were:

  • If the warning was displayed when the user clicks on the Export button. This works fine if the user clicks Yes to overwrite, but the issue is if the user clicks No, the Export dialog has already closed (due to the set up of the framework).
  • The user could manually modify the output file in the textfield and the file exist check doesn't get validated straight away.
  • If we use the isValidPath to invalidate the output (so that the output field background goes red), this disables Export button. This means that the user cannot overwrite the file.

Why Should This Be In Core?

This will be more user friendly and apply all plugins extending AbstractGeoExportPlugin.

Benefits

This will be more user friendly and apply all plugins extending AbstractGeoExportPlugin.

Possible Drawbacks

Verification Process

Copy and paste an existing valid filepath in the output textfield when exporting to GeoPackage.
A warning notification that the output file already exist should be displayed. Close the notification.
This will allow you to continue to either change the output file or click on the Export button.

Applicable Issues

#2274

Copy link
Collaborator

@mimosa2 mimosa2 left a comment

Choose a reason for hiding this comment

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

I tested this by copying the file path. The existing filename warning popup appeared but only option available was 'Ok' which then overwritten the file. There should a question asking 'Do you want to overwrite?' with options 'Yes', 'No' and 'Cancel'.

@Delphinus8821
Copy link
Collaborator

This solution can cause the popup to appear twice if you overwrite a file from the file chooser and then hit export. A way around this could be to put a flag on the file chooser button that if that button has been pressed and the check box hasn't been edited since, then don't show the popup on export

@andromeda-224
Copy link
Collaborator Author

The solution has been updated such that a flag is set when the filechooser's overwrite dialog Yes has been clicked and so that there won't be another dialog popping up after that. If the filechooser dialog has not been clicked on to choose the file and a duplicate file is manually typed (or pasted) into the inputfield, an Overwrite dialog will pop up. In this dialog, if you choose No or Cancel Overwrite, the Export button will be disabled. If you type in a valid filename, the Export button will be enabled.

parametersCreated.addController(OUTPUT_PARAMETER_ID, (master, params, change) -> {
final String output = params.get(master.getId()).getStringValue();
final File file = new File(output);
FileParameterType.FileParameterValue fileParamValue = (FileParameterType.FileParameterValue) params.get(master.getId()).getParameterValue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

final

if (change == ParameterChange.VALUE && super.isValidPath(file) && !fileParamValue.isFileChooserSelected()) {
if (doesFileExist(new File(output))) {
String msg = String.format("The file %s already exists. Do you want to replace the existing file?", file.getName());
final NotifyDescriptor nd = new NotifyDescriptor.Confirmation(msg, "Overwrite file", NotifyDescriptor.YES_NO_CANCEL_OPTION, NotifyDescriptor.QUESTION_MESSAGE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

NO and CANCEL do the same thing.
Could just have YES and NO as the options, no need to include a CANCEL option.

Comment on lines 64 to 68
if (DialogDisplayer.getDefault().notify(nd) == NotifyDescriptor.YES_OPTION) {
return;
} else {
params.get(master.getId()).setError(String.format("The file %s already exists.", file.getName()));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block can be simplified to:

if (DialogDisplayer.getDefault().notify(nd) == NotifyDescriptor.NO_OPTION) {
    params.get(master.getId()).setError(String.format("The file %s already exists.", file.getName()));                        
}

Updated to YES/NO dialog and as per review
Copy link

sonarqubecloud bot commented Jun 2, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
17.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

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.

4 participants