leginee commented on code in PR #485:
URL: https://github.com/apache/openoffice/pull/485#discussion_r3455532589
##########
main/sc/source/ui/view/viewdata.cxx:
##########
@@ -2952,6 +2952,15 @@ void ScViewData::ReadUserDataSequence(const
uno::Sequence <beans::PropertyValue>
pTabData[nZoomTab]->aPageZoomY = aDefPageZoomY;
}
+ // The loop above delete'd and re-new'd pTabData[] entries (including
the
+ // active one) but left pThisTab pointing at a freed ScViewDataTable.
Restore
+ // the pThisTab == pTabData[nTabNo] invariant before anyone
dereferences it
+ // (e.g. ScTabView::SetTabNo -> GetActivePart()). Mirrors SetTabNo
(line
+ // ~1502). Without it, a debug build AVs on document open (reads
0xDDDDDDDD);
+ // release masks it only via allocator MRU reuse.
+ CreateTabData( nTabNo );
+ pThisTab = pTabData[nTabNo];
Review Comment:
> Why not rather call ScViewData::UpdateThis()?
> This code creates pTabData[nTabNo] if it does not exist.
> ScViewData::UpdateThis() searches for the last existing element of
pTabData[] without changing its contents.
I let the AI answer:
Both would refresh pThisTab, but UpdateThis() is the wrong tool here — it
carries a side effect that's appropriate for tab deletion, not for an
active-tab read. Three reasons our two-line form is correct and UpdateThis()
isn't:
1. UpdateThis() mutates nTabNo — it clamps the active sheet downward.
```
void ScViewData::UpdateThis() {
do {
pThisTab = pTabData[nTabNo];
if (!pThisTab) {
if (nTabNo>0) --nTabNo; // ← walks the active tab
DOWN
else pThisTab = pTabData[0] = new ScViewDataTable;
}
} while (!pThisTab);
}
```
That downward walk is exactly what DeleteTab/InsertTab (lines 515/528) want:
tabs were removed, so nTabNo may now be out of range and must be pulled back to
a surviving sheet. But ReadUserDataSequence is restoring the saved active tab —
nTabNo is meaningful and must not move. If the active sheet happened to have no
entry in the user data (pTabData[nTabNo] == NULL), UpdateThis() would silently
switch the document to a different, lower-indexed sheet on open. That's a
behavior regression, not a fix. CreateTabData(nTabNo) instead materializes that
slot in place and leaves nTabNo alone.
2. CreateTabData initializes the table; UpdateThis's fallback doesn't.
CreateTabData
([viewdata.cxx:1470](vscode-webview://0and1hd7healr0tlpsb02psleiicthqlvde48hdaraebfrgihgpa/main/sc/source/ui/view/viewdata.cxx#L1470))
seeds the new ScViewDataTable with the default zoom (eDefZoomType, aDefZoomX,
…) — which is the whole subject of the code right above our fix (the
zoom-default loop at 2944-2953). UpdateThis's NULL branch only does a bare new
ScViewDataTable (and only at index 0), with no zoom defaults. So our form stays
consistent with the surrounding zoom logic.
3. It's a verbatim copy of the file's own blessed idiom. SetTabNo (lines
[1501-1502](vscode-webview://0and1hd7healr0tlpsb02psleiicthqlvde48hdaraebfrgihgpa/main/sc/source/ui/view/viewdata.cxx#L1501))
is literally CreateTabData(nTabNo); pThisTab = pTabData[nTabNo];. For a
user-authorized source divergence in a build-migration project, the right
posture is the smallest change that mirrors existing correct code in the same
translation unit — not reaching for a different helper with broader semantics.
Lower risk, obviously-correct, easy to defend upstream.
Worth noting (from the memory): the proper structural fix is neither — it's
to delete the cached pThisTab member entirely and derive it via a ThisTab()
const { return pTabData[nTabNo]; } accessor, making the invariant correct by
construction and killing the whole bug class. We didn't do that because the
goal was a minimal, low-risk unblock, not a refactor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
