Skip to content

Honor Config.ConversionForUnit at the per-call conversion sites - #349

Open
youdie006 wants to merge 1 commit into
signintech:masterfrom
youdie006:conversion-for-unit-at-call-sites
Open

Honor Config.ConversionForUnit at the per-call conversion sites#349
youdie006 wants to merge 1 commit into
signintech:masterfrom
youdie006:conversion-for-unit-at-call-sites

Conversation

@youdie006

Copy link
Copy Markdown

Closes #323.

Rect and Box each have two conversion helpers:

  • exported UnitsToPoints(t int) -- builds a defaultUnitConfig{Unit: t} from the unit alone, so ConversionForUnit is always 0 inside it (rect.go:27, box.go:9)
  • unexported unitsToPoints(unitConfigurator) -- takes the whole Config, so the option is honoured (rect.go:42, box.go:29)

Start uses the config-aware form:

gp.config.PageSize = *gp.config.PageSize.unitsToPoints(gp.config)
gp.config.TrimBox = *gp.config.TrimBox.unitsToPoints(gp.config)

Nine other call sites pass only gp.config.Unit and drop the option. Image is the clearest, because both forms sit on adjacent lines:

func (gp *GoPdf) Image(picPath string, x float64, y float64, rect *Rect) error {
	gp.UnitsToPointsVar(&x, &y)                // honours ConversionForUnit
	rect = rect.UnitsToPoints(gp.config.Unit)  // drops it

so position and size disagree inside one call:

ConversionForUnit=0:  50.00 10.00 140.00 cm /I6 Do
ConversionForUnit=2: 100.00 20.00 280.00 cm /I6 Do   <- before: 50.00 20.00 280.00

Same effect on page geometry -- the same Rect{W:100,H:200} gives 200.00 400.00 through Config.PageSize and 100.00 200.00 through PageOption.PageSize, byte-identical to setting ConversionForUnit: 0.

The field's doc comment (config.go:39) promises the opposite: "If this variable is not 0. This value will be used to calculate the unit conversion instead of the existing const value in the system. And if this variable is not 0. Value in Config.Unit will not be used."

Why here and not in UnitsToPoints

#323 points at Rect.UnitsToPoints itself. That function takes an int and cannot see ConversionForUnit, so fixing it there means changing an exported signature. The call sites already have the whole Config in hand, and Start shows the intended form -- so this changes the nine call sites and leaves the public API alone.

Behaviour change

Only for documents that set ConversionForUnit. With it at 0 both helpers take the identical switch branch, so output is byte-identical -- which is why the whole existing suite passes unmodified and no existing test row changes.

Verification

go test ./... -- 4 packages ok, before and after (the run needs test/out/ to exist). go vet . clean. gofmt -l . lists only arabic_alphabet.go, which is pre-existing and not in this diff.

Tests added covering Image, ImageByHolder, ImageByHolderWithOptions, and AddPageWithOption for both PageSize and TrimBox. ConversionForUnit had zero occurrences in any _test.go before this.

I mutation-checked each of the nine changed lines separately, anchored to its enclosing function, since .UnitsToPoints(gp.config.Unit) is textually identical at all nine and a plain substitution hits the wrong one. Five of the nine are killed by the new tests:

site pinned
Image (:735) yes
ImageByHolder (:491) yes
ImageByHolderWithOptions rect (:505) yes
AddPageWithOption TrimBox (:807) yes
AddPageWithOption PageSize (:808) yes
ImageByHolderWithOptions mask rect (:527) no
ImageFromWithOption (:765) no
CellWithOption (:1131) no
Cell (:1146) no

I would rather say that than imply full coverage. The four unpinned ones are the same one-line change on the same kind of site -- each takes a caller-supplied *Rect and converts it with the unit alone -- and I could not get a stable output diff for them inside this pass: the mask path needs a second image with an alpha channel, and the cell paths render through the font metrics, where I could not isolate the rect's contribution from the text layout. If you would rather I either cover them properly or drop them from this PR, say which and I will.

What I did not change

  • The exported Rect.UnitsToPoints / Box.UnitsToPoints signatures (see above).
  • Config.K (config.go:44). It is read nowhere in the repo and its comment is "Not sure", so a fix would be choosing semantics rather than restoring them -- that belongs in an issue, not here.
  • unitOverride precedence: unitsToPoints still short-circuits to rect.unitOverride when set, exactly as the exported form does.

Disclosure: AI-assisted. I found and prepared this with an AI assistant, and I ran and verified everything above myself.

Rect and Box each have two conversion helpers: the exported
UnitsToPoints(t int), which builds a defaultUnitConfig from the unit
alone and so always sees ConversionForUnit as 0, and the unexported
unitsToPoints(unitConfigurator), which takes the whole Config.

Start already uses the config-aware form for Config.PageSize and
Config.TrimBox. Nine other call sites pass only gp.config.Unit and
silently drop the option, so a document configured with
ConversionForUnit renders per-call rects at the built-in factor.

Image is the clearest case: the line above converts x and y through
UnitsToPointsVar, which honors the option, and the next line converts
the rect without it. Reported as signintech#323.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rect’s UnitsToPoints function lost ConversionForUnit parameter.

1 participant