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

[FIX] Saving of multiple selections in ScatterPlot #2598

Merged
merged 2 commits into from
Sep 21, 2017

Conversation

markotoplak
Copy link
Member

@markotoplak markotoplak commented Sep 19, 2017

Issue

Selection saving did not work.

Description of changes

Selection saving needed only a small fix. To also save different selection groups I changed the format of saved selections.

Includes
  • Code changes
  • Tests
  • Documentation

@markotoplak
Copy link
Member Author

Potential problem: the save files are incompatible with older versions. As far as I tested there is no mechanism that would notify users that they are trying to open a file that is too new for their Orange installation.

@markotoplak
Copy link
Member Author

markotoplak commented Sep 19, 2017

How to properly test saving of schema_only settings? test_points_selection in test_owscatterplot only tests loading of settings - and that is why we did not catch this earlier.

@@ -518,6 +522,13 @@ def onDeleteWidget(self):
self.graph.plot_widget.getViewBox().deleteLater()
self.graph.plot_widget.clear()

@classmethod
def migrate_settings(cls, settings, version):
if "selection" in settings \
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer

if version < 2:

@astaric
Copy link
Member

astaric commented Sep 19, 2017

For testing saving of settings, you can use something like this:
https://github.com/biolab/orange3/blob/master/Orange/widgets/visualize/tests/test_owscatterplot.py#L256

Current widget settings can be packed

settings = self.widget.settingsHandler.pack_data(self.widget)

and restored to a new setting instance

w = self.create_widget(OWScatterPlot, stored_settings=settings)

The new widget gets the settings if would get if the first widget was saved to a file.

@astaric
Copy link
Member

astaric commented Sep 19, 2017

As for the compatibility with older version, you could use a new setting name. This way, when opening a workflow with an old version of Orange, the selection would be lost, but there would be no crash.

@codecov-io
Copy link

codecov-io commented Sep 19, 2017

Codecov Report

Merging #2598 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2598      +/-   ##
==========================================
+ Coverage   75.08%   75.08%   +<.01%     
==========================================
  Files         327      327              
  Lines       57725    57741      +16     
==========================================
+ Hits        43341    43356      +15     
- Misses      14384    14385       +1

Rename setting selection into selection_group so that
older version would not crash with new schemas.
@markotoplak
Copy link
Member Author

I added saving tests to the first commit and also renamed the setting.

@@ -518,6 +522,11 @@ def onDeleteWidget(self):
self.graph.plot_widget.getViewBox().deleteLater()
self.graph.plot_widget.clear()

@classmethod
def migrate_settings(cls, settings, version):
if version < 2 and "selection" in settings and settings["selection"]:
Copy link
Member

Choose a reason for hiding this comment

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

if version < 2 and settings.get(selection):

@astaric astaric changed the title Scatter plot selection saving fix [FIX] Saving of multiple selections in ScatterPlot Sep 21, 2017
@astaric astaric merged commit 3d954ea into biolab:master Sep 21, 2017
@markotoplak markotoplak deleted the scatterplot_save branch October 5, 2017 12:45
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