Fixed legend alignment bug

[ctessum]

Before, when there were multiple lengend entries, the thumbnail and the label were not vertically aligned correctly

[kortschak]

Is there some way you could write a test for this?

[eaburns]

Chris, could you please post an image demonstrating this? I am looking at example_barChart.png, which is generated from plotter/main.go. I don't see a difference in alignment between the legend text and the thumbnail when I (visually) compare with and without this change. It looks, to me, like the baseline of the text is aligned with the base of the barchart box in both cases.

By the way, it should probably be centered, but freetype-go doesn't currently expose the info needed to properly vertically align text. See: https://code.google.com/p/freetype-go/issues/detail?id=15.

[eaburns]

I take it back. I was in the wrong branch when I tried your change. But when I try it from your branch the legend still looks the same, but now the plot is drawn off center:

[ctessum]

So, When I clone the tip gonum/plot from and run main.go, I get this:

With the suggested change, it looks like this.

What is happening is that the legend icon is a rectangle covering the "icon" canvas. When the bottom of the rectangle is changed but the top is not, each icon just gets bigger. For line and scatterplots, the icon gets drawn in the center of the "icon" canvas, so the issue isn't as obvious but the fix is the same.

Note that in the tip, the eps, pdf, and svg versions of the example figures are off center like the one you included. One of the other PRs that I submitted should fix this.

[eaburns]

I suspect that this is another bug introduced by the Rectangle change.
Thanks for looking into this.

LGTM.

[kortschak]

I think you are right; with the previous code, incrementing Min.Y and retaining Size would just shift the Rect, This is not true with the new rect.

[eaburns]

I wonder what other bugs this introduced? I'm sure we'll find them in time, but it's too bad I never wrote proper tests :(

[kortschak]

I think I'll go over that PR again and see if I can spot anymore.

[sbinet]

yeah. sorry. I found one in vgimg/vgx11 too.

[kortschak]

Writing a reasonable test for this requires that Legend.draw be exported. Is there any reason why this should not happen, @eaburns?

[kortschak]

@sbinet would you send a PR for the issue in vgx11?

[eaburns]

I'll take a look when I can. Without looking, I wonder why draw needs to be
exported for us to look at the results of the vg recorder. Or are you
testing it some other way? The nice thing about your recorder is that it
doesn't require us to pollute the API by exposing unnecessary methods
simply for tests. However, I realize that it is rather low level and may be
tedious for some kinds of tests.

On Sat, Mar 14, 2015, 6:59 PM Dan Kortschak notifications@github.com
wrote:

@sbinet https://github.com/sbinet would you send a PR for the issue in
vgx11?

—
Reply to this email directly or view it on GitHub
#25 (comment).

[kortschak]

One of the things that I found difficult with doing tests with a recorder like thing is the amount of non-relevant drawing that is done when testing for sub component. This test is just for the legend, so it would be nice to only test for the legend drawing actions. The way to do this is allow just the legend to be drawn, that required draw to be expirted.

I think that to achieve some of the flexibility aims in the design discussion doc, this may ned to happen anyway.

[kortschak]

Actually thinking for a second. No, doesn't need to be exported for this. Sorry. The flexibility comment stands.

[eaburns]

So should we defer the decision about exporting it until we want more
flexibility?

On Mon, Mar 16, 2015, 4:45 PM Dan Kortschak notifications@github.com
wrote:

Actually thinking for a second. No, doesn't need to be exported for this.
Sorry. The flexibility comment stands.

—
Reply to this email directly or view it on GitHub
#25 (comment).

[kortschak]

Later is always better. Sorry for the noise.

[kortschak]

I have a test for this here: https://gist.github.com/kortschak/c616c4cbe672e329759c (the values for the Rectangle may not be sane, but it behaves correctly - you are welcome to adjust them if you want).

It's written based on gob_clean (it clobbers plot_test.go otherwise).

Oh, and it also depends on the existence of an export_test.go:

// Copyright ©2015 The gonum Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package plot

import (
    "github.com/gonum/plot/vg/draw"
)

// Draw exports the Legend draw method for testing.
func (l *Legend) Draw(c draw.Canvas) { l.draw(c) }

[eaburns]

I commented on the gist. I had another nitpick too but I couldn't comment on a line number, so I just didn't say it. I assume this will make it into a PR before we merge it anyway; maybe I'll remember my nit then :)

Did you verify the golden "want" value manually? You may want to add a comment above it saying how it was derived.

[kortschak]

Answered on gist.

Did you verify the golden "want" value manually? You may want to add a comment above it saying how it was derived.

Ha! Sure, the tests are purely for regression testing, so the image is verified (I trusted that @ctessum and you posted the correct images above and that these indicate the fix is correct) and I output the values of the Actions in the know working case using goon and mechanically/manually make them nicer for the source. I then check that the legend code without the fix fails. It does. It's not pretty really.

[kortschak]

Yesterday, I forked go-spew to make this kind of thing easier. The Golden values can be obtained with minimal effort using github.com/kortschak/utter.Dump.

[ctessum]

What are the steps forward with this?

[kortschak]

I would like the tests added. After that, I'm happy. Finding input that give rounder path comp values would presumably make @eaburns happy.

[eaburns]

Yes

On Fri, Mar 20, 2015, 4:16 PM Dan Kortschak notifications@github.com
wrote:

I would like the tests added. After that, I'm happy. Finding input that
give rounder path comp values would presumably make @eaburns happy.

—
Reply to this email directly or view it on GitHub
#25 (comment).

[kortschak]

I've updated the gist to avoid the values we were seeing - this will brittly depend on fonts. I don't think we can do anything about that.

[ctessum]

In order for the test to compile, legend.Draw needed to be exported. If that's not a problem, I'll squash and merge it with the master.

[eaburns]

I like Dan's export_test.go approach, which gets around exporting legend.Draw. See his comment above—the one including the gist.

[ctessum]

I guess I don't understand how that works. Where does export_test.go go to make it work any differently that just exporting legend.Draw?

[kortschak]

The difference is that export_test.go is only compiled when building tests, so legend.Draw only exists in the test executable. You'll notice that the rest of the tests are in package plot_test which export_test.go declares package plot. This allows us to escape the import cycles that otherwise exist.

If legend.draw is eventually exported, this requirement would go away.

[kortschak]

I don't understand what is going on here. The test should be in plot, not gob. Please rebase onto master and put the test into package plot.

[kortschak]

Ahh, OK. Carry on.

[ctessum]

Rebasing was what caused the problem. Let me know if you see any other issues.

[kortschak]

LGTM, but please wait for @eaburns

Are you happy with the instructions about how to merge that I pointed to?

[ctessum]

think so.

[kortschak]

OK. After Ethan has had a look, do the squash and then ping.

[eaburns]

It's unfortunate about the font sizes, but LGTM. Thanks for all the work on this!

[kortschak]

Yeah, I was digging around to see if I could fake it, but since it's in vg, I couldn't do that without resorting to unsafe messiness - which I don't think we want.

[ctessum]

Merged. Correctly?

[kortschak]

Not quite. The merge is correct, but without the squash and push -f to your branch, the link between the commit and this review is lost.