You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@caio-pizzol
First, thank you. Five PRs from a first-time contributor is a lot to land on
anyone's week, and you have given them real attention — the notes on #3953 and #3961 caught things I had genuinely gotten wrong, not style nits. That kind of
review is expensive to give, and I don't take it for granted.
I also owe you an explanation, because the shape of what I opened is my doing
and it is harder to read than it needed to be.
I found one bug — the first column of a Hebrew section landing on the left —
and while fixing it I kept finding more in the same code. Rather than stopping,
I kept branching off my own work. CONTRIBUTING.md asks contributions to target main, so I targeted main, which means each stacked PR carries its parent's
commits in its own diff. The result is three PRs whose diffs overlap heavily and
two that don't overlap at all, with no way to tell which is which from the PR
list. That's on me, not on the guidelines. This issue is the map I should have
given you on day one.
Two are independent — start here if you want the quick ones
Safari leaves the RTL caret one position behind after a space. WebKit returns no rect for a collapsed range at the end of a text node; this restores the measurement.
4 files, +1800
Neither touches the column code. Neither depends on anything. They can merge in
any order, today.
Three are a stack, and must merge in order
main
└─ #3953 place the first column on the right in RTL sections
├─ #3961 give the footnote band its own column layout
└─ #3962 seven defects in column layout and balancing
#3953 is the foundation.ColumnLayout had no notion of direction at all.
This adds it and mirrors the column strip in RTL. Nothing else in the stack
compiles without it.
#3961 sits on #3953. A footnote band in a two-column RTL section printed one
body-column wide with the rule on the wrong side; Word prints one strip across
the content area. Only 2 commits are mine — everything else in its diff is #3953 and disappears when #3953 merges.
#3962 also sits on #3953 — not on #3961. Nine commits of mine covering seven
defects, most of them found by your review and cubic's. The first two are older
than the RTL work and are reachable in plain LTR documents.
#3961 and #3962 are siblings, not a chain: they don't depend on each other
and can merge in either order once #3953 is in.
Since you squash-merge, both will need a rebase on main after #3953 lands. I'll
do that the moment it merges — say so if you'd rather have it earlier.
There is a real boundary defect in getColumnAtX: at an exact RTL column
boundary an inclusive comparison hands a fragment to the wrong column, and with w:space="0" the two column resolvers disagree outright. cubic caught it on #3961 and I confirmed it.
The fix lives in #3962, not in #3953. So #3953 and #3961 would merge with it
still present. If you'd rather main never be in that state, say the word and
I'll move that commit down into #3953.
Status
CI is green on all five and the CLA is signed. I've replied to every review
comment except one open note on #3963 about the analysis cache, which I'm
working on now.
Thanks again for the patience. Happy to close, split, or re-target any of these
if a different shape would be easier for you to review.
@caio-pizzol
First, thank you. Five PRs from a first-time contributor is a lot to land on
anyone's week, and you have given them real attention — the notes on #3953 and
#3961 caught things I had genuinely gotten wrong, not style nits. That kind of
review is expensive to give, and I don't take it for granted.
I also owe you an explanation, because the shape of what I opened is my doing
and it is harder to read than it needed to be.
I found one bug — the first column of a Hebrew section landing on the left —
and while fixing it I kept finding more in the same code. Rather than stopping,
I kept branching off my own work.
CONTRIBUTING.mdasks contributions to targetmain, so I targetedmain, which means each stacked PR carries its parent'scommits in its own diff. The result is three PRs whose diffs overlap heavily and
two that don't overlap at all, with no way to tell which is which from the PR
list. That's on me, not on the guidelines. This issue is the map I should have
given you on day one.
Two are independent — start here if you want the quick ones
w:bCswith now:b. SuperDoc read onlyw:b, so headings that are bold in Word rendered at normal weight.Neither touches the column code. Neither depends on anything. They can merge in
any order, today.
Three are a stack, and must merge in order
#3953 is the foundation.
ColumnLayouthad no notion of direction at all.This adds it and mirrors the column strip in RTL. Nothing else in the stack
compiles without it.
#3961 sits on #3953. A footnote band in a two-column RTL section printed one
body-column wide with the rule on the wrong side; Word prints one strip across
the content area. Only 2 commits are mine — everything else in its diff is
#3953 and disappears when #3953 merges.
#3962 also sits on #3953 — not on #3961. Nine commits of mine covering seven
defects, most of them found by your review and cubic's. The first two are older
than the RTL work and are reachable in plain LTR documents.
#3961 and #3962 are siblings, not a chain: they don't depend on each other
and can merge in either order once #3953 is in.
Since you squash-merge, both will need a rebase on
mainafter #3953 lands. I'lldo that the moment it merges — say so if you'd rather have it earlier.
Suggested order
One thing worth knowing before #3953 merges
There is a real boundary defect in
getColumnAtX: at an exact RTL columnboundary an inclusive comparison hands a fragment to the wrong column, and with
w:space="0"the two column resolvers disagree outright. cubic caught it on#3961 and I confirmed it.
The fix lives in #3962, not in #3953. So #3953 and #3961 would merge with it
still present. If you'd rather
mainnever be in that state, say the word andI'll move that commit down into #3953.
Status
CI is green on all five and the CLA is signed. I've replied to every review
comment except one open note on #3963 about the analysis cache, which I'm
working on now.
Thanks again for the patience. Happy to close, split, or re-target any of these
if a different shape would be easier for you to review.