toolkit/inc/controls/table/AccessibleGridControl.hxx | 2 toolkit/inc/controls/table/tablecontrol.hxx | 4 - toolkit/source/controls/svtxgridcontrol.cxx | 10 +- toolkit/source/controls/table/AccessibleGridControl.cxx | 5 - toolkit/source/controls/table/tablecontrol.cxx | 55 ++++++---------- toolkit/source/controls/table/tablecontrol_impl.cxx | 13 --- toolkit/source/controls/table/tablecontrol_impl.hxx | 2 7 files changed, 33 insertions(+), 58 deletions(-)
New commits: commit 33a7e6910f7075c3b24b56feba4e6318caeb4952 Author: Michael Weghorn <[email protected]> AuthorDate: Tue Jan 28 09:23:53 2025 +0100 Commit: Michael Weghorn <[email protected]> CommitDate: Wed Jan 29 08:25:36 2025 +0100 uno grid a11y: Drop check for a11y context disposed from outside AccessibleGridControlAccess owns its AccessibleGridControl (`m_xContext` member) and is responsible for disposing it. It does that in AccessibleGridControlAccess::DisposeAccessImpl where it also clears `m_xContext` to nullptr. Therefore, this // if the context died meanwhile (we're no listener, so it won't tell us explicitly when this happens), // then reset and re-create. check looks very suspicious and should never be reached, as anything "from outside", i.e. something not owning the AccessibleGridControl should never dispose it. Drop that check. (In case there actually *is* something disposing the accessible context from elsewhere, that should most likely be changed in the first place.) See also Change-Id: Id16dd7dbf8264d887f52e2fe304b0568079cb924 Author: Michael Weghorn <[email protected]> Date: Tue Jan 28 09:17:10 2025 +0100 uno grid a11y: Replace alive check by simple null check TableControl_Impl owns its accessible (`m_pAccessibleTable`) and disposes it in TableControl_Impl::disposeAccessible which gets called from TableControl::dispose. TableControl_Impl::disposeAccessible also sets `m_pAccessibleTable` to nullptr. As `m_pAccessibleTable` should only ever be disposed by its owner, I can't think of any (valid) scenario where `m_pAccessibleTable` could be non-null but disposed, so simply use a null check instead of TableControl_Impl::isAccessibleAlive that was checking for it being non-null and not diposed. for a similar commit elsewhere. Change-Id: Ibe89b39be7540d68144400927bc96c318b2873c1 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/180831 Reviewed-by: Michael Weghorn <[email protected]> Tested-by: Jenkins diff --git a/toolkit/source/controls/table/AccessibleGridControl.cxx b/toolkit/source/controls/table/AccessibleGridControl.cxx index 35ce22d2e798..0e0a9dab1af5 100644 --- a/toolkit/source/controls/table/AccessibleGridControl.cxx +++ b/toolkit/source/controls/table/AccessibleGridControl.cxx @@ -255,11 +255,6 @@ css::uno::Reference< css::accessibility::XAccessibleContext > SAL_CALL Accessibl { SolarMutexGuard g; - // if the context died meanwhile (we're no listener, so it won't tell us explicitly when this happens), - // then reset and re-create. - if ( m_xContext.is() && !m_xContext->isAlive() ) - m_xContext = nullptr; - if (!m_xContext.is() && m_xTable) m_xContext = new AccessibleGridControl(m_xParent, this, *m_xTable); commit 6b94a653b763655b09032e8b2964bf3eb03a8f7a Author: Michael Weghorn <[email protected]> AuthorDate: Tue Jan 28 09:17:10 2025 +0100 Commit: Michael Weghorn <[email protected]> CommitDate: Wed Jan 29 08:25:28 2025 +0100 uno grid a11y: Replace alive check by simple null check TableControl_Impl owns its accessible (`m_pAccessibleTable`) and disposes it in TableControl_Impl::disposeAccessible which gets called from TableControl::dispose. TableControl_Impl::disposeAccessible also sets `m_pAccessibleTable` to nullptr. As `m_pAccessibleTable` should only ever be disposed by its owner, I can't think of any (valid) scenario where `m_pAccessibleTable` could be non-null but disposed, so simply use a null check instead of TableControl_Impl::isAccessibleAlive that was checking for it being non-null and not diposed. Change-Id: Id16dd7dbf8264d887f52e2fe304b0568079cb924 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/180830 Tested-by: Jenkins Reviewed-by: Michael Weghorn <[email protected]> diff --git a/toolkit/inc/controls/table/AccessibleGridControl.hxx b/toolkit/inc/controls/table/AccessibleGridControl.hxx index a50f03d30f66..c1f1a649d2a3 100644 --- a/toolkit/inc/controls/table/AccessibleGridControl.hxx +++ b/toolkit/inc/controls/table/AccessibleGridControl.hxx @@ -162,8 +162,6 @@ public: void DisposeAccessImpl(); - bool isAlive() const { return m_xContext.is() && m_xContext->isAlive(); } - void commitCellEvent(sal_Int16 nEventId, const css::uno::Any& rNewValue, const css::uno::Any& rOldValue) { diff --git a/toolkit/source/controls/table/tablecontrol_impl.cxx b/toolkit/source/controls/table/tablecontrol_impl.cxx index fd64b459fa1e..30decac8f104 100644 --- a/toolkit/source/controls/table/tablecontrol_impl.cxx +++ b/toolkit/source/controls/table/tablecontrol_impl.cxx @@ -2268,14 +2268,14 @@ namespace svt::table void TableControl_Impl::commitCellEvent( sal_Int16 const i_eventID, const Any& i_newValue, const Any& i_oldValue ) { - if (isAccessibleAlive()) + if (m_pAccessibleTable) m_pAccessibleTable->commitCellEvent( i_eventID, i_newValue, i_oldValue ); } void TableControl_Impl::commitTableEvent( sal_Int16 const i_eventID, const Any& i_newValue, const Any& i_oldValue ) { - if (isAccessibleAlive()) + if (m_pAccessibleTable) m_pAccessibleTable->commitTableEvent( i_eventID, i_newValue, i_oldValue ); } @@ -2365,16 +2365,9 @@ namespace svt::table m_pAccessibleTable = nullptr; } - - bool TableControl_Impl::isAccessibleAlive() const - { - return m_pAccessibleTable && m_pAccessibleTable->isAlive(); - } - - void TableControl_Impl::impl_commitAccessibleEvent( sal_Int16 const i_eventID, Any const & i_newValue ) { - if (isAccessibleAlive()) + if (m_pAccessibleTable) m_pAccessibleTable->commitEvent( i_eventID, i_newValue ); } diff --git a/toolkit/source/controls/table/tablecontrol_impl.hxx b/toolkit/source/controls/table/tablecontrol_impl.hxx index bef89eb6c747..d55591477223 100644 --- a/toolkit/source/controls/table/tablecontrol_impl.hxx +++ b/toolkit/source/controls/table/tablecontrol_impl.hxx @@ -304,7 +304,6 @@ namespace svt::table virtual void tableMetricsChanged() override; private: - bool isAccessibleAlive() const; void impl_commitAccessibleEvent( sal_Int16 const i_eventID, css::uno::Any const & i_newValue commit abd9bb06912daac0568fd47c87886c519a37bad8 Author: Michael Weghorn <[email protected]> AuthorDate: Tue Jan 28 09:08:59 2025 +0100 Commit: Michael Weghorn <[email protected]> CommitDate: Wed Jan 29 08:25:21 2025 +0100 uno grid a11y: Let impl handle alive check Make TableControl_Impl::isAccessibleAlive private and drop all uses in TableControl. They are unnecessary there, because the TableControl_Impl methods that get called already call TableControl_Impl::isAccessibleAlive themselves to check whether the accessible is still alive, and that is in my opinion actually an implementation detail only the impl should have to care about. Drop "IfAccessibleAlive" from the method names for TableControl::commitCellEventIfAccessibleAlive and TableControl::commitTableEventIfAccessibleAlive. Change-Id: I5589289de0ca96ce5fc5993df5cef3877dce881a Reviewed-on: https://gerrit.libreoffice.org/c/core/+/180819 Tested-by: Jenkins Reviewed-by: Michael Weghorn <[email protected]> diff --git a/toolkit/inc/controls/table/tablecontrol.hxx b/toolkit/inc/controls/table/tablecontrol.hxx index a4ed69f4a60b..af5b1e2d9d1f 100644 --- a/toolkit/inc/controls/table/tablecontrol.hxx +++ b/toolkit/inc/controls/table/tablecontrol.hxx @@ -130,8 +130,8 @@ namespace svt::table // Those do not really belong into the public API - they're intended for firing A11Y-related events. However, // firing those events should be an implementation internal to the TableControl resp. TableControl_Impl, // instead of something triggered externally. - void commitCellEventIfAccessibleAlive( sal_Int16 const i_eventID, const css::uno::Any& i_newValue, const css::uno::Any& i_oldValue ); - void commitTableEventIfAccessibleAlive( sal_Int16 const i_eventID, const css::uno::Any& i_newValue, const css::uno::Any& i_oldValue ); + void commitCellEvent(sal_Int16 const i_eventID, const css::uno::Any& i_newValue, const css::uno::Any& i_oldValue); + void commitTableEvent(sal_Int16 const i_eventID, const css::uno::Any& i_newValue, const css::uno::Any& i_oldValue); sal_Int32 GetAccessibleControlCount() const; sal_Int32 GetRowCount() const; diff --git a/toolkit/source/controls/svtxgridcontrol.cxx b/toolkit/source/controls/svtxgridcontrol.cxx index 74036177d8ab..33468dd3c3f2 100644 --- a/toolkit/source/controls/svtxgridcontrol.cxx +++ b/toolkit/source/controls/svtxgridcontrol.cxx @@ -792,12 +792,12 @@ void SVTXGridControl::ProcessWindowEvent( const VclWindowEvent& rVclWindowEvent // works when the control is used outside the UNO context if (pTable->GetCurrentRow() != ROW_INVALID && pTable->GetCurrentColumn() != COL_INVALID) { - pTable->commitCellEventIfAccessibleAlive( + pTable->commitCellEvent( AccessibleEventId::STATE_CHANGED, Any( AccessibleStateType::FOCUSED ), Any() ); - pTable->commitTableEventIfAccessibleAlive( + pTable->commitTableEvent( AccessibleEventId::ACTIVE_DESCENDANT_CHANGED, Any(), Any() @@ -805,7 +805,7 @@ void SVTXGridControl::ProcessWindowEvent( const VclWindowEvent& rVclWindowEvent } else { - pTable->commitTableEventIfAccessibleAlive( + pTable->commitTableEvent( AccessibleEventId::STATE_CHANGED, Any( AccessibleStateType::FOCUSED ), Any() @@ -820,7 +820,7 @@ void SVTXGridControl::ProcessWindowEvent( const VclWindowEvent& rVclWindowEvent // works when the control is used outside the UNO context if (pTable->GetCurrentRow() != ROW_INVALID && pTable->GetCurrentColumn() != COL_INVALID) { - pTable->commitCellEventIfAccessibleAlive( + pTable->commitCellEvent( AccessibleEventId::STATE_CHANGED, Any(), Any( AccessibleStateType::FOCUSED ) @@ -828,7 +828,7 @@ void SVTXGridControl::ProcessWindowEvent( const VclWindowEvent& rVclWindowEvent } else { - pTable->commitTableEventIfAccessibleAlive( + pTable->commitTableEvent( AccessibleEventId::STATE_CHANGED, Any(), Any( AccessibleStateType::FOCUSED ) diff --git a/toolkit/source/controls/table/tablecontrol.cxx b/toolkit/source/controls/table/tablecontrol.cxx index 555ad34a69b8..9ea268764edf 100644 --- a/toolkit/source/controls/table/tablecontrol.cxx +++ b/toolkit/source/controls/table/tablecontrol.cxx @@ -141,24 +141,21 @@ namespace svt::table Control::KeyInput( rKEvt ); else { - if ( m_pImpl->isAccessibleAlive() ) - { - m_pImpl->commitCellEvent( AccessibleEventId::STATE_CHANGED, - Any( AccessibleStateType::FOCUSED ), - Any() - ); - // Huh? What the heck? Why do we unconditionally notify a STATE_CHANGE/FOCUSED after each and every - // (handled) key stroke? - - m_pImpl->commitTableEvent( AccessibleEventId::ACTIVE_DESCENDANT_CHANGED, - Any(), - Any() - ); - // ditto: Why do we notify this unconditionally? We should find the right place to notify the - // ACTIVE_DESCENDANT_CHANGED event. - // Also, we should check if STATE_CHANGED/FOCUSED is really necessary: finally, the children are - // transient, aren't they? - } + m_pImpl->commitCellEvent( AccessibleEventId::STATE_CHANGED, + Any( AccessibleStateType::FOCUSED ), + Any() + ); + // Huh? What the heck? Why do we unconditionally notify a STATE_CHANGE/FOCUSED after each and every + // (handled) key stroke? + + m_pImpl->commitTableEvent( AccessibleEventId::ACTIVE_DESCENDANT_CHANGED, + Any(), + Any() + ); + // ditto: Why do we notify this unconditionally? We should find the right place to notify the + // ACTIVE_DESCENDANT_CHANGED event. + // Also, we should check if STATE_CHANGED/FOCUSED is really necessary: finally, the children are + // transient, aren't they? } } @@ -496,16 +493,14 @@ namespace svt::table } } - void TableControl::commitCellEventIfAccessibleAlive( sal_Int16 const i_eventID, const Any& i_newValue, const Any& i_oldValue ) + void TableControl::commitCellEvent(sal_Int16 const i_eventID, const Any& i_newValue, const Any& i_oldValue) { - if ( m_pImpl->isAccessibleAlive() ) - m_pImpl->commitCellEvent( i_eventID, i_newValue, i_oldValue ); + m_pImpl->commitCellEvent( i_eventID, i_newValue, i_oldValue ); } - void TableControl::commitTableEventIfAccessibleAlive( sal_Int16 const i_eventID, const Any& i_newValue, const Any& i_oldValue ) + void TableControl::commitTableEvent(sal_Int16 const i_eventID, const Any& i_newValue, const Any& i_oldValue) { - if ( m_pImpl->isAccessibleAlive() ) - m_pImpl->commitTableEvent( i_eventID, i_newValue, i_oldValue ); + m_pImpl->commitTableEvent( i_eventID, i_newValue, i_oldValue ); } bool TableControl::HasRowHeader() @@ -601,15 +596,11 @@ namespace svt::table void TableControl::Select() { ImplCallEventListenersAndHandler( VclEventId::TableRowSelect, nullptr ); + m_pImpl->commitAccessibleEvent( AccessibleEventId::SELECTION_CHANGED ); - if ( m_pImpl->isAccessibleAlive() ) - { - m_pImpl->commitAccessibleEvent( AccessibleEventId::SELECTION_CHANGED ); - - m_pImpl->commitTableEvent( AccessibleEventId::ACTIVE_DESCENDANT_CHANGED, Any(), Any() ); - // TODO: why do we notify this when the *selection* changed? Shouldn't we find a better place for this, - // actually, when the active descendant, i.e. the current cell, *really* changed? - } + m_pImpl->commitTableEvent( AccessibleEventId::ACTIVE_DESCENDANT_CHANGED, Any(), Any() ); + // TODO: why do we notify this when the *selection* changed? Shouldn't we find a better place for this, + // actually, when the active descendant, i.e. the current cell, *really* changed? } TableCell TableControl::hitTest(const Point& rPoint) const diff --git a/toolkit/source/controls/table/tablecontrol_impl.hxx b/toolkit/source/controls/table/tablecontrol_impl.hxx index cc53602671ac..bef89eb6c747 100644 --- a/toolkit/source/controls/table/tablecontrol_impl.hxx +++ b/toolkit/source/controls/table/tablecontrol_impl.hxx @@ -293,8 +293,6 @@ namespace svt::table getAccessible(vcl::Window& i_parentWindow); void disposeAccessible(); - bool isAccessibleAlive() const; - // ITableModelListener virtual void rowsInserted( RowPos first, RowPos last ) override; virtual void rowsRemoved( RowPos first, RowPos last ) override; @@ -306,6 +304,7 @@ namespace svt::table virtual void tableMetricsChanged() override; private: + bool isAccessibleAlive() const; void impl_commitAccessibleEvent( sal_Int16 const i_eventID, css::uno::Any const & i_newValue
