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

[ENH] Improve Save widget's gui #3545

Merged
merged 12 commits into from
Mar 1, 2019
Merged

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Jan 21, 2019

Issue

Fixes #3539.

OWSave widget didn't age well.

screenshot 2019-01-21 at 10 26 23

  • 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

screenshot 2019-01-21 at 10 24 56

screenshot 2019-01-21 at 10 24 42

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.

screenshot 2019-01-21 at 10 30 28

Includes
  • Code changes
  • Tests
  • Documentation

@janezd janezd changed the title [WIP] Improve the Save widget's gui [WIP] Improve Save widget's gui Jan 21, 2019
@ajdapretnar
Copy link
Contributor

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
Copy link
Contributor Author

janezd commented Jan 21, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

janezd commented Jan 22, 2019

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

@janezd janezd force-pushed the improve-owsave-gui branch 2 times, most recently from e0af6c6 to 0e3e2e6 Compare January 23, 2019 21:08
@codecov
Copy link

codecov bot commented Jan 23, 2019

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
Copy link

codecov bot commented Jan 23, 2019

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

@janezd janezd changed the title [WIP] Improve Save widget's gui [ENH] Improve Save widget's gui Jan 23, 2019
@janezd janezd force-pushed the improve-owsave-gui branch 2 times, most recently from 894866a to f0a34b1 Compare January 24, 2019 08:47
@ajdapretnar
Copy link
Contributor

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:
screen shot 2019-01-25 at 10 57 13

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
Copy link
Contributor Author

janezd commented Jan 25, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

janezd commented Jan 25, 2019

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 janezd changed the title [ENH] Improve Save widget's gui [WIP] [ENH] Improve Save widget's gui Jan 25, 2019
@janezd
Copy link
Contributor Author

janezd commented Jan 31, 2019

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 janezd force-pushed the improve-owsave-gui branch from f0a34b1 to e3d5e76 Compare January 31, 2019 22:40
@janezd
Copy link
Contributor Author

janezd commented Jan 31, 2019

@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.)

screenshot 2019-01-31 at 23 25 35

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
Copy link
Contributor

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

@janezd janezd force-pushed the improve-owsave-gui branch from e3d5e76 to 9daee4c Compare February 1, 2019 14:05
@janezd janezd changed the title [WIP] [ENH] Improve Save widget's gui [ENH] Improve Save widget's gui Feb 1, 2019
@janezd janezd assigned VesnaT and unassigned ajdapretnar Feb 14, 2019
@lanzagar
Copy link
Contributor

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 janezd changed the title [ENH] Improve Save widget's gui [WIP] [ENH] Improve Save widget's gui Feb 20, 2019
@janezd janezd force-pushed the improve-owsave-gui branch from 1aae063 to c5c73d5 Compare February 24, 2019 21:52
@janezd
Copy link
Contributor Author

janezd commented Feb 24, 2019

  • 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.

screenshot 2019-02-24 at 22 52 58

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

screenshot 2019-02-24 at 22 53 11

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

screenshot from 2019-02-24 22-48-23

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 janezd changed the title [WIP] [ENH] Improve Save widget's gui [RFC] [ENH] Improve Save widget's gui Feb 24, 2019
@janezd janezd force-pushed the improve-owsave-gui branch 2 times, most recently from 34908cb to 07a9813 Compare February 26, 2019 20:59
@@ -987,7 +987,7 @@ class ExcelReader(FileFormat):
"""Reader for excel files"""
EXTENSIONS = ('.xlsx',)
DESCRIPTION = 'Microsoft Excel spreadsheet'
SUPPORT_COMPRESSED = True
SUPPORT_COMPRESSED = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a change but a fix.

return

if version < 2:
migrate_to_version_2()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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.
screen shot 2019-02-28 at 11 50 04

@janezd janezd force-pushed the improve-owsave-gui branch 3 times, most recently from d2d0ef5 to c83cb1b Compare February 28, 2019 22:25
@janezd janezd changed the title [RFC] [ENH] Improve Save widget's gui [ENH] Improve Save widget's gui Feb 28, 2019
@janezd janezd force-pushed the improve-owsave-gui branch from 64cc2b9 to 1d0cbce Compare March 1, 2019 12:21
@thocevar
Copy link
Contributor

thocevar commented Mar 1, 2019

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.

@janezd janezd force-pushed the improve-owsave-gui branch from 1d0cbce to aba988f Compare March 1, 2019 12:47
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.

6 participants