I've rebased this branch onto #4511 to avoid conflicts with updates in that PR, so it should be merged AFTER that one.
In hindsight, I could have waited for that to be merged then rebased onto develop which I may do instead.

In hindsight, I could have waited for that to be merged then rebased onto develop which I may do instead.

OK, I've rebased onto develop and that cleaned up the history. Thanks!

Update: regarding the failed jobs, there are 2 failed tests in TestServer_boot, which are unrelated to this PR. I tried re-running, no luck... but I'm guessing it's a problem on the CI machines?

there are 2 failed tests in TestServer_boot, which are unrelated to this PR. I tried re-running, no luck... but I'm guessing it's a problem on the CI machines?

Yes, some tests that use the server occasionally and inconsistently fail, unfortunately.

While I was resuming work on #5161, I noticed that this PR dropped ControlSpec units from the Grid labels. I understand that having a single label in the plotter is better, but it also made DrawGrid.test a bit worse. Moreover, there's weird cropping of the numbers happening here. Compare:

(
c = ControlSpec.new(20,20000, \lin, units:"Hz");
g = GridLines(c);
d = DrawGrid.test(g, g, 300@300);
)

before this PR:

after this PR:

I have a workaround for the cropping that I'm using while I work on the other PR, but I'm not sure what's the best/easiest way to add units to DrawGrid.test. Or maybe it's not a big deal there?

What do you think @mtmccrea @telephon ?

For reference, here's my current workaround for DrawGrit.test:

DrawGrid {
(...)
	*test { arg horzGrid,vertGrid,bounds;
		var w, grid, insetW = 0, insetH = 0;
		bounds = bounds ?? {Rect(0,0,500,400)};
		grid = DrawGrid(bounds,horzGrid,vertGrid);
		grid.x.labelOffset = 20 @ 18;
		grid.y.labelOffset = 20 @ 20;
		horzGrid !? {insetH = 20};
		vertGrid !? {insetW = 26};
		w = Window("Grid",bounds).front;
		UserView(w,bounds ?? {w.bounds.moveTo(0,0)})
		.resize_(5)
		.drawFunc_({ arg v;
			grid.bounds = v.bounds.insetAll(insetW, 0, 0, insetH);
			grid.draw
		})
		.background_(Color.white)
		^grid
	}
}

Thanks for catching this, @dyfer!

This gave me the opportunity to look more closely at GridLines and I've put together a patch that should adapt DrawGrid:*test to the latest version:

*test { arg horzGrid, vertGrid, bounds;
		var w, grid;
		var insetH = 40, insetV = 20; 	// left, bottom margins for labels
		var pad = 15; 					// right, top margin

		bounds = bounds ?? { Rect(0, 0, 500, 400) };
		bounds = bounds.asRect; 		// in case bounds are a Point
		insetH = insetH + pad;
		insetV = insetV + pad;

		grid = DrawGrid(
			bounds.insetBy(insetH.half, insetV.half).moveTo(insetH-pad, pad),
			horzGrid, vertGrid
		);
		grid.x.labelOffset = insetH @ insetV.half;
		grid.y.labelOffset = insetH @ insetV.half;

		w = Window("Grid", bounds).front;

		UserView(w, bounds ?? { w.bounds.moveTo(0,0) })
		.drawFunc_({ |v| grid.draw })
		.onResize_({ |v|
			grid.bounds = v.bounds.insetBy(insetH.half, insetV.half).moveTo(insetH - pad, pad);
		})
		.resize_(5)
		.background_(Color.white);

		^grid
	}

The full fix would also need this setter to be swapped into DrawGridY:

lblRect = Rect.aboutPoint(
	Point(0, y), labelOffset.x/2, labelOffset.y/2
).right_(bounds.left - txtPad);

But may not be wouldn't be necessary for your testing...

The numbers are still a little cropped since the margins are hardcoded and previously I was testing with smaller numbers.

My example also hard-codes margins, but wide enough for testing default specs, I think.

I don't imagine this is used in production, but I think it should be still functional... (maybe?)

That's my impression also. The docs make it seem like GridLines never quite made it out of the prototype stage (for example the description mentions things that aren't yet implemented but "could be"? Maybe those comments should be removed).

Also, I'm not sure why the first label is not there, haven't checked that yet.

That's because the spec goes from 20 -> 20k, so GridLines doesn't return a label for 20 Hz (steps are 5k). changing the spec from 0 -> 20k would show a label for 0.
You can see the labels generated by the grid: d.x.commands.do(_.postln) (at the tail of the message list).

This is also in part because the recent PR also makes sure grid lines are drawn at the boundaries of the grid, whether or not a labeled gridline coincides with the boundaries. Without this, you would have "uncapped" edges of the plot, which looks strange (see my screenshots in the first post).

I think it's still reasonable to have made these design decisions based on the Plotter use case, which is almost certainly the predominant one. (Another design decision was to not vertically center the top- and botttom-most y-labels on the top/bottom grid lines, otherwise they could collide with top of the view or the leftmost xlabel, respectively).

In any case, is the best way to patch this by creating a new PR or somehow amend this one?

I'd like to revisit this tomorrow, re-test and update docs before pushing.

This gave me the opportunity to look more closely at GridLines and I've put together a patch that should adapt DrawGrid:*test to the latest version:

Thanks!

The numbers are still a little cropped since the margins are hardcoded and previously I was testing with smaller numbers.

My example also hard-codes margins, but wide enough for testing default specs, I think.

Sounds good.

Also, I'm not sure why the first label is not there, haven't checked that yet.

That's because the spec goes from 20 -> 20k, so GridLines doesn't return a label for 20 Hz (steps are 5k). changing the spec from 0 -> 20k would show a label for 0. You can see the labels generated by the grid: d.x.commands.do(_.postln) (at the tail of the message list).

This is also in part because the recent PR also makes sure grid lines are drawn at the boundaries of the grid, whether or not a labeled gridline coincides with the boundaries. Without this, you would have "uncapped" edges of the plot, which looks strange (see my screenshots in the first post).

Oh... so no label for the first line when not part of the step is consistent with the old behavior, right? I'm not sure I like it that way (would prefer the first line to always have a label), but that's probably something for a separate issue/PR.

I think it's still reasonable to have made these design decisions based on the Plotter use case, which is almost certainly the predominant one. (Another design decision was to not vertically center the top- and botttom-most y-labels on the top/bottom grid lines, otherwise they could collide with top of the view or the leftmost xlabel, respectively).

Yes, I agree the Plotter should be the template. And I like not centering the top/bottom y labels.

In any case, is the best way to patch this by creating a new PR or somehow amend this one?

I'm thinking - a new PR, on a new branch (well, you could push to this branch and open a new PR with it? but I'm not sure that would work / merge cleanly)

I'd like to revisit this tomorrow, re-test and update docs before pushing.

Great, thanks!

Aside from the inset, how do you feel about dropping spec units from the *test plot? I think that as long as they're included in the Plotter use case it's fine, but I was at first surprised to not see them in the *test case.

Finally, one more details: in the new version one of your plots misses vertical units. Is this intentional or some kind of a random occurence?

This is also in part because the recent PR also makes sure grid lines are drawn at the boundaries of the grid, whether or not a labeled gridline coincides with the boundaries. Without this, you would have "uncapped" edges of the plot, which looks strange (see my screenshots in the first post).

Oh... so no label for the first line when not part of the step is consistent with the old behavior, right?

Almost... in this case, previously, there would have been no first line/label, just blank space (the 'quantized' GridLines would have started at 5k). This PR added lines at the extents of the grid. You can see this behavior by dynamically resizing the window and watching how the grid behaves at the extents.

I'm not sure I like it that way (would prefer the first line to always have a label), but that's probably something for a separate issue/PR.

Yes, this would actually be a change in GridLines (likely -getParams) so that it produces a label/line for the values at the max and min of the spec (or range), not just the in-range 'quantized' steps.

In any case, is the best way to patch this by creating a new PR or somehow amend this one?

I'm thinking - a new PR, on a new branch (well, you could push to this branch and open a new PR with it? but I'm not sure that would work / merge cleanly)

Ok, I'll put together a new PR 👍

Aside from the inset, how do you feel about dropping spec units from the *test plot? I think that as long as they're included in the Plotter use case it's fine, but I was at first surprised to not see them in the *test case.

I'll look into this... Plotter now puts the units in the axis labelX/Y, not the grid labels. It does this when setting the spec and domainSpec, which isn't happening in this *test, so it may just be a matter of explicitly setting the label.

Finally, one more details: in the new version one of your plots misses vertical units. Is this intentional or some kind of a random occurrence?

The 'before' and 'after' images above aren't exact comparisons, because the plot sizes were just adjusted by hand to show a variety of behavior, including when labels automatically disappear below window size limits. So the case you highlighted likely isn't omitting units in particular, it's omitting all of the y-labels (axis and grid labels) altogether because of the size of the window, but I'll confirm this too.

See #5858 for the updated behavior: