Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Simplify and improve Qt borders/spacing tool.#8683
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Replaced sliders in Qt borders/spacing tool by spinboxes, which 1. should be easier to set to an exact value, and 2. do not continuously trigger redraws unless the user presses enter or uses the arrows to step the values (the redraws can be quite slow when working with a complex plot).The spinbox step size of 0.005 was chosen for consistency with theearlier choice of 5/1000.Greatly simplified the implementation. Attributes on the SubplotToolQtinstance are not kept because it is impossible to keepback-compatibility (the sliders simply don't exist anymore). Newattributes are all private; only `.default` (which has the same meaning)is kept as is.Tested with PyQt 4.12, PySide 1.2.4, PyQt 5.8.
efiring left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall this looks like a good change, but I haven't tried running it yet.
| self.defaults= { | ||
| attr:getattr(self._figure.subplotpars,attr) | ||
| forattrin ["left","bottom","right","top","wspace","hspace"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest making this dictionary generation a method of Figure.SubplotParams, so the line would become, e.g.,
self.defaults=self._figure.subplotpars.dict()
| forattrinself.defaults: | ||
| widget=self._widgets[attr] | ||
| widget.blockSignals(True) | ||
| widget.setValue(getattr(self._figure.subplotpars,attr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then I would probably use the new dict() method to make a dictionary after calling tight_layout, and use that dictionary to replace the getattr calls. This is a minor matter of taste, and I wouldn't insist on it, but to me it would be more pythonic. Regardless, having something like the dict() method available would be generally useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wouldvars(self._figure.subplotpars)[attr] be good enough for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've never usedvars with an argument--I didn't know it worked that way. Now that I do, I would say that using it is indeed more readable than using the call togetattr, which I find rather ugly. Or one could use the equivalentself._figure.subplotpars.__dict__[attr]. Or SubplotParams could be turned into a specialized Bunch, so it could be indexed directly.
| self._setSliderPositions() | ||
| self.targetfig.canvas.draw_idle() | ||
| def_reset(self): | ||
| forattrinself.defaults: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self.defaults keeps being used simply as a source of the list of parameters; for the sake of readability, I might replace those uses with a genuine list, e.g.self.params. Yes, it would add one more line of code to the module.
efiring commentedMay 29, 2017
It is outside the present scope, but you might consider a related functional enhancement. I often use the tool to tweak the parameters, and then I transfer the numbers to code in a script. This would be easier if there were a button that would pop up a window with a text string defining a dictionary that could be copied and pasted directly into the script under development. It could be just the dictionary, or it could be closer to a full line of code, e.g. fig.subplots_adjust(left=0.14,right=0.99) It would be simplest to include all of the parameters, even if only some of them are changed. |
efiring commentedMay 29, 2017
One more idea related to the above: the string generation could be via a |
anntzer commentedMay 29, 2017
Included all your suggested changes (the textbox is readonly). Not inheriting from the common matplotlib.widgets.SubplotTool anymore as we don't implement any of that API anyways...
|
efiring left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for adding the export. I have just a few more style tweaks, and I still haven't tried running it.
| # the redraw callbacks. | ||
| self._reset() | ||
| forattrinself._defaults: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self._defaults -> self._attrs
| def_reset(self): | ||
| forattrinself._defaults: | ||
| self._widgets[attr].setValue(self._defaults[attr]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
forkey,valueinself._defaults.items():self._widgets[key].setValue(value)
| right.addStretch(1) | ||
| self._widgets["Export values"]=widget= \ | ||
| QtWidgets.QPushButton("Export values") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We try to avoid backslash continuations, and avoiding this one is easy:
widget=QtWidgets.QPushButton("Export values")self._widgets["Export values"]=widget
anntzer commentedMay 29, 2017
all covered |
efiring left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've tested it, and it works for me. Nice!
tacaswell commentedMay 31, 2017
Sweet! |
Replaced sliders in Qt borders/spacing tool by spinboxes, which
or uses the arrows to step the values (the redraws can be quite
slow when working with a complex plot).
The spinbox step size of 0.005 was chosen for consistency with the
earlier choice of 5/1000.
Greatly simplified the implementation. Attributes on the SubplotToolQt
instance are not kept because it is impossible to keep
back-compatibility (the sliders simply don't exist anymore). New
attributes are all private; only
.default(which has the same meaning)is kept as is.
Tested with PyQt 4.12, PySide 1.2.4, PyQt 5.8.
The new layout:
