From 6c5257b680044514debc1dde1267ab985ef4548c Mon Sep 17 00:00:00 2001 From: Michael Weghorn Date: Tue, 22 Mar 2022 09:06:26 +0100 Subject: [PATCH] wina11y: Slightly clean up WinResIDGenerator Changes: * drop the optional `maxNum` param for the constructor, it's never set to anything different than the default value * store negative number in member (renamed from `max` to `m_nMin`) instead of storing positive number and inverting that when returning it in `ResIDGenerator::GenerateNewResID` * Use -1 as the first resource ID instead of -2. Also, add a comment that negative child IDs are used because it's common to use such ones to indicate unique resource IDs in IAccessible2. Quoting James Teh's comment on an NVDA pull request of mine [1]: > The use of negative child ids doesn't fit well into the IAccessible > spec, but it has been done by IAccessible2 for a very long time and > should be considered standard for all intents and purposes. A negative > child id should be treated as a unique id, while a positive child id > should be treated as a child index. > > That said, as noted in #13277 (comment), IAccessible2 elements are > always full IAccessible objects, not "simple elements". Thus, anything > that returns an accessible (including accSelection) really should return > an object pointer. In the case of accSelection, this means VT_DISPATCH > for a single selection or VT_UNKNOWN and iEnumVARIANT (with VT_DISPATCH > elements) for multiple selection. > > In short, NVDA supporting negative child ids returned from accSelection > isn't necessarily "wrong", but ideally, LibreOffice would be fixed to > return full objects. The latter (returning full objects) has already been addressed in commit 00c0ee8cf0fac0c933c5ae600e99a64b1c7d4397 Author: Michael Weghorn Date: Mon Jan 31 07:41:14 2022 +0000 tdf#147083 wina11y: Return a11y object instead of child ID but a unique ID is still relevant, s. e.g. the documentation of `IAccessible2::uniqueID` [2], which also mentions potential ways to implement unique IDs: > One means of implementing this would be to create a factory with a 32 > bit number generator and a reuse pool. The number generator would emit > numbers starting at 1. Each time an object's life cycle ended, its > number would be saved into a reuse pool. The number generator would be > used whenever the reuse pool was empty. > > Another way to create a unique ID is to generate it from a pointer > value, e.g. an object's address. That would be unique because no two > active objects can use the same allocated memory space. The first approach is what the LO implementation currently does, except that negative numbers are used. [1] https://github.com/nvaccess/nvda/pull/13277#issuecomment-1024622871 [2] https://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_accessible2.html#aac1342376cf195a6a6e9ee3b7e30ae3b Change-Id: I6c0a6c4a4e3a69396205fe2d69cd66af6525a273 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/131927 Tested-by: Jenkins Reviewed-by: Michael Weghorn --- winaccessibility/inc/ResIDGenerator.hxx | 10 ++++++---- winaccessibility/source/service/ResIDGenerator.cxx | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/winaccessibility/inc/ResIDGenerator.hxx b/winaccessibility/inc/ResIDGenerator.hxx index 19e06c9b88ad..400f58a2e620 100644 --- a/winaccessibility/inc/ResIDGenerator.hxx +++ b/winaccessibility/inc/ResIDGenerator.hxx @@ -19,20 +19,22 @@ #pragma once -#define PRIMARY_RESID 0x00000001 #include //ResID i.e. MSAA child ID, //this class is responsible for generating a child ID +// +// In IAccessible2, negative child IDs are commonly used to +// indicate unique resource IDs. class ResIDGenerator { private: - long max; + long m_nMin; std::deque subList; public: - ResIDGenerator(long maxNum = PRIMARY_RESID) - : max(maxNum) + ResIDGenerator() + : m_nMin(-1) { } ~ResIDGenerator(); diff --git a/winaccessibility/source/service/ResIDGenerator.cxx b/winaccessibility/source/service/ResIDGenerator.cxx index 6c88a12a402c..7498380300c7 100644 --- a/winaccessibility/source/service/ResIDGenerator.cxx +++ b/winaccessibility/source/service/ResIDGenerator.cxx @@ -25,7 +25,7 @@ ResIDGenerator::~ResIDGenerator() {} /** * SubList stores those IDs that were ever generated and deleted, the method - * return the ID from subList first if subList is not empty,else return ++max. + * return the ID from subList first if subList is not empty, else return m_nMin--. * Add the obsolete IDs by calling SetSub method * * @param @@ -39,8 +39,8 @@ long ResIDGenerator::GenerateNewResID() subList.pop_front(); return nRes; } - assert(max < LONG_MAX); - return -(++max); + assert(m_nMin > LONG_MIN); + return m_nMin--; } /* vim:set shiftwidth=4 softtabstop=4 expandtab: */