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] settings: Do not clear schema_only settings on close_context #2691

Merged
merged 2 commits into from
Oct 23, 2017

Conversation

ales-erjavec
Copy link
Contributor

Issue

Since gh-2301 schema_only settings are destructively cleared when used in combination with a context handler. This breaks (what I would hope to be) a valid use case

class A(OWWidget):
    ...
    bar = Setting(None, schema_only=True)
    def __init__(self):
        if self.bar is None:
             self.bar = ...  # not loaded from a saved workflow

    def foo(self):
        self.closeContext()
        self.openContext(...)
        assert self.bar is not None

This state where bar is None would then be saved in a workflow.
This pattern is used in Textable add-on for issuing uuids to widgets

The same pattern is also used in OWBaseLearner widget for the learner_name but without a context handler.

I am not sure what the intent there was. Presumably the semantics of the selection restore in scatter plot, ... is such that it is only applied once when restored from a saved workflow.

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@ales-erjavec ales-erjavec force-pushed the fixes/schema-only-settings-clear branch 2 times, most recently from 3057ff6 to cbefe1b Compare October 17, 2017 16:09
@codecov-io
Copy link

codecov-io commented Oct 17, 2017

Codecov Report

Merging #2691 into master will increase coverage by <.01%.
The diff coverage is 86.11%.

@@            Coverage Diff             @@
##           master    #2691      +/-   ##
==========================================
+ Coverage   75.83%   75.83%   +<.01%     
==========================================
  Files         338      338              
  Lines       59545    59566      +21     
==========================================
+ Hits        45156    45173      +17     
- Misses      14389    14393       +4

@kernc
Copy link
Contributor

kernc commented Oct 17, 2017

Presumably the semantics of the selection restore in scatter plot, ... is such that it is only applied once when restored from a saved workflow.

What if not only scatter plot; would all widgets need a self.__did_restore_state?

@ales-erjavec
Copy link
Contributor Author

What if not only scatter plot; would all widgets need a self.__did_restore_state?

Presumably, yes.

@ales-erjavec ales-erjavec changed the title [FIX] settings: Do not clear schema_only settings on close_context [WIP][FIX] settings: Do not clear schema_only settings on close_context Oct 18, 2017
@ales-erjavec ales-erjavec force-pushed the fixes/schema-only-settings-clear branch from cbefe1b to c4e8017 Compare October 18, 2017 12:04
@ales-erjavec
Copy link
Contributor Author

A kingdom for a saveState slot.

@ales-erjavec ales-erjavec changed the title [WIP][FIX] settings: Do not clear schema_only settings on close_context [FIX] settings: Do not clear schema_only settings on close_context Oct 18, 2017
@astaric astaric added this to the 3.7 milestone Oct 20, 2017
@astaric
Copy link
Member

astaric commented Oct 23, 2017

@ales-erjavec the addition of sparse support in scatter plot created conflicts with this. Can you rebase?

* undo changes to settings from biolabgh-2301
* change selection restore in scatter plot and linear projection accordingly
@ales-erjavec ales-erjavec force-pushed the fixes/schema-only-settings-clear branch from 4bba9ce to 8767e6e Compare October 23, 2017 07:58
@ales-erjavec
Copy link
Contributor Author

Ok.

@astaric
Copy link
Member

astaric commented Oct 23, 2017

Thanks

@astaric astaric merged commit 445176c into biolab:master Oct 23, 2017
@ales-erjavec ales-erjavec deleted the fixes/schema-only-settings-clear branch November 14, 2017 08:55
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.

4 participants