[ENH] Improve Save widget's gui

[janezd]

Issue

Fixes #3539.

OWSave widget didn't age well.

• Choosing the format in a separate combo box instead of in the save dialog? The GUI of its previous incarnation was clear to his creator (me), but I would have to explain it at workshops - not a good sign. But it made sense for as long as one set the format with "Save as". The current incarnation required setting the format in a combo and then use Save as (or Save) to set the name -- the save dialog was passed the filter chosen in the combo. Unusual and cumbersome.

• Compression options. I suppose this complication was needed because of the "Compression" checkbox, which the user would have to decide before clicking Save (and it's availability depended on the format). The widget offered three different compressions, one of which was even new to me. Gzip should suffice.

• Auto save checkbox didn't follow the usual semantics. Auto-apply buttons apply the changes in the widgets, while input data is always handled, disregarding the checkbox. In this widget, it was the opposite.

• Ugly Save and Save As buttons. The previous layout had a single line, with Save button, that included the file name, being arbitrarily long. Additional controls required a more standard layout, resulting in ugly wide buttons.

• Selection of filters depended on data sparsity.. Sparse data was handled by reducing the number of available formats in the combo. The code for this was complicated and could, I suppose, switch the file format behind the user's back. Perhaps a better way is to just show an user error message.

• Mess under the hood. Contrived GUI resulted in contrived code. (This was not a result of a single coder; every change made the widget more complex.) One notable self-inflicted injury was that the widget had to manage the base name (chosen in the save dialog - extension was thrown away!) and then two extensions (file format and compression) controlled by the checkboxes. To maintain this update_extension was called in various places. The code became dangerous to touch.

Description of changes

I decided to discard Save As, which confused the user and have "Set File Name" instead. This also sets the file format. Visibility of checkboxes for compression and type annotations depend upon the format (I generally oppose hiding controls because they change the widget size and confuse the user, but in this case it does no such harm.)

The function of the auto save checkbox is stated explicitly.

There is only a single option for compression.

I also added report and info in the status bar.

The code is now rather simple -- the widget contains a constructor, input data handler, two handlers for push buttons, a method that updates GUI and a method checking whether to enable saving.

Setting migrations are minimal (one renamed setting).

There are two commits in the PR. The first contains an interim design that mostly just shuffled the controls in the interface. The picture below does not contain the save button - it appears after the file is set (with Save As). While this looks nice, it still contains all the problems listed above and was impossible to improve on. I will squash the commits before the PR is merged.

Includes

• Code changes
• Tests
• Documentation

[ajdapretnar]

I like the smaller size of the widget. Just one question, since you are doing this. Does the separation of Save Data and Save Distances make sense? Would it be too horrible to have one widget capable of saving different data structures?

[janezd]

I usually support reducing the number of widgets. But to do this here, the widget would

• have to have two input signals. What if the user connects both? Report an error?
• Or it could have a single input signal that would accept any type of object and report an error if the user connected anything that can't be saved.
• Or it could have a signal that accepts either this or that (a union-like signal). We don't have this in Orange.
  The last case cannot be implemented at the moment and the former two allow the user to create an invalid schema, which we try to prevent.

The next complication is the format. Does the dialog for selecting the filename (and thus selecting the format) show formats for data and for distances? Or does it depend upon the input data type? So when the input type changes, the widget has to essentially reset its settings and forget the file name... And when the input type changes back, does the widget retrieve the last settings for this type? Thus it would have to have parallel settings for two input types. It sounds like trying to squeeze two widgets into one.

Finally, at the moment we implement this, somebody will want to save models, too. Oh, and network add-on should be able to extend this widget with its own formats, too.

I think that this would lead to OWFrankensteinSave with impossible (and ambiguous) GUI and code. Even just combining saving of tables and distances into a single widget is unmanageable, IMHO.

[ales-erjavec]

Or it could have a signal that accepts either this or that (a union-like signal). We don't have this in Orange.

This can be achieved by defining an ad-hoc abstract meta class for the input and registering all the accepted types.

InputType(abc.ABC):
    pass
InputType.register(Type1)
InputType.register(Type2)

This works for the inputs, but not outputs.

[janezd]

Thanks for this. But I'd still avoid having such widget unless we also solve other problems, including design.

[codecov]

Codecov Report

Merging #3545 into master will increase coverage by 0.03%.
The diff coverage is 99.38%.

@@            Coverage Diff             @@
##           master    #3545      +/-   ##
==========================================
+ Coverage   83.78%   83.82%   +0.03%     
==========================================
  Files         370      370              
  Lines       66680    66833     +153     
==========================================
+ Hits        55871    56021     +150     
- Misses      10809    10812       +3

[codecov]

Codecov Report

Merging #3545 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3545      +/-   ##
==========================================
+ Coverage   84.24%   84.29%   +0.05%     
==========================================
  Files         370      370              
  Lines       67475    67746     +271     
==========================================
+ Hits        56844    57109     +265     
- Misses      10631    10637       +6

[ajdapretnar]

I don't like the fact that the widget prompts me to set the name before saving in the form of a warning.

Could there be a line edit where one sets the name? A way to avoid opening a new window if not necessary? There could be a default name suggested, for example, which would speed up the saving process.

This is what Excel has:

It is not collapsed by default, but ours could be. Expanding it allows the user to navigate the file system and decide where to save the data.

[janezd]

We can't implement our own dialog for selecting a file, because it would have to be different for every operating system (or even every window manager?). Even Qt doesn't have it - it has its own, non-native dialog that doesn't look good. Excel can have it, because Microsoft can afford and maintain it.

If it's just a line edit -- we could, but I'd prefer the "normal way" of selecting a file in a GUI. No ordinary user can type /usr/janez/projects/foo/bar/x.tab or ~/janez/blah/blah.

[ajdapretnar]

No ordinary user can type /usr/janez/projects/foo/bar/x.tab or ~/janez/blah/blah.

No, of course not. I meant simply writing iris, which would be saved with os.path + name + extension.

Speaking of which, when I edit the file name from iris.tab to, say, rozice, the file would be saved without extension, even though .tab is selected below. I hate this very much. We surely can check if the extension exists and then add it ourselves if it doesn't.

[janezd]

Comments from discussion:

• Set File Name -> Save As... (and saves)
• Filter should include * so macOS correctly updates extensions
• Check extension set in the dialog; what works in macOS may not work the same elsewhere
• Save dialog should only offer formats that support sparse data if input is sparse
• Checkboxes should always be visible; give warning if unsupported
• Save button can be greyed-out if file name is not set

[janezd]

I no longer like the idea that the widget should show only file formats that are allowed for a particular setting. If the user selects compression or type annotations and clicks Save As..., (s)he won't see the Excel format and probably won't know it exists until (s)he, by pure chance, disable Excel.

The same arguments holds even if the filtering is based only on sparsity. I prefer offering all formats but show an error that the user should select the pickle format (which is currently the only format that supports sparse data anyway).

[janezd]

@ajdapretnar said that she usually just puts the data into this widget and saves it. Hence Save top left. And hence a small little tiny widget. (Compare with the screenshot at the top.)

File name is again in the button; it's clear and there's no better place where to put it. (I tried label. Ugly.) Save As... belongs beside this button, and the auto save checkbox should be immediately below.

The other two checkboxes are below. They trigger warnings if incompatible with the chosen format but don't prevent saving.

Sparse data triggers error "Use .pkl format for sparse data." if format does not support sparse data (only .pkl does).

Annotation check box can be removed; the user could chose "Tab-delimited with type annotations (.tab)" as a format in the save dialog. Gzip can't be handled similarly because of QTBUG-44227 with double extensions (.tab.gz) on mac Os.

[ajdapretnar]

This is probably the best solution to our problem. Once the tests pass, it is ready to merge.

[lanzagar]

A couple of issues:

• When adding the widget, it starts with an error (File name is not set) - I don't like being greeted by errors before I have a chance to do anything
• When clicking Save As it crashed - I think it was a problem with old settings, it doesn't happen now after I cleared them (but users should not get a crash in any case)
• The compression option/checkbox does not affect extensions available in the system file chooser opened by Save As. This has several downsides: can't find and click a file I want to save to, the warning/question about overwriting files is wrong, etc
• I am not really sure why we switched the position of Save / Save As buttons from bottom to top? Isn't the previous more standard and what one would expect - first you choose some parameters like compression/annotation, then you save - (western) people usually read from top to bottom and software usually has buttons that do something at the bottom.

EDIT: I managed to reproduce the settings crash:

Traceback (most recent call last):
  File "/home/lan/dev/orange3/Orange/widgets/data/owsave.py", line 107, in save_file_as
    data_name += self.filt_ext[self.filter]
KeyError: 'Tab-separated values (.tab)'

[janezd]

• The compression option/checkbox does not affect extensions available in the system file chooser opened by Save As. This has several downsides: can't find and click a file I want to save to, the warning/question about overwriting files is wrong, etc

Existing widget already had this problem: if you selected a file, then checked compression and then saved again, it could overwrite an existing file without a warning.

Apparently the only way to do this properly is to list compressed formats in the dialog (instead of having a compression checkbox) and to implement the file selection dialog for specific OSes.

• On macOS, native dialog treats "foo.csv.gz" as having an extension ".gz" and changes it to "foo.csv.gz.csv.gz" (and adds more and more of them).
• On Linux, Qt does not use native dialog but its own which also has a bug in setting double extensions(!).
• On Windows, I've no idea.

On Linux, the bug was fixable by connecting to some signals. On Mac, the save dialog offers format but doesn't show (and know about) extensions. After the user selects a file, (s)he is asked whether to overwrite an existing file. If (s)he doesn't, the file dialog reappears. Behaviour is thus similar as usual. The only downside is that the extension is assigned after selecting a file (e.g., the user selects iris.foo, which changes to iris.tab if Tab delimited format is chosen). This is similar to default Windows behaviour (adding a .txt in notepad). :)

