search

wilkelab / gridtext

Improved text rendering support for grid graphics in R
https://wilkelab.org/gridtext
Other
96 stars 17 forks source link

Moving 'gp' out of LogicalVector args #29

Open bandrewfox opened 2 years ago

bandrewfox commented 2 years ago

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

See issue comments: https://github.com/wilkelab/gridtext/issues/22#issuecomment-1310901004

clauswilke commented 2 years ago

Ok, looks like a few more fixes are needed. The function raster_grob() defined here: https://github.com/wilkelab/gridtext/blob/09d71ab703771f58cb87ccaf16f0871e221b94de/src/grid.cpp#L58 is missing these lines: https://github.com/wilkelab/gridtext/blob/09d71ab703771f58cb87ccaf16f0871e221b94de/src/grid.cpp#L27-L29

bandrewfox commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

clauswilke commented 2 years ago

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

bandrewfox commented 1 year ago

@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.