Moving 'gp' out of LogicalVector args

[bandrewfox]

For bugs #22 and #7 and #11 and #31

See issue comments:
#22 (comment)

[clauswilke]

Ok, looks like a few more fixes are needed. The function raster_grob() defined here:

gridtext/src/grid.cpp

Line 58 in 09d71ab

List raster_grob(RObject image, NumericVector x_pt, NumericVector y_pt, NumericVector width_pt, NumericVector height_pt,

is missing these lines:

gridtext/src/grid.cpp

Lines 27 to 29 in 09d71ab

	if (gp.isNULL()) {
	gp = gpar_empty();
	}

[bandrewfox]

Thanks for the quick response on this.

Based on the fact that your code and all the packages which depend on this code have been working ok, then should "gp" just be dropped from the arguments to raster_grob? As it is now and has been for 2+ years, it has been passed to LogicalVector, which I assume just means that it is ignored. So maybe it isn't needed. I'm worried that suddenly adding it back in (probably as you originally intended) would end up causing some other problems in code which depends on this.

[clauswilke]

No, it would be better to do it right. All the other functions in grid.cpp check whether the gp argument is null and if so create an empty gpar argument, just for some reason this never made it into the raster_grob() function, probably because it had the mistake you identified.

https://github.com/wilkelab/gridtext/blob/master/src/grid.cpp

[bandrewfox]

Alright, sounds good. Did you want me to update my branch and make a new pull request or are you going to do that?

[clauswilke]

Just make the change in your branch, commit, and push, and the PR should update.

[clauswilke]

Hm, still doesn't work. I'm a bit at a loss right now. Need to think about it.

[bandrewfox]

@clauswilke I think I fixed the two test failures. One was by changing the test to expect an empty gpar() object rather than NULL. The other was to set the gp param to an empty gpar object rather than an empty list within the cpp code. Let me know if this passes the workflow tests.