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

Re-Generate legend, through apply_callback/Apply #2947

Merged
merged 6 commits into from May 2, 2014
Merged

Re-Generate legend, through apply_callback/Apply #2947

merged 6 commits into from May 2, 2014

Conversation

anykraus
Copy link
Contributor

@anykraus anykraus commented Apr 2, 2014

If there is a legend, it is re-generated when "Apply" is clicked.
See Issue #2934:
Might have side effects.
#2934
tacaswell: "However this will clobber any legend that is not the auto-magically generated one so it should probably also get a tick box to enable/disable the updating of the legend."
Tick box not included.

If there is a legend, it is re-generated when "Apply" is clicked.
See Issue #2934:
Might have side effects.
#2934
tacaswell: "However this will clobber any legend that is not the auto-magically generated one so it should probably also get a tick box to enable/disable the updating of the legend."
Tick box not included.
@tacaswell tacaswell added this to the v1.4.0 milestone Apr 2, 2014
@tacaswell
Copy link
Member

Could you add the tick box?

Clobbering people's carefully laid out legends is bad form.

@anykraus
Copy link
Contributor Author

anykraus commented Apr 3, 2014

Maybe I can. This would be the opportunity to learn GUI programming.
My schedule is kind of busy till next Friday 2014-04-11, but on that Friday I should be able to invest 2 hours into that.

Beside the check box for generally enabling/disabling legend re-generation there is something else to improve:
With my patch the re-generated legend is not draggable any more(if it was before). I've got two ideas how to solve that:
a)don't re-generate, just re-draw (this might also prevent clobbering self made legends, if this works, I don't need to learn GUI programming):
legend = axis.get_legend()
legend.draw()

b) # store
legend_is_draggable = legend._draggable is not None
...
legend = axes.legend()

restore

legend.draggable(legend_is_draggable)

I guess I can test again matplotlib/examples/pylab_examples/legend_demo3.py or something similar.

@tacaswell
Copy link
Member

@anykraus Have you had a chance to work on this at all?

anykraus added 3 commits April 22, 2014 14:19
Checkbox added that controls if an automatic legend is re-generated, or generated for the first time.
This checkbox defaults to False, as it generates a simple legend when set to True, see matplotlib.pyplot.legend(), which clobbers any carefully handcrafted legend.
Copy some properties from the old legend to the new (re-)generated legend.
If the old legend was draggable, the new one is now also.
The number of columns in the legend is passed on from to old legend to the new legend.
Fixed crash if there was no previous legend.
Fixed ncol inheritance.
@anykraus
Copy link
Contributor Author

I added a checkbox (to the "Axes" tab of the "Figure Options" window 63894f9) and included using two important properties from the old legend in the new legend (ncol and draggable in 80beedf).
Fixed up everything in 07930c0 as it was not working if there was no previous legend.

Anything else to do? (A new pull request?)

P.S.: I started copying other properties in
https://github.com/anykraus/matplotlib/blob/figureoptions-add-legend-tab/lib/matplotlib/backends/qt4_editor/figureoptions.py
But this needs polishing.
It might be worth to add a "Legend" tab beside the "Axes" and "Curves" tabs into the "Figure options" window to allow changing some of the legend's main properties.

@@ -54,6 +54,8 @@ def figure_edit(axes, parent=None):
('Min', ymin), ('Max', ymax),
('Label', axes.get_ylabel()),
('Scale', [axes.get_yscale(), 'linear', 'log'])
sep,
('(Re-)Generate automatic legend', False) #defaults to False, as it clobbers carefully hand crafted legends /2014-04-22
Copy link
Member

Choose a reason for hiding this comment

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

This file is apparently not tested for pep8 compliance, but could you wrap the long line and in-line comments should be of the form

stuff  # comment

It seems petty and dumb, but maintaining a consistent style really does help with the readablity of the code (which it turn helps out the devs who spend lots of time reading said code).

There is no need to put dates/name in-line, git keeps track of that for us.

@tacaswell
Copy link
Member

Left a few minor style comments.

No need for a new PR, github will just keep adding commits to this on (so long as you push them to the correct branch).

This will need an entry in CHANGELOG and if you think it should be advertised a few lines in whats_new.rst.

Adding a new tab does sound useful, however that should go on it's own branch (and probably should wait for 1.5).

@@ -54,6 +54,8 @@ def figure_edit(axes, parent=None):
('Min', ymin), ('Max', ymax),
('Label', axes.get_ylabel()),
('Scale', [axes.get_yscale(), 'linear', 'log'])
Copy link
Member

Choose a reason for hiding this comment

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

You are missing a , here

Ran pep8.py 1.5.6 (https://github.com/jcrocholl/pep8) over the code.
Fixed all complaints.
@anykraus
Copy link
Contributor Author

Interesting: Travis CI build fails with
"ValueError: Some exclude patterns were unnecessary as the files they pointed to either passed the PEP8 tests or do not point to a file:
*/matplotlib/backends/qt4_editor/figureoptions.py"
A) Do we remove the exclude? (see EXCLUDE_FILES in lib/matplotlib/tests/test_coding_standards.py )
B) Revert back to a non PEP8 compliant version of figureoptions.py?

@tacaswell
Copy link
Member

Definitely A.

The exclude list of there so that we did not have to pep8 the entire library in one shot, but one we get a file compliment the tests make sure it won't revert.

(edit stupid phone)

*/matplotlib/backends/qt4_editor/figureoptions.py
now passes pep8 checks.
@WeatherGod
Copy link
Member

"duke compliment"? Google doesn't return anything useful.

On Wed, Apr 23, 2014 at 7:30 AM, Thomas A Caswell
notifications@github.comwrote:

Definitely A.

The exclude list of there so that we did not have to pep8 the entire
library in one shot, but one we get a duke compliment the tests make sure
it won't revert.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2947#issuecomment-41150309
.

tacaswell added a commit that referenced this pull request May 2, 2014
Re-Generate legend, through apply_callback/Apply
@tacaswell tacaswell merged commit 0fa7771 into matplotlib:master May 2, 2014
anykraus added a commit to anykraus/matplotlib that referenced this pull request May 7, 2014
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.

None yet

4 participants