Merge lp://qastaging/~victor-mireyev/simple-scan/bug-719741 into lp://qastaging/~simple-scan-team/simple-scan/trunk

Proposed by Victor Mireyev
Status: Superseded
Proposed branch: lp://qastaging/~victor-mireyev/simple-scan/bug-719741
Merge into: lp://qastaging/~simple-scan-team/simple-scan/trunk
Diff against target: 78 lines (+28/-11)
1 file modified
src/ui.vala (+28/-11)
To merge this branch: bzr merge lp://qastaging/~victor-mireyev/simple-scan/bug-719741
Reviewer Review Type Date Requested Status
Robert Ancell Needs Fixing
Review via email: mp+166375@code.qastaging.launchpad.net

This proposal has been superseded by a proposal from 2013-06-04.

Description of the change

Fix LP#719741. Save dialog should force file name extensions

To post a comment you must log in.
Revision history for this message
Robert Ancell (robert-ancell) wrote :

I don't like how this silently changes the filename as entered - as a user I type in "Blah" but the actual file is "Blah.pdf" etc without any indication this has occurred. I'd prefer a dialog as the GIMP does that suggests the name is changed but allows the user to override this if necessary.

A stray change - not required for the patch but confusing to have it here:
+ extension = default_file_name.substring (index);

Should use C style comments:
+ // Check if file name doesn't have extension

It is confusing calling a callback as a way of updating the extension - you should split out the function from on_file_type_changed and use it directly.
+ on_file_type_changed (selection);

review: Needs Fixing
Revision history for this message
Robert Ancell (robert-ancell) wrote :

Note I tried this in the GIMP (2.8.4) and it was actually broken! The dialog said I could choose the file type to ignore the warning but it doesn't work.

Revision history for this message
Victor Mireyev (greymouse2005) wrote :

Hi Robert! Thank you for review.

1) Please consider an example:
victor@victor-desktop:~/test$ cat hello.vala

class HelloSubstring {

    public static int main(string[] args) {
        string a = "image.png";
        int i = a.last_index_of_char('.');
        stdout.printf("Substring: %s\n", a.substring(i));
        stdout.printf("Slice: %s\n", a.slice(0, i));
        return 0;
    }
}

victor@victor-desktop:~/test$ valac hello.vala
victor@victor-desktop:~/test$ ./hello
Substring: .png
Slice: image

The slice method in this case extracts filename. That's why by default there are no highlighted rows in file types list as it should be. Contrary, the substring method extracts an extension. So I think it's required for the patch.

2) I have tried "Save as" and "Export as" commands in the GIMP. In the first case, it appends extension without any warning. In the second case it displays warning and leaves file dialog on screen if user hasn't selected any filetype but deleted an extension. But if user has already selected filetype and then deleted an extension, it doesn't display any warning at all! Instead it shows options dialog for the selected filetype and appends an extension.

Do you propose to simply display a warning and leave file dialog on screen if user deleted an extension?

3) I agree with the rest of your comments and working on fixing issues you've shown.

Revision history for this message
Michael Nagel (nailor) wrote :

personally i have no problem with adding the extension to the entered filename. on the contrary, the longer i think about it, the more i am in favor.

while extensions in linux are not as important as extensions in windows, extensionless files suck in every graphical userface i know and use. on the command line they are acceptable sometimes (but e.g. github encourages to name your file README.md). extensionless "files" might be ok in interfaces that do not expose actual files and where data is only ever viewed with the original application (think iPhone apps, Google Drive Documents/Spreadsheets) but never with a generic file manager (think explorer, nautilus) .

so if scanned/saved images were exclusive to simple scan, we might consider dropping the extension and never look back. but as long as the save functionality exports data to the shared filesystem where it is viewed/accessed by different applications, it is just selfish and bad attitude to go extensionless.
as long as there is no doubt about the user's intention, just do it even if it is not expressed 100% formally correct. and currently there is no reasonable doubt that the user does not want to create an extensionless file. this could change with time, but in the meantime simple scan should behave simple, friendly and do nothing unexpected and not display unnecessary warnings that only cause users to ignore warnings.

also, a different point of view:

the filename in the filesystem by definition consists of the human-readable-part entered in the text field plus the abbreviation of the filetype selected in the dropdown list.

so if you type "scan1" and select portable data format, you get "scan1.pdf".

simple scan does not add something to the name, you just enter the name in two fields.

on the contrary, sometimes simple scan *REMOVES* something from the name: if what you specify to be the human-readable-part happens to end with the abbreviation of the filetype selected, the abbrevation is skipped. if you type "scan1.pdf" and select portable data format, you do not end up with "scan1.pdf.pdf" because that would be just silly. that is what everyone wants and expects. to override, you can always put "scan1.pdf.pdf" in the box -- no third ".pdf" will be added...

617. By Victor Mireyev

Refactor file extension replacing.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches