Background
Raised in review of #153 (browser-floor wheel/scroll modernisation). The wheel-delta sign fix (b71d05ee on that PR) had to be applied identically at 7 call sites across 5 editors:
event.deltaY !== undefined ? -event.deltaY : event.wheelDelta — sign-only, in {documenteditor,pdfeditor,presentationeditor,visioeditor}/main/app/controller/DocumentHolder.js and spreadsheeteditor/main/app/controller/DocumentHolder.jse.deltaY !== undefined ? -(e.deltaMode === 1 ? e.deltaY * 30 : e.deltaY) : (...) — TabBar.js, adds deltaMode line-unit scalinge.deltaY > 0 — Print.js ×5, boolean shape
Sign convention bugs have already bitten this exact codebase once (that's what b71d05ee fixed). Consolidating the sign-normalisation logic into a single Common.Utils helper would mean a future convention fix only needs to land once.
Why this wasn't done in #153
Deliberately deferred, not overlooked — the DocumentHolder.js files are upstream-maintained OnlyOffice source. #129's audit doc set the standing principle for this fork: minimise source-level delta on upstream-shared files, because every line of local divergence there is a future merge-conflict site. Threading a fork-specific helper call into 5 upstream files to save a one-line duplication works against that principle more than the duplication itself costs.
Options for whoever picks this up
- Cheapest, near-zero fork delta: a one-line comment at each site naming the up-positive convention the branches expect (e.g.
// up-positive: negative delta = scroll up), rather than a shared function. Catches a reviewer's eye without adding a symbol upstream doesn't have.
- Full extraction: only worth it if the fork's merge cadence with upstream on these specific files is low enough that the extra delta doesn't matter — check recent merge-conflict history on
DocumentHolder.js across editors before deciding.
- Note the shape isn't fully uniform: the 5
DocumentHolder.js sites are sign-only, TabBar.js adds deltaMode scaling, Print.js is a boolean — a single helper wouldn't cleanly cover all 7 as one signature.
Not blocking anything; low priority.
Background
Raised in review of #153 (browser-floor wheel/scroll modernisation). The wheel-delta sign fix (
b71d05eeon that PR) had to be applied identically at 7 call sites across 5 editors:event.deltaY !== undefined ? -event.deltaY : event.wheelDelta— sign-only, in{documenteditor,pdfeditor,presentationeditor,visioeditor}/main/app/controller/DocumentHolder.jsandspreadsheeteditor/main/app/controller/DocumentHolder.jse.deltaY !== undefined ? -(e.deltaMode === 1 ? e.deltaY * 30 : e.deltaY) : (...)—TabBar.js, adds deltaMode line-unit scalinge.deltaY > 0—Print.js×5, boolean shapeSign convention bugs have already bitten this exact codebase once (that's what
b71d05eefixed). Consolidating the sign-normalisation logic into a singleCommon.Utilshelper would mean a future convention fix only needs to land once.Why this wasn't done in #153
Deliberately deferred, not overlooked — the
DocumentHolder.jsfiles are upstream-maintained OnlyOffice source. #129's audit doc set the standing principle for this fork: minimise source-level delta on upstream-shared files, because every line of local divergence there is a future merge-conflict site. Threading a fork-specific helper call into 5 upstream files to save a one-line duplication works against that principle more than the duplication itself costs.Options for whoever picks this up
// up-positive: negative delta = scroll up), rather than a shared function. Catches a reviewer's eye without adding a symbol upstream doesn't have.DocumentHolder.jsacross editors before deciding.DocumentHolder.jssites are sign-only,TabBar.jsadds deltaMode scaling,Print.jsis a boolean — a single helper wouldn't cleanly cover all 7 as one signature.Not blocking anything; low priority.