Ensure states/roles are unique

[feerrenrut]

Note this is an API breaking change.

Link to issue number:

None

Summary of the issue:

Values in the State/Role enums are getting large, it is easy for a developer to accidentally duplicate a value.

NVDA states were initially defined as powers of 2 so that the states property on NVDAObject could return multiple (bit flag) values within an integer. However, for a long time now, the states property returns a Python set. The values no longer have to set a unique bit.

Also, now that Role/State constants are enums, we no longer depend on manually
matching values printed in logging with the values in the file (because
enums are named).

Description of how this pull request fixes the issue:

• Decorate Role/State enum classes with unique.
• Prevent errors manually assigning a value by using auto.

Testing strategy:

System tests
Usage on alpha

Known issues with pull request:

This not strictly a required API breaking change, however add-ons should not be relying on the explicit values.

Change log entries:

For Developers

- The ``controlTypes.Role`` and ``controlTypes.State`` enum values are no longer fixed/hardcoded and have changed.
   The ``controlTypes.State``  members are no longer bit flags, states should not be determined using bitwise operations.
   There are mappings for IAccessible, IAccessible2 and UIA:
  - IAccessibleHandler.IAccessible2StatesToNVDAStates
  - IAccessibleHandler.IAccessibleStatesToNVDAStates
  - IAccessibleHandler.IAccessibleRolesToNVDARoles
  - UIAHandler.UIAControlTypesToNVDARoles
  -

Code Review Checklist:

• Pull Request description:
  • description is up to date
  • change log entries
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• API is compatible with existing add-ons.
• Documentation:
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English

[michaelDCurran]

The change looks okay, though as discussed, we can not be fully confident that there is no code in the wild that relied on the exact value of these constants.
I also feel that the PR description needs a slight correction:
NVDA roles and states were never originally written such that their values mirrored MSAA roles and states. Rather, NVDA states were defined as powers of 2 because the original states property on NVDAObjects returned 1 or more state values bit-wise ored together. But for a long time now the states property has returned a Python set, so this is no longer a necessary restriction.

[CyrilleB79]

The change looks okay, though as discussed, we can not be fully confident that there is no code in the wild that relied on the exact value of these constants.

For information, I have already seen add-ons or scratchpad scripts (globalPlugins/appModules) with hard-coded numeric values of role or state. Since NVDA 2022.1 is an API-breaking release, it should not be a problem however and it's up to add-on authors to update their code.
Or did I miss something?

[feerrenrut]

Or did I miss something?

@CyrilleB79 you are correct. Our only reservation is how late this is in the dev-cycle.

I have already seen add-ons or scratchpad scripts (globalPlugins/appModules) with hard-coded numeric values of role or state

Do you have any examples I could look at?

[michaelDCurran]

@feerrenrut I'm happy with the pr description now.

[CyrilleB79]

I have already seen add-ons or scratchpad scripts (globalPlugins/appModules) with hard-coded numeric values of role or state

Do you have any examples I could look at?

Yes: at least in Thunderbird+ 3.3 downloadable on this page. This is a popular extension in French community.
But again, the authors are active and will update their code if needed.

[CyrilleB79]

Just to clarify my previous comment:

I have seen examples where numeric values were used for role or state. However, I do not remember any situation where bitwise operations were performed among them. Probably that's what you were looking for actually.

Sorry for the confusion.

[feerrenrut]

Thanks @seanbudd, I'll update the description with that suggestion.

Thanks for the examples @CyrilleB79, I was just curious about how the values are used, and how widely. I'll have a look at that add-on. If it will be difficult for add-ons to update, we may reconsider this PR.

If there is any technical reason why hardcoded values would be used rather than importing, or difficulty with development using imported values, that would also be good to know.

[CyrilleB79]

Cc @abdel792 if you may comment on the use of numeric values for roles in Thunderbird+ add-on.
Also, I do not know if Pierre-Louis has a GitHub account; could you please cc him if he has? Thanks.

[seanbudd]

Thanks @feerrenrut

[feerrenrut]

@CyrilleB79 I've had a look through that add-on (Thunderbird+-3.3-2022-01-24) but didn't find hard coded state constant values. Could you point any out to me?

[feerrenrut]

@michaelDCurran and @seanbudd, if you both agree, I think we can merge this in to get wider testing / feedback, accepting the risk of having to revert.

[michaelDCurran]

Merging this is fine with me.

[CyrilleB79]

@CyrilleB79 I've had a look through that add-on (Thunderbird+-3.3-2022-01-24) but didn't find hard coded state constant values. Could you point any out to me?

