Skip to content

[FIX] Python script widget: prevent data loss #3529

Merged
janezd merged 5 commits intobiolab:masterfrom
markotoplak:owscript-save
Jan 18, 2019
Merged

[FIX] Python script widget: prevent data loss #3529
janezd merged 5 commits intobiolab:masterfrom
markotoplak:owscript-save

Conversation

@markotoplak
Copy link
Member

@markotoplak markotoplak commented Jan 11, 2019

Issue

Users were losing their scripts:

  1. If they added a new enrty to the library (button "+") they lost the current script.
  2. If they did not save the script into the library it got lots.

Fixes #3531

Description of changes
  1. Adding an entry to the library initializes that script with the current script text.
  2. The current script text is saved as a schema_only settings and is restored on load. I also added an option to restore the library version.
Includes
  • Code changes
  • Tests
  • Documentation

@markotoplak
Copy link
Member Author

Currently, this PR is changing a setting on every textChanged. I tried with long texts and it did not seem problematic.

Still, I think saving scriptText belongs to storeSpecificSettings, but it is not called without a context. Should I then rather add a context handler?

@codecov
Copy link

codecov bot commented Jan 11, 2019

Codecov Report

Merging #3529 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3529      +/-   ##
==========================================
+ Coverage   83.67%   83.68%   +0.01%     
==========================================
  Files         370      370              
  Lines       66174    66213      +39     
==========================================
+ Hits        55369    55412      +43     
+ Misses      10805    10801       -4

@markotoplak markotoplak changed the title [WIP] Python script widget: prevent data loss [FIX] Python script widget: prevent data loss Jan 11, 2019
@markotoplak
Copy link
Member Author

I changed the widget so that it does not waste time for copying text on every input by subclassing a SettingsHandler. The subclassed SettingsHandler runs a widget's method before packing settings. Ideas for a nicer solution?

@astaric
Copy link
Member

astaric commented Jan 14, 2019

ContextHandler calls self.settings_from_widget(widget) in pack_data, which in turn calls widget.storeSpecificSettings().

You could consider "promoting" this behaviour to SettingsHandler thus making it available to all widgets, not just those that use contexts?

All widgets have this method (https://github.com/biolab/orange3/blob/master/Orange/widgets/widget.py#L882) and it does nothing by default so it should not break anything.

@markotoplak
Copy link
Member Author

markotoplak commented Jan 14, 2019

@astaric, true, but I would do it in some other PR. Also the call to retrieveSpecificSettings should be moved...

@astaric
Copy link
Member

astaric commented Jan 15, 2019

Then I would at least use the same widget method (storeSpecificSettings) for the job.

This way, if the functionality is ever moved to SettingsHandler, a search for storeSpecificSettings will find this widget and PrepareSavingSettingsHandler can be removed at that point.

@markotoplak markotoplak force-pushed the owscript-save branch 2 times, most recently from fef20aa to ddf514d Compare January 15, 2019 13:33
Copy link
Member

@astaric astaric left a comment

Choose a reason for hiding this comment

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

Tested 1. and 2., both work as described.

When multiple scripts in the library are unsaved only the changes of the "active" one are saved in the workflow. The rest are still lost. Asking to save to library or discard when switching active script might be a solution (probably not in this PR)

The "library" allows me to deselect the current item (CMD clicking the currently selected item), and edit the script. The changes are "remembered" in the last selected item, but the changed indicator is not shown. When workflow is saved and restored, the changes are reapplied to the active script and changed indicator is shown.

@janezd janezd self-assigned this Jan 18, 2019
@janezd janezd merged commit 87696a7 into biolab:master Jan 18, 2019
@markotoplak markotoplak deleted the owscript-save branch January 30, 2019 13:30
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.

3 participants