plot: properly handle descent in axis labels glyph boxes

[sbinet]

Fixes #676.
Fixes #680.

Please take a look.

[codecov-commenter]

Codecov Report

Merging #708 (2fdb2e0) into master (5291cf6) will increase coverage by 0.59%.
The diff coverage is 100.00%.

❗ Current head 2fdb2e0 differs from pull request most recent head c29ba9a. Consider uploading reports for the commit c29ba9a to get more accurate results

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   72.53%   73.12%   +0.59%     
==========================================
  Files          56       58       +2     
  Lines        5076     5370     +294     
==========================================
+ Hits         3682     3927     +245     
- Misses       1207     1249      +42     
- Partials      187      194       +7     

Impacted Files	Coverage Δ
axis.go	86.94% <100.00%> (+2.41%)	⬆️
legend.go	96.92% <100.00%> (ø)
plot.go	79.42% <100.00%> (+8.97%)	⬆️
vg/vggio/vggio.go	67.37% <0.00%> (ø)
vg/vggio/context.go	100.00% <0.00%> (ø)
vg/geom.go	81.25% <0.00%> (+18.75%)	⬆️

---

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 5291cf6...c29ba9a. Read the comment docs.

[sbinet]

I'll address your comments shortly.
It's still a draft pr :)

(Mainly because there's something weird going on with the palette, probably coming from a larger vertical source being used up for each element of the palette. It's also present for legend entries.
There's also still an issue with the glyph boxes for the axis tick labels.
And I need to add the glyph boxes for legend entries too.

If somebody has any clue for the palette issue, I am all ears.)

[sbinet]

it's somewhat better.
but there's probably still an issue with the legends, as testdata/legend_standalone_golden.png might indicate (notice the purple boxes around the legends on the top. it's probably an issue with the "legend icons" rather than the "legend text", though.)

and I've "fixed" the glyphboxes around the tick-axis labels in testdata/glyphbox_golden.png, but that's only through some trial&error and hardcoding some magical values.
this needs to be properly fixed.

[sbinet]

I think I've found (one of) the glyphbox bug(s) for the tick-axis labels.
the glyphbox doesn't take into account the displacement originating from the axis label.

[sbinet]

ok, it's finally looking better.
TODO:

• handle glyphbox for the plot's title
• handle glyphbox for the axes' labels
• handle glyphbox for the legends

for these items, one needs the underlying vg/draw.Canvas (to center or not the label/title, to get the legend-canvas dimensions).
we'd probably need to change the plot.GlyphBoxer interface accordingly.

[sbinet]

playing a bit w/ the "extended" plot.GlyphBoxer interface, I've found another bug: padY doesn't take into account the space for the plot title.
it's taken care of outside padY for plot.Plot.Draw and plot.Plot.DataCanvas, but not in other places.

refactoring the handling of carving out the space for the title into padY would help with #680.

before I go too deep into that alley, would you (@kortschak) agree with proceeding with this interface change?
it's a bit tricky because it's an optional interface, so people's code might be silently broken.
maybe, during some transition period, test for the old interface inside plot.Plot.GlyphBoxes, collect the types that implement it, and log them (or panic)?

[kortschak]

What does the extended interface look like?

Before I read you last sentence, I was thinking that perhaps a two phase logging and then panicking transition would work, but you already got there. Yes, agree.

[sbinet]

going from:

type GlyphBoxer interface {
        GlyphBoxes(*Plot) []GlyphBox
}

to the new:

type GlyphBoxer interface {
        GlyphBoxes(c draw.Canvas, p *Plot) []GlyphBox
}