You can have a look at the event_foreground( function. I have pasted it below for convenience.

	def event_foreground(self, obj,nextHandler):
		role = obj.role
		#message("foreground, tite " + str(obj.name))
		#beep(120, 20)
		#z.mtlog.addprops(obj, "event_foreGround debut :")
		if role == 4 : # role dialog
			#message("foreground Dialogue")
			utilTB2.curFrame = utilTB.getDialogID(obj)
			#message(utilTB2.curFrame)
		elif role == 34 and self.appReady :
			#beep(150,30)
			#z.mtlog.addprops(obj, "event_foreground, appel de getFrameID quand role==34")
			utilTB2.curFrame = utilTB.getFrameID(obj)
			if  utilTB2.curFrame == "messengerWindow" :
				utilTB2.curTab = utilTB2.getCurTab2(obj, True, self, "event foreground")
			elif utilTB2.curFrame == "msgcomposeWindow" :
				utilTB2.curTab = "compose-no-tabs"
				#beep(250, 30)
				#fo = api.getFocusObject()
				#z.mtlog.addprops(fo, "event_foreground, "" compose, focusObject : ")
			else :
				utilTB2.curTab = "no-tabs"
			#z.mtlog.add("event foreGround FrameID : " + str(utilTB2.curFrame) + ", curTab : " + str(utilTB2.curTab) + ", role : " + str(obj.role))
		nextHandler()
	

[feerrenrut]

Thanks @CyrilleB79, I see. At least this should be a direct replacement with the enum.

[michaelDCurran]

This pr causes issue #13457
We no longer announce if a cell in Excel has a formula as the in-process state constants no longer match the controlTypes states constants.
I would strongly suggest we revert this pr, at least for NVDA 2022.1 until we can work out a way to have both the in-proc and NvDA core agree on the state constants.

[seanbudd]

While not an ideal solution, using IntFlag for states should cause these to match again. However, we run the risk of them breaking again unless we can figure out how to synchronise these.

[lukaszgo1]

Has it been considered to no longer expose these states from the inprocess code as an long - rather add boolean's to the structure for each state? That seems much safer than relying on an auto-generated enum value.

[feerrenrut]

This should be fixed via #13465

@lukaszgo1, it's a potential option, but has it's own downs sides:

• ABI compatibility breaks every time a new constant is added (requiring developers / testers / users) to exit application / sign out / restart when updating)
• Developers still need to make a change to the in-proc code, and the python code.

In #13465 I have separated the ABI for excel in-proc from general NVDA States, similar to IAccessible and IAccessible2 states, there is now an excelCellStates mapping to NVDA States. This moves the coupling from controlTypes and excel in-proc code, to excel in-proc and excel python code.

[lukaszgo1]

@feerrenrut
I'm not sure i follow the below arguments. Note that I'm by no means trying to argue that what I've proposed is better its more that you way more experienced than I am, I've learned a lot from you over the years, and therefore I'm probably missing something obvious here.

* ABI compatibility breaks every time a new constant is added (requiring developers / testers / users) to exit application / sign out / restart when updating)

That should not be an issue - nVDA stores the in process libraries in the version specific directory so each time new build of NVDA is started the new set of libraries is freshly injected into Excel.

* Developers still need to make a change to the in-proc code, and the python code.

Isn't it the same for the solution that you've proposed in #13465? A new state for an Excell cell still requires a new state being added into enum both in Python and in C++.

Using booleans rather than Bitwise operations would result in much more readable code. Obviously they still should be decoupled from the actual values of an enum members in controlTypes, but if this would be done with boolean's we just could create a dict where keys are names of the structure members and actual values from controlTypes are values. It is also very likely that I'm just allergic to using bitwise operations where not strictly necessary in a high level language such as Python.

[abdel792]

Hi @CyrilleB79 and all,

In fact, the Thunderbird+ add-on has been around for a long time, and several people contributed to it.

I'd proposed my help to Pierre-Louis only for some additional features he needed.

The use of numeric values for roles had been introduced by other contributors, who preceded my work on this addon.

I didn't modify their code, although I could see there was some mismatch with the coding rules for NVDA.

There were far too many instructions to correct, and I didn't have too much free time.

If you open Windows Notepad and then NVDA's Python console, a statement like the following should show True:

focus.role == 8

I think that's what you want to talk about?

Thanks.

[feerrenrut]

That should not be an issue - nVDA stores the in process libraries in the version specific directory so each time new build of NVDA is started the new set of libraries is freshly injected into Excel.

Yes, that is theoretically true. In practice we find that the library doesn't always get unloaded. The only way to remedy this is to restart the application, or sometimes it even requires logging off.
It's not often a new state is added, so this isn't really a big concern.

It's really important that python and c++ agree on the layout of the struct, adding booleans for each state makes that less likely. A mistake when adding a new state could result in crashes or some other adjacent field being very wrong, vs a mistake when adding a bit field value means that only the newly added state is not reported. Given this is the thing the developer is working on they should notice a missing state, but might not notice that all of a sudden the comment is no longer reported when the cell has a formula (as an example).

Isn't it the same for the solution that you've proposed in #13465? A new state for an Excell cell still requires a new state being added into enum both in Python and in C++.

Yes, my point there was that it is the same. IE no advantage either way in terms of the number of things the developer has to remember.

Using booleans rather than Bitwise operations would result in much more readable code.

This might be a matter of style / personal preference, consider the code that has to convert the booleans into a list of states:

cellInfo=self.excelCellInfo
cellStates = []
if cellInfo.hasFormula:
    cellStates.append(controlTypes.HASFORMULA)
elif cellInfo.hasComment:
    cellStates.append(controlTypes.HASCOMMENT)
elif ...