No longer crash Windows Explorer on Windows 7 when focusing metadata edit fields#10153
Conversation
…under Windows 7. Fixes nvaccess#5337
|
@feerrenrut @LeonarddeR Given the fact that this crash is quite serious would it be possible to have it reviewed, and included in 2019.3? |
| return windowText | ||
|
|
||
|
|
||
| class metadataEditField(RichEdit50): |
There was a problem hiding this comment.
Note that class names start in upper case
| class metadataEditField(RichEdit50): | |
| class MetadataEditField(RichEdit50): |
There was a problem hiding this comment.
Mistakes like this are perfect candidates for automatic detection by linter. Would it be possible to implement this somehow?
| By default these fields would use ITextDocumentTextInfo , | ||
| but to avoid Windows Explorer crashes we need to use EditTextInfo here. """ | ||
| def _get_TextInfo(self): | ||
| return EditTextInfo |
There was a problem hiding this comment.
| return EditTextInfo | |
| if ((winVersion.winVersion.major, winVersion.winVersion.minor) == (6, 1)): | |
| cls.TextInfo = EditTextInfo | |
| else: | |
| cls.TextInfo = super().TextInfo | |
| return cls.TextInfo |
| """ Used for metadata edit fields in Windows Explorer in Windows 7. | ||
| By default these fields would use ITextDocumentTextInfo , | ||
| but to avoid Windows Explorer crashes we need to use EditTextInfo here. """ | ||
| def _get_TextInfo(self): |
There was a problem hiding this comment.
I think this is a perfect case to benefit from magic class properties:
| def _get_TextInfo(self): | |
| @classmethod | |
| def _get_TextInfo(cls): |
See also the suggestion for the code below. This ensures that:
- The winVersion check is performed only once, rather than multiple times
- The appropriate TextInfo class is cached for the lifetime of the nVDA instance.
| return # Optimization: return early to avoid comparing class names and roles that will never match. | ||
|
|
||
| if( | ||
| (winVersion.winVersion.major, winVersion.winVersion.minor) == (6, 1) |
There was a problem hiding this comment.
I think I prefer the MetadataEditField overlay to be added to all these fields.
Having said that, have you come across cases wherhe RichEdit50W is used as the window class for edit boxes in Explorer that aren't metadata edit fields? How about edit fields in the properties window of files, for example?
There was a problem hiding this comment.
Are you suggesting to use editTextInfo for all RichEdits in Win Explorer regardless of the Windows version? I am personally against it because Windows Explorer is also used in Control Panel, and it isn't realistic to check all edit fields there under every version of Windows. I personally think that the check implemented at present might not be enough, however I haven't found any other negatively affected fields under Win 7. The main priority when bug fixing for me is not to introduce regression so better to be safe than sorry.
LeonarddeR
left a comment
There was a problem hiding this comment.
I'm sorry, should have been a request for changes from the start.
|
Although I cannot get it to do it here on windows 7, might I suggest that
this and the mouse crashing explorer fix are also rolled out into a 2019.2.1
release. It would make windows 7 a lot more stable for machines where both
blind and sighted people are using the same machine.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal E-mail to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
Newsgroup monitored: alt.comp.blind-users
-----
|
|
Okay, all these fields have common windowControlID, so we can match on this. |
|
@LeonarddeR all your requests are now addressed. I am now matching on windowControlID when choosing classes, and performing more expensive check for WIndows Version at the class level. |
LeonarddeR
left a comment
There was a problem hiding this comment.
Thanks for applying the changes @lukaszgo1
|
@LeonarddeR Thanks for approving. I've looked at the dev info for these controls one more time, and it turns out that the full displayed text of these is available as its value. I've tried to overwrite _get_windowText and return content of value there to have best of both words i.e. no longer crash Win Explorer, and have content of these fields readable but it didn't work. Any ideas how to make it working? It shouldn't block this from being merged but would be nice to have. |
|
I think the value is actually retrieved from _get_windowText already.
Why would you like to do this? I think using EditTextInfo is the perfect
solution to this issue.
|
|
As I said in the pr description when EditTextInfo is used for these fields only part of the actual content of these is readable. However entire content is available as a value, so it would be nice to have it accessible as well. Note that windowText and value differs in these cases. |
|
Mm, I'm sorry, I missed this.
However, I'm afraid that it is difficult to work around this, as
EditTextInfo relies on windowText a lot.
|
|
Is this pr ready to be merged, or is using value important? If it is just about avoiding the crash, I think taking this as it is is the best way to go. |
|
Pretty sure that using value as window text will break things. I vote for merging as is and if we really need to address this again, we could revisit this by creating a new textInfo that uses value as its source. |
|
Thanks for merging.
How hard it would be in your opinion? |
|
A 2019.2.1 version of NVDA is on the way. I believe it has been requested that this pr would be included in that as well, see #10153 (comment). However, this issue has been there for ages and I don't think it is that severe to go into a point release. Thoughts @michaelDCurran? |
|
As the crash was not directly because NVDA was doing something wrong,
and because we don't entirely understand why the solution actually
works, I do not think this should be in 2019.2.1.
|
|
Leonard de Ruijter wrote:
A 2019.2.1 version of NVDA is on the way. I believe it has been requested that this pr would be included in that as well, see #10153 (comment). However,
this issue has been there for ages and I don't think it is that severe to go into a point release. Thoughts @michaelDCurran?
I'm not him, but I have a thought: if we have a solution, what is the harm in
including it? It seems a bit unnecessarily dismissive of Win 7 users to fail to
include it, if they've been asking for it (they have), and we have it finally,
but decline to include it because of...Why exactly not?
I'm not sure why a bugfix has to be "big enough" to be included in a
release of any kind, point or otherwise.
Imo Bugs should be fixed if solutions exist, in every release that occurs after
the fix is found, unless the fix breaks something else for that class of release.
|
|
Every fix for a bug can introduce a new one. In history, point releases
were made to fix regressions introduced in the major reelease for which
it was a point release.
|
|
A few thoughts: |
|
@lukaszgo1 Alright. If you can create a new pr against nvaccess/beta, and test that it still fixes the issue, then I will review it. I have a feeling that this change might be a bit too complex to simply be rebased onto beta, though I'm not sure. It does however mean that we will release a 2019.2.1beta1 and ask for feedback over a longer period of time to ensure this does not cause any other regressions. But other than this and possibly reverting liblouis to 3.9, no more changes will be considered for 2019.2.1. |
|
@michaelDCurran I've created #10234 |
Link to issue number:
Fixes #5337
Summary of the issue:
In 89dd4b7 we started using ITextDocumentTextInfo for RichEdit on Windows 7 and above to fix some braille bugs. This however causes crashes in metadata fields in Windows Explorer under Windows 7.
Description of how this pull request fixes the issue:
For these particular edit fields EditTextInfo is used instead.
Testing performed:
Focused some mp3 files with tags, and an xlsx file with author and title. Tabbed to the metadata fields, ensured that the crash is gone.
Known issues with pull request:
Content of these edit boxes isn't always accessible, Even if it would be these controls aren't keyboard navigable so it isn't a big loss.
Change log entry:
Section: Bug fixes
Windows Explorer on Windows 7 would no longer crash when accessing metadata edit fields.