Change default padding based on axis titles

[aaronayres35]

closes #564

Currently, with manual testing this seems to work how I want it to but I am not sure the current solution is worth the added complexity.
If we're going to add complexity to the code I think it might be better to try and get a more "perfect" approach that sets padding directly based off of the axis title extent. Hence the WIP title as I am still thinking about alternate solutions.

[aaronayres35]

if explicitly set (or if padding trait is sett, which then explicitly sets these), that value will always be used. Setting sets the shadow traits (with leading underscore), and in the getters we explicitly check if the shadow trait is set and return it. Otherwise we then check for axis titles and adjust as needed either returning 80 or 50 as a default.

Note I am just now considering if a user explicitly sets one off these traits to 0 I dont think it will work as then we will not enter the if getattr(self, MAP[side]) block. This is a bug that needs fixing.
EDIT: fixed

[aaronayres35]

I handle all 4 cases because it is possible to set axis orientation to "top" or "bottom" as well

[aaronayres35]

as an example, this code:

rom numpy import linspace, sin
from traits.api import HasTraits, Instance
from traitsui.api import View, Item
from chaco.api import Plot, ArrayPlotData
from enable.api import ComponentEditor

class LinePlot(HasTraits):
    plot = Instance(Plot)

    traits_view = View(
        Item('plot',editor=ComponentEditor(), show_label=False),
        width=500, height=500, resizable=True, title="Chaco Plot")

    def _plot_default(self):
        x = linspace(-14, 14, 100)
        y = sin(x) * x**3
        plotdata = ArrayPlotData(x=x, y=y)

        plot = Plot(plotdata)
        plot.plot(("x", "y"), type="line", color="blue")
        plot.title = "sin(x) * x^3"

        plot.padding_left = 0
        
        plot.x_axis.title = "X AXIS"
        plot.y_axis.title = "Y AXIS"
        
        plot.y_axis.orientation = "top"
       
        return plot

if __name__ == "__main__":
    LinePlot().configure_traits()

yields this on master:

and this on this branch:

[aaronayres35]

The extra padding on the bottom on this branch (80 rather than 50) I'm not a huge fan of. This is due to the simplistic nature of just adding space if we have an axis title. As you can see, at the top we need extra space so axis title doesn't overlap with plot title. Similarly left right we need it (hence the original issue), because axis ticks / values take up some of the spacing and sort of push the axis title further away from the plot. The x axis doesn't usually have this problem because of the orientation of the numbers on the ticks. In other words the text for 1000 for example is much wider than it is tall, and 100000 even wider, but the same height.

In reality an ideal solution would calculate explicitly the extent of the ticks, and the size of the axis title and allocate spacing to perfectly fit what we need. The awkward thing about this code feels like it would live in a draw method inside the axis itself, but the DataView is where the padding is held. Should the Axis take control over the DataViews padding? That feels wrong.
In any case that is a separate issue / approach entirely. I am just pointing out something I don't love about the current solution.

Perhaps we just make the assumption that padding_bottom can be 50 regardless of the presence or absence of an axis title? We were just assuming 50 always before anyway.

[aaronayres35]

@rahulporuri
I looked into it and Int(allow_none=True) does not work / is not implemented.

>>> from traits.api import HasTraits, Instance, Int
>>> class Thing(HasTraits):
...     a = Int()
...     b = Int(allow_none=True)
...     c = Instance(HasTraits, allow_none=False)
...     d = Instance(HasTraits)  # default value of allow_none for Instance trait type is True
... 
>>> thing = Thing()
>>> thing.d = None
>>> thing.c = None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/aayres/Desktop/traits/traits/base_trait_handler.py", line 75, in error
    object, name, self.full_info(object, name, value), value
traits.trait_errors.TraitError: The 'c' trait of a Thing instance must be a HasTraits, but a value of None <class 'NoneType'> was specified.
>>> thing.b = None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/aayres/Desktop/traits/traits/base_trait_handler.py", line 75, in error
    object, name, self.full_info(object, name, value), value
traits.trait_errors.TraitError: The 'b' trait of a Thing instance must be an integer, but a value of None <class 'NoneType'> was specified.

[rahulporuri]

I think this deserves an issue in traits - traits should explicitly complain during class definition instead of at runtime.

[aaronayres35]

That makes sense, for my own understanding we want to catch a user setting allow_none in a trait definition correct? I suspect, since allow_none is not a supported arg/kwarg for the Int trait type, it is now being used as metadata? Ie one could do something like @observe('+allow_none')?
We should probably special handle this case

[rahulporuri]

LGTM.

I've been putting off reviewing this PR because proposed solution looks slightly overworked/unweidly and I was hoping i'd be able to conjure something better out of thin air.

Alas, some solution is better than no solution so let's go ahead with this.

A couple of comments though :

• it would be good to add tests for the new code - to check that the padding values are what we expect
• i'm not sure where but i would like a reference to the original issue in the new code so that people who come asking questions know where to look.

[aaronayres35]

@rahulporuri this PR look good to push through?

[rahulporuri]

still LGTM