Remove need for hashing NVDAObject and TextInfo objects #9746
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ingEventCounts: NvDAObject and TextInfo instances can no longer be used directly as the key in a dict due to hashing issues in Python3. Rather, the key is now the id (address) of the object.
LeonarddeR
reviewed
Jun 15, 2019
| #: @type: weakref.WeakValueDictionary | ||
| # Theoretically we could use weakref.WeakSet, | ||
| # but as there is no easy way to have NVDAObjects and TextInfos remain hashable in Python3, as they override equality, | ||
| # We store them on a WeakValueDictionary, keyed by their id (address). |
Collaborator
There was a problem hiding this comment.
This comment suggest that we could theoretically use a WeakSet, but that's not going to work as in sets, only hashable types are allowed. Doesn't that mean we theoretically can not use a set?
source/eventHandler.py
Outdated
| _pendingEventCountsByName={} | ||
| # Note that due to the fact it is hard to keep NVDAObjects remaining hashable in Python3 due to their equality method being overridden, | ||
| # the id (address) of the NVDAObject will be used as the key in the dict, not the NVDAObject itself. | ||
| _pendingEventCountsByObj={} |
Collaborator
There was a problem hiding this comment.
I think I prefer this to be renamed to something like:
Suggested change
| _pendingEventCountsByObj={} | |
| _pendingEventCountsByObjId={} |
source/eventHandler.py
Outdated
| # Note that due to the fact it is hard to keep NVDAObjects remaining hashable in Python3 due to their equality method being overridden, | ||
| # the id (address) of the NVDAObject will be used as the key in the dict, not the NVDAObject itself. | ||
| _pendingEventCountsByObj={} | ||
| _pendingEventCountsByNameAndObj={} |
Collaborator
There was a problem hiding this comment.
Similar here:
Suggested change
| _pendingEventCountsByNameAndObj={} | |
| _pendingEventCountsByNameAndObjId={} |
Member
Author
|
Hopefully I made the comment a bit easier to understand?
|
LeonarddeR
approved these changes
Jun 15, 2019
michaelDCurran
added a commit
that referenced
this pull request
Jun 16, 2019
This reverts commit 6ef31d9.
michaelDCurran
added a commit
that referenced
this pull request
Jun 16, 2019
This reverts commit 6ef31d9.
michaelDCurran
added a commit
that referenced
this pull request
Jun 16, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
None
Summary of the issue:
In baseObject.AutoPropertyObject: all instances are tracked on the class by storing them in a WeakKeyDictionary. Tracking of these instances is to allow clearing the proprty cache of all living objects in one go.
In eventHandler: there are several dictionaries that track currently queued events, keyed by eventName, object, or eventName and object.
Storing NvDAObjects and or TextInfo objects in these dictionaries requires these objects to be hashable.
However, in Python3, if an object's equality check is customized (I.e. a custom
__eq__method is provided), the object becomes unhashable, as Python3 requires that any two objects that report being equal must also share the same hash value.If
__eq__is implemented on an object and there is a need for the object to be hashable, one must also implement__hash__on the object (and all it subclasses it seems) as well.As our equality checks are rather complex, and many of them short-circuit for performance reasons, it is next to impossible to provide an optimal
__hash__that will honor the requirement that the hash values of two objects be equal if the objects report equal.Therefore, we should try to remove the need to hash NVDAObjects and TextInfo instances, by no longer using these as keys for dictionaries.
Description of how this pull request fixes the issue:
In baseObject.AutoPropertyObject:
The instance WeakKeyDictionary has been changed to a WeakValueDictionary which holds the instances as its values, keyed by the instance's id (address).
The eventHandler._pendingEventCounts dictionaries no longer use the object as the key directly, rather the id (address) of the object.
In both cases of baseObject and eventHandler, we are only tracking the objects over their lifetime, and we expect to only look up the same physical object (not one that just "looks" like that object), thus using the object's id (address) is perfectly fine.
Testing performed:
TypeError("Unhashable type") is no longer raised when trying to start / use NVDA.
Known issues with pull request:
None.
Change log entry:
None.
Section: New features, Changes, Bug fixes