Widget is now pretty tiny.

File selection on macOS (note the absence of extensions in the filter):

File selection on Linux (note that flies with the wrong extension are not show - that's up to Qt):

When adding the widget, it starts with an error (File name is not set) - I don't like being greeted by errors before I have a chance to do anything

This error now only shows if auto save is on.

When clicking Save As it crashed - I think it was a problem with old settings, it doesn't happen now after I cleared them (but users should not get a crash in any case)

Migrations are yet to be tested.

I am not really sure why we switched the position of Save / Save As buttons from bottom to top? Isn't the previous more standard and what one would expect - first you choose some parameters like compression/annotation, then you save - (western) people usually read from top to bottom and software usually has buttons that do something at the bottom.

It's because I wanted the auto save checkbox under the buttons. So with @lanzagar we decided to put a box around the two checkboxes above (annotations and compression). With compression gone, I gave up and put auto save above the buttons.

To do: Somebody must try it on Windows

Diff coverage won't be improved (much) because of the code that deals with platform-specific file dialogs.

[janezd]

This is not a change but a fix.

[janezd]

Migrations are in a local function because the code uses return-s to abort migration if it fails or when it's done. Not using return-s would make the code overly if-i -- this is way simpler. However, if these were returns from migrate_setting, they would complicate future migrations. Hence a local function.

[ajdapretnar]

I really like this. It works perfectly from what I have tested.
Only a small comment that does not have to be addressed here, but just as a note. The widget is quite small. When it reports an error, it cannot fit both the error and status on the same line, so it cuts the status. Perhaps we should ensure this doesn't happen globally.

[thocevar]

Works ok on Windows with the exception that selecting a double extension clears the file name. I guess we're giving up on that and hope that Windows will start handling double extensions better.