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

Customizable plot: no need for QApplication at import #4941

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

markotoplak
Copy link
Member

Issue

Fixes #4934. To set Updater there had to be an active QApplication when Updater was imported, because default font settings were set as class attributes. This led to some segfaults in tests, which also created a QApplication instance.

Description of changes

Default font properties were moved to the __init__ of CommonParameterSetter. Also, parameter setters are now encapsulated by corresponding graphs and not mixed into them.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #4941 into master will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #4941      +/-   ##
==========================================
- Coverage   84.24%   84.22%   -0.03%     
==========================================
  Files         283      278       -5     
  Lines       57873    57043     -830     
==========================================
- Hits        48756    48045     -711     
+ Misses       9117     8998     -119     

@markotoplak markotoplak force-pushed the refactor_visual_settings branch 4 times, most recently from 47a98f4 to f276f78 Compare August 13, 2020 11:26
To set Updater there had to be an active QApplication when Updater was
imported, because default font settings were set as class attributes.
This led to some segfaults in tests, which also created a QApplication
instance. Default font properties were moved to the __init__ of
CommonParameterSetter. Also, parameter setters are now encapsulated by
corresponding graphs and not mixed into them.
@markotoplak markotoplak force-pushed the refactor_visual_settings branch from f276f78 to 8d587bd Compare August 13, 2020 12:08
@markotoplak markotoplak changed the title [WIP] Customizable plot: no need for QApplication at import Customizable plot: no need for QApplication at import Aug 13, 2020
Copy link
Contributor

@VesnaT VesnaT left a comment

Choose a reason for hiding this comment

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

LGTM, only appveyor is complaining.

Orange/widgets/visualize/utils/customizableplot.py Outdated Show resolved Hide resolved
@@ -273,6 +231,8 @@ def update_axis(axis, **settings):
}
}

self.initial_settings = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use

initial_settings: Dict[str, Dict[str, SettingsType]] = NotImplemented

if you want to keep the old functionality, but I'm fine with any solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@markotoplak
Copy link
Member Author

I do not think that Appveyor complaints are connected with this PR. It also crashes similarly in unrelated PRs, see for example https://ci.appveyor.com/project/biolab/orange3-75xll/builds/34706479.

@janezd
Copy link
Contributor

janezd commented Aug 28, 2020

Appveyor is no more.
@VesnaT, merge?

@markotoplak markotoplak merged commit 74a1154 into biolab:master Sep 2, 2020
@markotoplak markotoplak deleted the refactor_visual_settings branch September 28, 2020 07:57
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.

Tests segfault on PyQt 5.12-13 due to wrong QApplication initializations
3 participants