From 8633139e25f81a35ef135b742eb6ae8b85cd1fb0 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Tue, 20 Aug 2024 16:42:33 +0200 Subject: [PATCH] Accessibility on macOS: clean up reference counting logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify the "populate" functions to always create a new array, rather than trying to reuse the existing array. Use (auto)release to free the old arrays. When creating place holder cells for a new row (where we know which row array to add the cell to), shortcut the code to not find the row array. Don't release cell elements that we got from the cache. Consistently fix the row array size if it's not matching the table's. Assert some more invariants and add logging output. Fixes: QTBUG-122751 Pick-to: 6.7 Change-Id: Ie122a4754f6a3f4c19c5412a7c9e589f2861d4ae Reviewed-by: Tor Arne Vestbø (cherry picked from commit 1f7e926411d1e0d958c37dc597f24101d410c46b) Reviewed-by: Qt Cherry-pick Bot --- .../cocoa/qcocoaaccessibilityelement.mm | 127 +++++++++++------- 1 file changed, 80 insertions(+), 47 deletions(-) diff --git a/src/plugins/platforms/cocoa/qcocoaaccessibilityelement.mm b/src/plugins/platforms/cocoa/qcocoaaccessibilityelement.mm index b319dd072e8..a07c9fef3e2 100644 --- a/src/plugins/platforms/cocoa/qcocoaaccessibilityelement.mm +++ b/src/plugins/platforms/cocoa/qcocoaaccessibilityelement.mm @@ -125,6 +125,10 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of if (iface->tableInterface()) { [self updateTableModel]; } else if (const auto *cell = iface->tableCellInterface()) { + // Called with role when populating a row with placeholder cells. The caller + // inserts us into the correct array, so we don't have to look for it. + if (role == NSAccessibilityCellRole) + return self; // If we create an element for a table cell, initialize it with row/column // and insert it into the corresponding row's columns array. m_rowIndex = cell->rowIndex(); @@ -156,8 +160,12 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of << "Table representation column count is out of sync:" << rowElement->columns.count << "!=" << tableInterface->columnCount(); } - rowElement->columns = [rowElement populateTableRow:rowElement->columns - count:tableInterface->columnCount()]; + if (rowElement->columns) { + [rowElement->columns autorelease]; + rowElement->columns = nil; + } + rowElement->columns = [rowElement populateTableRow:tableInterface->columnCount()]; + [rowElement->columns retain]; } qCDebug(lcAccessibilityTable) << "Creating cell representation for" @@ -203,10 +211,17 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of return [self elementWithId:anId]; } +// called by QAccessibleCache::removeAccessibleElement - (void)invalidate { axid = 0; - rows = nil; - columns = nil; + if (rows) { + [rows autorelease]; + rows = nil; + } + if (columns) { + [columns autorelease]; + columns = nil; + } synthesizedRole = nil; NSAccessibilityPostNotification(self, NSAccessibilityUIElementDestroyedNotification); @@ -238,19 +253,10 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of return synthesizedRole != nil; } -- (NSMutableArray *)populateTableArray:(NSMutableArray *)array role:(NSAccessibilityRole)role count:(int)count +- (NSMutableArray *)populateTableArray:(NSAccessibilityRole)role count:(int)count { if (QAccessibleInterface *iface = self.qtInterface) { - if (array && int(array.count) != count) { - [array release]; - array = nil; - } - if (!array) { - array = [NSMutableArray arrayWithCapacity:count]; - [array retain]; - } else { - [array removeAllObjects]; - } + auto *array = [NSMutableArray arrayWithCapacity:count]; Q_ASSERT(array); for (int n = 0; n < count; ++n) { // columns will have same axid as table (but not inserted in cache) @@ -272,37 +278,33 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of return nil; } -- (NSMutableArray *)populateTableRow:(NSMutableArray *)array count:(int)count +- (NSMutableArray *)populateTableRow:(int)count { Q_ASSERT(synthesizedRole == NSAccessibilityRowRole); - if (array && int(array.count) != count) { - [array release]; - array = nil; - } + qCDebug(lcAccessibilityTable) << "Populating table row" << m_rowIndex + << "with" << count << "placeholder cells"; + // When macOS asks for the children of a row, then we populate the row's column + // array with synthetic elements as place holders. This way, we don't have to + // create QAccessibleInterfaces for every cell before they are really needed. + // We don't add those synthetic elements into the cache, and we give them the + // same axid as the table. This way, we can get easily to the table, and from + // there to the QAccessibleInterface for the cell, when we have to eventually + // associate such an interface with the element (at which point it is no longer + // a placeholder). + auto *array = [NSMutableArray arrayWithCapacity:count]; + Q_ASSERT(array); - if (!array) { - array = [NSMutableArray arrayWithCapacity:count]; - [array retain]; - // When macOS asks for the children of a row, then we populate the row's column - // array with synthetic elements as place holders. This way, we don't have to - // create QAccessibleInterfaces for every cell before they are really needed. - // We don't add those synthetic elements into the cache, and we give them the - // same axid as the table. This way, we can get easily to the table, and from - // there to the QAccessibleInterface for the cell, when we have to eventually - // associate such an interface with the element (at which point it is no longer - // a placeholder). - for (int n = 0; n < count; ++n) { - // columns will have same axid as table (but not inserted in cache) - QMacAccessibilityElement *cell = - [[QMacAccessibilityElement alloc] initWithId:axid role:NSAccessibilityCellRole]; - if (cell) { - cell->m_rowIndex = m_rowIndex; - cell->m_columnIndex = n; - [array addObject:cell]; - } + for (int n = 0; n < count; ++n) { + // columns will have same axid as table (but not inserted in cache) + QMacAccessibilityElement *cell = + [[QMacAccessibilityElement alloc] initWithId:axid role:NSAccessibilityCellRole]; + if (cell) { + cell->m_rowIndex = m_rowIndex; + cell->m_columnIndex = n; + [array addObject:cell]; + [cell release]; } } - Q_ASSERT(array); return array; } @@ -313,8 +315,18 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of Q_ASSERT(!self.isManagedByParent); qCDebug(lcAccessibilityTable) << "Updating table representation with" << table->rowCount() << table->columnCount(); - rows = [self populateTableArray:rows role:NSAccessibilityRowRole count:table->rowCount()]; - columns = [self populateTableArray:columns role:NSAccessibilityColumnRole count:table->columnCount()]; + if (rows) { + [rows autorelease]; + rows = nil; + } + rows = [self populateTableArray:NSAccessibilityRowRole count:table->rowCount()]; + [rows retain]; + if (columns) { + [columns autorelease]; + columns = nil; + } + columns = [self populateTableArray:NSAccessibilityColumnRole count:table->columnCount()]; + [columns retain]; } } } @@ -350,7 +362,6 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of if (cellElement != self) { // for the same cell position Q_ASSERT(cellElement->m_rowIndex == m_rowIndex && cellElement->m_columnIndex == m_columnIndex); - [cellElement release]; } } @@ -467,8 +478,16 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of // axid matches the parent table axid so that we can easily find the parent table // children of row are cell/any items Q_ASSERT(m_rowIndex >= 0); - const int numColumns = table->columnCount(); - columns = [self populateTableRow:columns count:numColumns]; + Q_ASSERT(rows == nil); + const unsigned int numColumns = table->columnCount(); + if (!columns || columns.count != numColumns) { + if (columns) { + [columns autorelease]; + columns = nil; + } + columns = [self populateTableRow:numColumns]; + [columns retain]; + } return NSAccessibilityUnignoredChildren(columns); } } @@ -995,13 +1014,27 @@ static void convertLineOffset(QAccessibleTextInterface *text, int *line, int *of - (NSArray *) accessibilityRows { if (!synthesizedRole && rows) { QAccessibleInterface *iface = self.qtInterface; - if (iface && iface->tableInterface()) + QAccessibleTableInterface *tableInterface = iface ? iface->tableInterface() : nullptr; + if (tableInterface) { + const unsigned int rowCount = tableInterface->rowCount(); + if (rows.count != rowCount) { + qCDebug(lcAccessibilityTable) << "Updating table rows with" << rowCount << "rows"; + if (rows) { + [rows autorelease]; + rows = nil; + } + rows = [self populateTableArray:NSAccessibilityRowRole + count:rowCount]; + [rows retain]; + } return NSAccessibilityUnignoredChildren(rows); + } } return nil; } - (NSArray *) accessibilityColumns { + // we only implement this for a table, not for rows if (!synthesizedRole && columns) { QAccessibleInterface *iface = self.qtInterface; if (iface && iface->tableInterface())