it kind of make sense to tack the additional draw.Canvas parameter as plot.Plotter has it already (and that's what is used by the plotters to draw stuff, so they may use some of the informations from draw.Canvas to draw their data).

type Plotter interface {
        // Plot draws the data to a draw.Canvas.
        Plot(draw.Canvas, *Plot)
}

[kortschak]

So the draw.Canvas is there just to provide the mapping between coordinate systems? Is that correct?

[sbinet]

For the plot title and for the legend, it's also used to get the min/max of the area (to compute the headroom for the title, or to position the legend box on the correct corner)

[kortschak]

Sure, that SGTM then.

[sbinet]

here is the before/after:

after:

[sbinet]

and here is the "final final".
I couldn't manage to make the legend glyphbox to work.

rotated title and axes' labels aren't working either:

because of the GlyphBox position (X, Y fields) being in data coordinates, having the extra data.Canvas parameter was actually worthless (as there is only the ability to go from "data coordinates" to "canvas coordinates" but not the other way around).
what I did for the axes' labels only works for the default (centered) position, but doesn't work for the (top|right) position for the (resp.) (Y|X) axes.

here's the diff:

diff --git a/axis.go b/axis.go
index b1c7704..55693fa 100644
--- a/axis.go
+++ b/axis.go
@@ -302,6 +302,20 @@ func (a horizontalAxis) GlyphBoxes(c draw.Canvas, p *Plot) []GlyphBox {
        )
 
        if a.Label.Text != "" {
+               x := a.Norm(p.X.Max)
+               switch a.Label.Position {
+               case draw.PosCenter:
+                       x = a.Norm(0.5*(p.X.Max-p.X.Min) + p.X.Min)
+               case draw.PosRight:
+                       x -= a.Norm(0.5 * a.Label.TextStyle.Width(a.Label.Text).Points()) // FIXME(sbinet): want data coordinates
+               }
+               descent := a.Label.TextStyle.FontExtents().Descent
+               boxes = append(boxes, GlyphBox{
+                       X: x,
+                       Rectangle: a.Label.TextStyle.Rectangle(a.Label.Text).Add(vg.Point{
+                               Y: yoff + descent,
+                       }),
+               })
                yoff += a.Label.TextStyle.Height(a.Label.Text)
                yoff += a.Label.Padding
        }
@@ -417,11 +431,26 @@ func (a verticalAxis) GlyphBoxes(c draw.Canvas, p *Plot) []GlyphBox {
        )
 
        if a.Label.Text != "" {
+               yoff := a.Norm(p.Y.Max)
+               switch a.Label.Position {
+               case draw.PosCenter:
+                       yoff = a.Norm(0.5*(p.Y.Max-p.Y.Min) + p.Y.Min)
+               case draw.PosTop:
+                       yoff -= a.Norm(0.5 * a.Label.TextStyle.Width(a.Label.Text).Points()) // FIXME(sbinet): want data coordinates
+               }
+
                sty := a.Label.TextStyle
                sty.Rotation += math.Pi / 2
 
                xoff += a.Label.TextStyle.Height(a.Label.Text)
-               xoff += a.Label.TextStyle.FontExtents().Descent
+               desc := a.Label.TextStyle.FontExtents().Descent
+               boxes = append(boxes, GlyphBox{
+                       Y: yoff,
+                       Rectangle: sty.Rectangle(a.Label.Text).Add(vg.Point{
+                               X: xoff - desc,
+                       }),
+               })
+               xoff += desc
                xoff += a.Label.Padding
        }
 
diff --git a/legend.go b/legend.go
index 6d55dea..49cc4bb 100644
--- a/legend.go
+++ b/legend.go
@@ -171,8 +171,32 @@ func (l *Legend) Rectangle(c draw.Canvas) vg.Rectangle {
 }
 
 func (l *Legend) GlyphBoxes(c draw.Canvas, p *Plot) []GlyphBox {
-       // TODO
-       return nil
+       var (
+               iconx = c.Min.X
+               sty   = l.TextStyle
+               em    = sty.Rectangle(" ")
+               enth  = l.entryHeight()
+               desc  = sty.FontExtents().Descent
+               xoff  = iconx + l.ThumbnailWidth + em.Max.X
+               yoff  = c.Max.Y - enth - desc
+       )
+
+       if !l.Left {
+               iconx = c.Max.X - l.ThumbnailWidth
+               xoff = iconx - em.Max.X
+       }
+       xoff += l.XOffs
+
+       if !l.Top {
+               yoff = c.Min.Y + (enth+l.Padding)*(vg.Length(len(l.entries))-1)
+       }
+       yoff += l.YOffs
+
+       return []GlyphBox{{
+               X:         xoff.Points(), // FIXME(sbinet): want data coordinates
+               Y:         yoff.Points(), // FIXME(sbinet): want data coordinates
+               Rectangle: l.Rectangle(c),
+       }}
 }
 
 // entryHeight returns the height of the tallest legend
diff --git a/plot.go b/plot.go
index dc2a1fb..4b76250 100644
--- a/plot.go
+++ b/plot.go
@@ -147,11 +147,13 @@ func (p *Plot) Draw(c draw.Canvas) {
                c.SetColor(p.BackgroundColor)
                c.Fill(c.Rectangle.Path())
        }
+
        if p.Title.Text != "" {
                descent := p.Title.TextStyle.FontExtents().Descent
                c.FillText(p.Title.TextStyle, vg.Point{X: c.Center().X, Y: c.Max.Y + descent}, p.Title.Text)
-               _, h, d := p.Title.TextStyle.Handler.Box(p.Title.Text, p.Title.TextStyle.Font)
-               c.Max.Y -= h + d
+
+               rect := p.Title.TextStyle.Rectangle(p.Title.Text)
+               c.Max.Y -= rect.Size().Y
                c.Max.Y -= p.Title.Padding
        }
 
@@ -179,7 +181,8 @@ func (p *Plot) Draw(c draw.Canvas) {
 // the plot data will be drawn.
 func (p *Plot) DataCanvas(da draw.Canvas) draw.Canvas {
        if p.Title.Text != "" {
-               da.Max.Y -= p.Title.TextStyle.Height(p.Title.Text) + p.Title.TextStyle.FontExtents().Descent
+               rect := p.Title.TextStyle.Rectangle(p.Title.Text)
+               da.Max.Y -= rect.Size().Y
                da.Max.Y -= p.Title.Padding
        }
        p.X.sanitizeRange()
@@ -210,6 +213,16 @@ func (p *Plot) DrawGlyphBoxes(c draw.Canvas) {
                })
        }
 
+       if p.Title.Text != "" {
+               box := GlyphBox{
+                       Rectangle: p.Title.TextStyle.Rectangle(p.Title.Text).Add(vg.Point{
+                               X: c.Center().X,
+                               Y: c.Max.Y,
+                       }),
+               }
+               drawBox(c, box)
+       }
+
        for _, b := range p.GlyphBoxes(dac, p) {
                drawBox(dac, b)
        }
@@ -229,11 +242,21 @@ func (p *Plot) DrawGlyphBoxes(c draw.Canvas) {
        }
 
        cy := padY(p, draw.Crop(c, 0, 0, xheight, 0))
+       if p.Title.Text != "" {
+               rect := p.Title.TextStyle.Rectangle(p.Title.Text)
+               cy.Max.Y -= rect.Size().Y
+               cy.Max.Y -= p.Title.Padding
+       }
        for _, b := range y.GlyphBoxes(cy, p) {
                drawBox(cy, b)
        }
 
        cl := draw.Crop(c, ywidth, 0, xheight, 0)
+       if p.Title.Text != "" {
+               rect := p.Title.TextStyle.Rectangle(p.Title.Text)
+               cl.Max.Y -= rect.Size().Y
+               cl.Max.Y -= p.Title.Padding
+       }
        for _, b := range p.Legend.GlyphBoxes(cl, p) {
                drawBox(cl, b)
        }
diff --git a/plot_test.go b/plot_test.go
index 7b19777..30888a7 100644
--- a/plot_test.go
+++ b/plot_test.go
@@ -133,11 +133,15 @@ func TestDrawGlyphBoxes(t *testing.T) {
        cmpimg.CheckPlot(func() {
                p := plot.New()
 
+               p.Title.Text = "My very very very\nlong Title"
                p.X.Min = 0
                p.X.Max = 10
                p.Y.Min = 0
                p.Y.Max = 10
 
+               p.X.Label.Text = "X-axis"
+               p.Y.Label.Text = "Y-axis"
+
                f1 := plotter.NewFunction(func(x float64) float64 { return 5 })
                f1.LineStyle.Color = color.RGBA{R: 255, A: 255}
 

so, well... perhaps this interface change isn't needed just yet as we (at least I) don't understand all the requirements and how they fit together to do what we want.

I'd be tempted to:

• keep the "old" GlyphBoxer interface
• leave the non-working glyphboxes for rotated title and labels
• leave the non-working glyphbox for legend
• file issues for these non-working glyphboxes
• proceed with the PR as is.

WDYT?

[kortschak]

I think that is the way to go. We can attempt to fix the non-working things later.

[sbinet]

great.
and sorry for the back and forth as well as the "stream of consciousness".

[sbinet]

PTAL

[sbinet]

PTAL

[sbinet]

argh... after squashing the fixes from the review into the proper commit, vg/vggio is acting up again.
this diff "fixed" it again but it's rather vexing:

diff --git a/vg/vggio/vggio_test.go b/vg/vggio/vggio_test.go
index 259e53b..dfd0d67 100644
--- a/vg/vggio/vggio_test.go
+++ b/vg/vggio/vggio_test.go
@@ -13,6 +13,7 @@ import (
        "os"
        "runtime"
        "testing"
+       "time"
 
        "gioui.org/layout"
        "gioui.org/op"
@@ -55,6 +56,7 @@ func init() {
                if err == nil {
                        return
                }
+               time.Sleep(5 * time.Second)
        }
 
        panic(fmt.Errorf("vg/vggio_test: could not setup headless display: %+v", err))

I'll merge this PR w/o that fix/hack (as this isn't what you approved), and see whether this needs further attention.

[sbinet]

this time it went fine...