Legend background and foreground colors can be modified #2146

specmicp · 2020-05-21T09:50:49Z

Resolves #2145

Change in visual properties of the legend do not require to get the handles anymore
Background and foreground can be changed
Defaults for these properties are taken from the settings
The legend is also set to be draggable

- Change in visual properties of the legend do not require to get the handles anymore - Background and foreground can be changed - Defaults for these properties are taken from the settings - The legend is also set to be draggable

specmicp · 2020-05-21T10:00:29Z

It mostly works and is reasonable, except for the edge of the frame.

There are several options:

The edge color is the foreground color
No edge
The edge color can be freely modified by the user
inherit from the axes

I would tend to prefer 4, but it is not really set in the settings either. Maybe it should ?

The default is pretty good on black on white, but not that great for any other... It requires a bit of playing around

codecov · 2020-05-21T10:02:36Z

Codecov Report

Merging #2146 into master will decrease coverage by 0.02%.
The diff coverage is 83.75%.

@@            Coverage Diff             @@
##           master    #2146      +/-   ##
==========================================
- Coverage   87.87%   87.85%   -0.03%     
==========================================
  Files         246      246              
  Lines       22596    22659      +63     
==========================================
+ Hits        19857    19906      +49     
- Misses       2739     2753      +14

Impacted Files	Coverage Δ
glue/viewers/matplotlib/state.py	`90.18% <79.54%> (-0.66%)`	⬇️
glue/viewers/matplotlib/viewer.py	`93.92% <85.71%> (-1.17%)`	⬇️
glue/viewers/histogram/qt/options_widget.py	`87.09% <100.00%> (ø)`
glue/viewers/image/qt/options_widget.py	`84.00% <100.00%> (ø)`
glue/viewers/profile/qt/options_widget.py	`100.00% <100.00%> (ø)`
glue/viewers/scatter/qt/options_widget.py	`89.74% <100.00%> (ø)`
glue/core/data.py	`90.22% <0.00%> (-1.16%)`	⬇️
glue/viewers/matplotlib/qt/widget.py	`81.81% <0.00%> (+3.40%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a746d6f...836be79. Read the comment docs.

specmicp · 2020-05-21T10:03:04Z

In this commit, I also set the legend to be draggable so it can be drag around so the user can do whatever he wants. That's quite useful (at least for me), but it might mess up with other redraw... @astrofrog what do you think ?

astrofrog · 2020-05-21T10:15:26Z

@specmicp - thanks for fixing this! Just to make sure I understand, what is the difference between options 1 and 4? The axes frame color is set to the foreground color anyway, right?

astrofrog · 2020-05-21T10:18:26Z

The dragging is very nice by the way! I guess at the end of the day it depends if it causes all drawing-related tests to fail, but so far from playing around with it it works nicely. If it's a problem for redraws, you could allow location to be set to 'draggable' or something like that, so that it's opt-in?

astrofrog · 2020-05-21T10:19:48Z

One small issue - I think maybe we should call it 'box' color not 'frame' color, otherwise it might be confusing:

astrofrog · 2020-05-21T10:21:28Z

Thinking about the actual frame color a bit more, I think maybe having it be the same as the text (option 1) for now might make sense, and in future we could consider having separate global options for the text and frame colors for both axes and legend? (I'm not convinced this is needed)

However, it might be nice to have a toggle for turning the frame on/off on the legend?

astrofrog · 2020-05-21T10:23:00Z

Sorry for all the comments, but I'm also wondering whether as we add more of these properties for the legend it might be worth having a dedicated State class for the legend, so that then options would be set with viewer.state.legend.frame_color instead of viewer.state.legend_frame_color. It might be tidier to do this - what do you think? (could be done as a separate PR though!)

Add option for draggable legend

specmicp · 2020-05-21T13:04:38Z

@ astrofrog

Just to make sure I understand, what is the difference between options 1 and 4? The axes frame color is set to the foreground color anyway, right?
One small issue - I think maybe we should call it 'box' color not 'frame' color, otherwise it might be confusing:

frame is the property name for the box in the matplotlib API, that's why I went with that name, but I can see now how it can be confusing.
I changed it in the glue interface, not sure yet if I should change it in the code

I also added the option to draw or not the edge, and set up the edge color to the font color. We can play with it a bit before deciding. I agree that it does not make sense to allow full customization of the legend, because it's going to clutter the options for sure.

specmicp · 2020-05-21T13:05:59Z

The dragging is very nice by the way! I guess at the end of the day it depends if it causes all drawing-related tests to fail, but so far from playing around with it it works nicely. If it's a problem for redraws, you could allow location to be set to 'draggable' or something like that, so that it's opt-in?

I saw some strange things when resizing the plot, so I added the drag-behavior as an option. It's also clearer that it's a feature also !

specmicp · 2020-05-21T13:08:12Z

Sorry for all the comments, but I'm also wondering whether as we add more of these properties for the legend it might be worth having a dedicated State class for the legend, so that then options would be set with viewer.state.legend.frame_color instead of viewer.state.legend_frame_color. It might be tidier to do this - what do you think? (could be done as a separate PR though!)

I though of the same idea, so I think it's a good thing indeed.

Solve's legend inconsistency between qt interface and glue interface

astrofrog

Looks great! 👍 Just a couple of minor comments below.

astrofrog · 2020-05-25T09:47:57Z

glue/viewers/matplotlib/state.py

+
+    visible = DeferredDrawCallbackProperty(False, docstring="Whether to show the legend")
+
+    loc_and_drag = DeferredDrawSelectionCallbackProperty(0, docstring="The location of the legend in the axis")


I think this would be clearer as just location as it was before - then the option can be draggable when it is draggable.

yes,

I changed back that name.
To have the argument to pass to matplotlib API, the property 'mpl_location' need to be used now

astrofrog · 2020-05-25T09:48:10Z

glue/viewers/matplotlib/state.py

@@ -34,13 +37,69 @@ def notify(self, *args, **kwargs):

 VALID_WEIGHTS = ['light', 'normal', 'medium', 'semibold', 'bold', 'heavy', 'black']

-VALID_LOCATIONS = ['best',
+VALID_LOCATIONS = ['best (draggable)', 'best',


What about just 'draggable' for the first option?

Much better indeed !

specmicp · 2020-05-25T21:09:00Z

@astrofrog I changed the naming according to your requests, and corrected some bugs. So I think this PR is ready to be merged for more testing.

I will do a new PR soon, with more tests and the documentation of the API, if no big problem is discovered soon.

astrofrog

Looks great, thanks!

Legend background and foreground colors can be modified

aff19ce

- Change in visual properties of the legend do not require to get the handles anymore - Background and foreground can be changed - Defaults for these properties are taken from the settings - The legend is also set to be draggable

Add customization of legend box edge

32c2a6a

Add option for draggable legend

Separate legend state from the main matplotlib viewers

4106105

Solve's legend inconsistency between qt interface and glue interface

astrofrog reviewed May 25, 2020

View reviewed changes

specmicp added 2 commits May 25, 2020 21:00

Legend: bugfix, codestyle and naming fix

68973c5

add tests and bugfix for MatplotlibLegendState

55567d0

specmicp force-pushed the legend_background branch from 08f2f87 to 55567d0 Compare May 25, 2020 19:52

update ui

836be79

astrofrog approved these changes May 26, 2020

View reviewed changes

astrofrog merged commit 2d6609b into glue-viz:master May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Legend background and foreground colors can be modified #2146

Legend background and foreground colors can be modified #2146

specmicp commented May 21, 2020

specmicp commented May 21, 2020 •

edited by astrofrog

Loading

codecov bot commented May 21, 2020 •

edited

Loading

specmicp commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

specmicp commented May 21, 2020

specmicp commented May 21, 2020

specmicp commented May 21, 2020

astrofrog left a comment

astrofrog May 25, 2020

specmicp May 25, 2020

astrofrog May 25, 2020

specmicp May 25, 2020

specmicp commented May 25, 2020 •

edited

Loading

astrofrog left a comment


		visible = DeferredDrawCallbackProperty(False, docstring="Whether to show the legend")

		loc_and_drag = DeferredDrawSelectionCallbackProperty(0, docstring="The location of the legend in the axis")

Legend background and foreground colors can be modified #2146

Legend background and foreground colors can be modified #2146

Conversation

specmicp commented May 21, 2020

specmicp commented May 21, 2020 • edited by astrofrog Loading

codecov bot commented May 21, 2020 • edited Loading

Codecov Report

specmicp commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

astrofrog commented May 21, 2020

specmicp commented May 21, 2020

specmicp commented May 21, 2020

specmicp commented May 21, 2020

astrofrog left a comment

Choose a reason for hiding this comment

astrofrog May 25, 2020

Choose a reason for hiding this comment

specmicp May 25, 2020

Choose a reason for hiding this comment

astrofrog May 25, 2020

Choose a reason for hiding this comment

specmicp May 25, 2020

Choose a reason for hiding this comment

specmicp commented May 25, 2020 • edited Loading

astrofrog left a comment

Choose a reason for hiding this comment

specmicp commented May 21, 2020 •

edited by astrofrog

Loading

codecov bot commented May 21, 2020 •

edited

Loading

specmicp commented May 25, 2020 •

edited

Loading