Skip to content

[WIP] (store|retrieve)SpecificSettings called by non-context settings#3541

Closed
markotoplak wants to merge 4 commits intobiolab:masterfrom
markotoplak:storeSpecificSettings
Closed

[WIP] (store|retrieve)SpecificSettings called by non-context settings#3541
markotoplak wants to merge 4 commits intobiolab:masterfrom
markotoplak:storeSpecificSettings

Conversation

@markotoplak
Copy link
Member

Issue

storeSpecificSettings was only called with a ContextHandler. Before, if was called after packing normal settings and before packing context settings.

In #3529 I implemented a special SettingsHandler because I needed to prepare settings for saving just before saving them.

Description of changes

storeSpecificSettings is not called in SettingsHandler

TODO
  • test if they are really called before packing
  • retrieveSpecificSettings
Includes
  • Code changes
  • Tests
  • Documentation

@ales-erjavec
Copy link
Contributor

Put Color widget on canvas, delete it without any input connections:

Traceback (most recent call last):
  File "/Users/aleserjavec/workspace/orange3/Orange/canvas/scheme/widgetsscheme.py", line 795, in eventFilter
    widget.saveSettings()
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/widget.py", line 899, in saveSettings
    self.settingsHandler.update_defaults(self)
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/settings.py", line 684, in update_defaults
    widget.storeSpecificSettings()
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/data/owcolor.py", line 372, in storeSpecificSettings
    for var in self.disc_colors]
AttributeError: 'NoneType' object has no attribute 'disc_data'

@codecov
Copy link

codecov bot commented Jan 18, 2019

Codecov Report

Merging #3541 into master will decrease coverage by 0.05%.
The diff coverage is 93.61%.

@@            Coverage Diff             @@
##           master    #3541      +/-   ##
==========================================
- Coverage   83.97%   83.92%   -0.06%     
==========================================
  Files         370      370              
  Lines       66941    66887      -54     
==========================================
- Hits        56215    56132      -83     
- Misses      10726    10755      +29

@markotoplak
Copy link
Member Author

@ales-erjavec, thank you. So the problem is storeSpecificSettings wes only called before if the context was valid.

Therefore I needed to add checks for context validity in OWFile and OWColor. So if we go forward with this we risk things breaking. I quickly searched and no add-on on my disk is using this methods.

Would it be nicer if we deprecated storeSpecificSettings and used a signal "aboutToSaveData" instead?

@markotoplak markotoplak force-pushed the storeSpecificSettings branch from bf6c954 to 6ed52bd Compare January 25, 2019 13:59
@markotoplak
Copy link
Member Author

I give up. A different attempt, with a signal, is in #3567.

@markotoplak markotoplak closed this Feb 1, 2019
@markotoplak markotoplak deleted the storeSpecificSettings branch February 5, 2020 08:44
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.

2 participants