Bugzilla – Full Text Bug Listing
| Summary: | FileDialog overwrites file without prompt. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Alex Ignácio da Silva <alexignaciodasilva> | ||||||||
| Component: | SWT | Assignee: | Sravan Kumar Lakkimsetti <sravankumarl> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Arun Thondapu <arunkumar.thondapu> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | arunkumar.thondapu, carolynmacleod4, eclipse, gheorghe, loskutov, markus.kell.r, remy.suen, sravankumarl | ||||||||
| Version: | 3.6.1 | Flags: | arunkumar.thondapu:
review+
|
||||||||
| Target Milestone: | 4.4 M6 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
I've seen this bug in Ubuntu 10.04. Perhaps it's not a GTK bug, as when I try the same situation using gedit the file gets saved without the extension ("file"), therefore not overwriting the already existing file ("file.txt").
Running the snippet above on Windows XP produces the expected behavior of the dialog displaying the overwrite warning.
On Windows the file names returned by both open() and getFileName() have the extension ("file.txt"). On Linux the return value of open() has the extension and the return of getFileName() has not, so a workaround could be implemented by using the directory returned by open() with the file name returned by getFileName(). On Linux the dialog will warn the user if she tries to save the file in a directory with an already existing file without the extension ("file").
I understand the situation described below but after more in depth testing, I think it is actually a GTK bug. To explain further, the GTKFileChooser dialog (which is used by FileDialog) checks whether a file with the name specified by the user already exists only by using the name that is entered by the user. As you already noticed in this case, if the name is entered as "file" and a file already exists with that name, the confirmation dialog will appear. Similarly, if you try to save a file by entering the name as "file.txt" and a file with that name exists, it will warn you. However, it fails to handle the case where the filename is entered as "file" with a "*.txt" extension filter because GTK's check only ensures that there is no file named "file" in the specified directory and no check is done for "file.txt" at all. Regarding the difference in handling you noticed between gedit and FileDialog, this is because gedit accepts the filename exactly as it is returned by GTK whereas the FileDialog code explicitly appends the extension filter to the filename if no extension is specified by the user. This is what is causing an SWT application using FileDialog to overwrite the file in this case. I'm still contemplating whether to file a bug with GTK for this issue or if it is possible to find some workaround for the same. Will update soon. Thanks! (In reply to comment #1) > I've seen this bug in Ubuntu 10.04. Perhaps it's not a GTK bug, as when I try > the same situation using gedit the file gets saved without the extension > ("file"), therefore not overwriting the already existing file ("file.txt"). > > Running the snippet above on Windows XP produces the expected behavior of the > dialog displaying the overwrite warning. > > On Windows the file names returned by both open() and getFileName() have the > extension ("file.txt"). On Linux the return value of open() has the extension > and the return of getFileName() has not, so a workaround could be implemented > by using the directory returned by open() with the file name returned by > getFileName(). On Linux the dialog will warn the user if she tries to save the > file in a directory with an already existing file without the extension > ("file"). I can confirm this too.
The bug was introduced in FileDialog.computeResultChooserDialog() method after 3.5.1.
We observe it right now on 3.7.1 in a slightly different way, but the common part is that the FileDialog now appends the file filter to the file name if the file name does not contains "." and filter is not "*" or "*.*".
For us if we set the filter to "[^.]*" (to hide dot files) the filter string will be always added to the file name.
This is the bad code added after 3.5.1 which breaks FileDialog behavior on Linux.
if (fullPath != null) {
int separatorIndex = fullPath.lastIndexOf (SEPARATOR);
fileName = fullPath.substring (separatorIndex + 1);
filterPath = fullPath.substring (0, separatorIndex);
int fileExtensionIndex = fileName.indexOf(FILE_EXTENSION_SEPARATOR);
if ((style & SWT.SAVE) != 0 && fileExtensionIndex == -1 && filterIndex != -1) {
if (filterExtensions.length > filterIndex) {
String selection = filterExtensions [filterIndex];
int length = selection.length ();
int index = selection.indexOf (EXTENSION_SEPARATOR);
if (index == -1) index = length;
String extension = selection.substring (0, index).trim ();
if (!extension.equals ("*") && !extension.equals ("*.*")) {
if (extension.startsWith ("*.")) extension = extension.substring (1);
fullPath = fullPath + extension;
}
}
}
}
return fullPath;
The fix should be changed to NOT append filter anymore.
At least the first assumption
int separatorIndex = fullPath.lastIndexOf (SEPARATOR);
fileName = fullPath.substring (separatorIndex + 1);
is plain wrong as for files without suffix the code below will now add suffixes.
It should probably do
int separatorIndex = fullPath.lastIndexOf (SEPARATOR);
if(separatorIndex <= 0)
return fullPath;
fileName = fullPath.substring (separatorIndex + 1);
Created attachment 212276 [details]
Simple patch reverting 3.6 changes
Attached the simplest possible patch which makes SAVE dialog working on Linux again.
The code in 3.6 SWT stream was changed to append filter suffixes to file names in some cases. Unfortunately there is no comment why it was done, but I suspect that this probably should fix some issues with latest? GTK releases. I may imagine that it could be that for some reason the filter suffix was removed in native code from selected files (initially containing suffix), and the 3.6 change should fix that. As I'm not aware in which GTK version this problem exists, I can't add the platform filter. Anyway, the 3.6 code is just buggy as it breaks the SAVE dialog behavior for ALL files without suffix if filter is set.
Proper patch would need:
1) Document, why the 3.6 addition of suffix from filter was needed.
2) Specify on which GTK version this addition of the suffix is required.
3) Check GTK version used and accordingly enable/disable this behavior.
4) Provide a working solution for newer GTK versions if the user didn't specified suffix for selected file.
The proposed patch based on the 3.7.2 swt version (org.eclipse.swt.gtk.linux.x86_64_3.7.2.v3740f) makes the recent 3.6 change mostly obsolete and so the code just works as before.
On my RH 5.6 Linux we have following GTK version:
$ rpm -qa | grep libgtk
libgtk-java-2.8.7-3.el5
libgtk-java-devel-2.8.7-3.el5
libgtk-java-devel-2.8.7-3.el5
libgtk-java-2.8.7-3.el5
and
uname -a
Linux socbl593 2.6.18-238.el5 #1 SMP Sun Dec 19 14:22:44 EST 2010 x86_64 x86_64 x86_64 GNU/Linux
Regards,
Andrey
P.S. just to make sure my gtk version is documented properly: rpm -qa | grep gtk2 gtk2-engines-2.8.0-3.el5 gtk2-devel-2.10.4-21.el5_7.7 gtk2-2.10.4-21.el5_7.7 gtk2-devel-2.10.4-21.el5_7.7 gtk2-engines-2.8.0-3.el5 gtk2-2.10.4-21.el5_7.7 Ping. Given patch works for us with no issues reported so far. Anybody to integrate it to 3.8? This same problem exists now for OS X: FileDialog for save, with extension list. When the typed filename has extension, the overwrite warning appears; when the extension is implicit, overwrite is not detected and a path is returned to the existing file (with extension). Do I need to file a separate report for OS X platform? Ping #2. The patch is there. It just works. All what you need is to look at it (and integrate :-)). *Anybody* to integrate it to 3.8 before it's too late for the release? Changing target milestone to 4.3 so that this will not get missed for the next round. Sravan, can you please try to reproduce this problem on your Linux setup and test the attached patch (check whether it works in all kinds of scenarios with different combinations of filenames and extension filters and also that it doesn't break any expected behavior) ? You can work on improving the patch or suggesting a different fix if needed. Thanks! This behaviour was changed as part of bug 285641. In this the code change was done to mimic the behaviour as Windows. The code change done as a part of 285641 is that after GTK returns the file name we are adding extension to it. So the behaviour we have created is 1. Open the GTK file chooser dialog 2. GTK dialog will check for the existence of file and warns if it about to overwritten and returns the new file 3. Add the extension 4. Write the contents in to the file Here if we closely see the steps you may notice that the filename on which we got overwrite warning and file name we used to write the contents are two different files. This is leading to overwrite of different file altogether. In my opinion there are two ways of dealing with this 1. After adding the extension verify whether the file exists. and modify the documentation to avoid setOverwrite(true) on the dialog so that the gtk will not check for the file name. and we do overwrite warning ourselves. 2. Reverting back the changes done in 285641. This will make the file chooser dialog behaviour across other gtk applications. I prefer the send solution as it will make the behaviour consistent across GTK applications The approach I am taking is reverting the chages done in bug 285641. The attached patch also works, but it will result in dead code. So I believe the correct approach would be reverting the changes for adding the extension as this is inconsistenent with other linux applications Playing ping-pong with bug 285641 / bug 277323 is not a sustainable solution. SWT has two kinds of consistency requirements: 1) implement platform behavior so that SWT apps feel like native apps 2) offer a portable (and stable!) API that shields client code from platform differences A common use case for the Save dialog is that there's a default file extension that should be used unless the user specifies a different extension. The user shouldn't have to enter the default extension manually. This use case is currently supported on all 3 platforms with the same application code (first filter extension is used by default). The only problem for this use case is that setOverwrite(true) doesn't seem to be implemented correctly on all platforms. If you want to support other use cases, then you should document those use cases and add APIs to implement them. Created attachment 239460 [details]
Revised Patch
Hi Markus, The current implementation sets a empty file name in the save dialog. My approach is to have default filename set in the savedialog with extension(first filter extension). This will solve the original problem of not adding the file extension to the file. The code changes are revert the changes done for bug 285641. and set a default filename with extension in the save dialog and highlight the filename portion so that filename can be edited. If the user needs a different extension he needs to type the extension explicitly. Also if the filter contains any regular expressions we do not add the extension. Thanks Sravan (In reply to Sravan Kumar Lakkimsetti from comment #14) > Created attachment 239460 [details] [diff] > Revised Patch 'if (fileName == "")' looks wrong. Shouldn't it use '"".equals(fileName)'? New code should adopt the formatting of surrounding code (here: space before method arguments list, but not after 'if (' ). isRegexExt(..) should be called isGlobExt and doc comments should also be fixed. The OS.gtk_file_filter_add_pattern(..) function takes glob patterns, but not regular expressions. (In reply to Markus Keller from comment #16) > (In reply to Sravan Kumar Lakkimsetti from comment #14) > > Created attachment 239460 [details] [diff] > > Revised Patch > > 'if (fileName == "")' looks wrong. Shouldn't it use '"".equals(fileName)'? > > New code should adopt the formatting of surrounding code (here: space before > method arguments list, but not after 'if (' ). > > isRegexExt(..) should be called isGlobExt and doc comments should also be > fixed. The OS.gtk_file_filter_add_pattern(..) function takes glob patterns, > but not regular expressions. Hi Markus, Based on your comments I made changes and uploaded to gerrit https://git.eclipse.org/r/21306 Please review the changes and let me know your comments Thanks Sravan Created attachment 239653 [details]
Updated Patch based on the review comments
Passing the review ball over to Arun. I mainly jumped in to prevent a fix that just reopens bug 285641, and because the term "regular expressions" in this context looked suspicious. GtkFileChooserDialog behavior seems to have changed in GTK 3 to append the default filter extension to the file name by itself, which means FileDialog may not need to append the extension explicitly. However, I'm not sure if this still solves the overwrite problem completely. Can you please test the patch with GTK 3 and update? Thanks! The bug has been committed in the following commit http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b647d62c5bdcee9c1d9104cd6245d80b8fca31e7 |
Build Identifier: 20100917-0705 When using the FileDialog to save a file with a name without extension ("file"), and the dialog extension filter is set to some extension ("Text Files"), the file will be saved with that extension ("file.txt"), and if a file with the same name already exists in the specified directory it will be overwritten without prompt, even if setOverwrite(true) was called. Reproducible: Always Steps to Reproduce: - run the snippet below, - hit the save button and navigate to a directory with an already existing file named "file.txt" and press OK, - the dialog won't warn the user about the existing file. public class Test { public static void main(String[] args) { Display display = new Display(); final Shell shell = new Shell(display); shell.setLayout(new GridLayout(1, false)); Button button = new Button(shell, SWT.PUSH); button.setText("Save"); button.addSelectionListener(new SelectionAdapter() { @Override public void widgetSelected(SelectionEvent e) { FileDialog dialog = new FileDialog(shell, SWT.SAVE); dialog.setFilterExtensions(new String[] {"*.txt"}); dialog.setFilterNames(new String[] {"Text Files"}); dialog.setFilterIndex(0); dialog.setText("Save"); dialog.setFileName("file"); dialog.setOverwrite(true); String filePath = dialog.open(); if((filePath != null) && filePath.endsWith("file.txt")) { System.out.println(dialog.getFileName()); MessageBox messageBox = new MessageBox(shell, SWT.ICON_ERROR | SWT.OK); messageBox.setText("Oops"); messageBox.setMessage("File was overwritten!"); messageBox.open(); } } }); shell.setSize(150, 100); shell.open(); while(!shell.isDisposed()) { if(!display.readAndDispatch()) display.sleep(); } display.dispose(); } }