Hi,

Before reviewing this: is this working on Windows 7, 8.1, and one or more earlier iterations of Windows 10? Thanks.

Presumably, as the original patch was written for Windows 7 in 2015.

I can confirm, that its working on Windows 7.

Hi,

Test results: it is at least working in Windows 10, Server 2012 R2 (server version of Windows 8.1), and Windows 7.

Thanks.

Hi,

One important thing: do you know if other Microsoft Management Console (MMC) tables must be supported? If yes, I'd leave it as is. But if the intention is to support Disk Management/graphical view only, you might want to ask NVDA to scrutinize this more - checking for window class names, for instance.

Thanks.

A few general commends if i may;

1. You should convert line endings from Machintosh to Windows. While not required, it is requested in the contributing guide.
2. It isn't working particularly well for a braille users, because of using ui.message. One solution which comes to my mint, (however better solution might exist) is to execute gainFocus event on the selected object.
3. As @josephsl pointed above checking only role isn't sufficient. Using windowText of this particular table might be better.
4. In this table arrows aren't only keys changing selection. I cannot recall the details now, but believe, that ad least Page up/down are moving in this table also. Maybe instead of binding to the specific key we should overwrite kePressed event on the class level.
5. When focus moves to the table the already selected item should be announced.

Working on review items.

I'm not sure of any other role_TABLE controls in the MMC, but if this patch can improve their accessibility I don't see the issue (unless, of course, it breaks more than it fixes).

I specifically used ui.message since it seems to report in both speech and Braille, but I'm new to NVDA development so might be doing it wrong. I replaced the ui.message call with eventHandler.executeEvent("gainFocus", i), but the experience (at least in speech) has significantly degraded. Is there anything else I should be doing?

I can't find a kePressedEvent (or keyPressedEvent) anywhere. Do I just create a method event_kePressed(self) in the MMCTable class to catch these? Will it only catch keystrokes when the table has focus?

Hi,

There might be better ways of handling focus changes with various commands.

How about this: why don't we table this pull request and get you started on writing an add-on for MMC? That way, it won't affect the user experience - it can be remedied easily by editing an app module inside an add-on and can be disabled if needed. Also, with an add-on approach, you can experiment with other portions of MMC, and also could add your name to a list of add-on authors.

I generally direct people to add-on route first as it gives them something they can experiment with easily without breaking NVDA too much. Once they understand some workings of NVDA and are more comfortable with NVDA codebase, then I encourage folks to think about submitting a pull request whenever they are ready. This is how some recent NVDA features came about (emoji panel support and settings panel work, to name a few).

It is also possible for folks to work the other way (NVDA Core first, followed by writing add-ons). The advantage of this way is that people know what to do in specialized situations and can apply NVDA Core ideas inside their add-ons. Sometimes, while people learn NVDA Core development, they'd venture into add-ons world at the same time (or vice versa), which also gives them a knowledge base to refer to.

What do you think?

Thanks.

1. What sorts of commands do you suggest for handling focus changes?
2. I understand that this needs wider testing (particularly with other MMC snap-ins), but I'd like to see this app module merged into core eventually. What does the transition process from add-on to core feature generally look like? Who should test the new app module? For how long?

Hi,

Focus announcement/handling: you may need to read MMC documentation to see which commands do what (not just arrow keys, but others as well).

As for add-on to Core, it depends. Ideally it should be tested by many people (not just us three, but by other IT professionals, system admins, end users, and so many others). The duration really doesn't matter (as in you don't have a set deadline) - what's more important is having willingness to incorporate feedback from people as they come in.

For example, support for the emoji panel (which is being widely discussed at the moment) was originally part of Windows 10 App Essentials add-on (a brain child of mine). I began working on emoji panel support the day a Windows Insider Preview build with this feature shipped (build 16215, came out in June 2017). Initially, I considered using a global plugin method to support emoji panel, but @jcsteh suggested writing an app module, and subsequent investigations confirmed his theory. I then spent summer of 2017 writing basic support for emoji panel, then I released a version of Windows 10 App Essentials add-on with this feature included in order to gather feedback. Based on feedback I got, I rewrote parts of this code throughout 2017 and 2018 to acknowledge feedback and to react to changes in Windows 10, releasing new add-on releases regularly so people can test changes.

In early 2018, NV Access people showed an interest in incorporating emoji panel support into NVDA based on my add-on work. After several code review passes, the emoji panel code was eventually included in NVDA master branch in July 2018, and made its debut as part of NVDA 2018.3. But my work is not done - there is a pull request that updates emoji panel support code to handle what's coming in Windows 10 Version 19H1 (in a few months), and I have requested reviewers to wait until 19H1 is showing signs of stability before reviewing and merging the code into NVDA.

The most important thing to keep in mind is not deadline or when it is okay to send a pull request - it is listening to feedback and acting accordingly. Your source code is a form of communication, and having willingness to listen to comments and applying in code says a lot about code quality. Also, always remember that what you submit via pull requests, and the code that gets eventually merged into the master branch is a testament to how impactful and precious your code is - the impact alone is the reason why some reviewers set a high bar for NVDA Core pull requests as opposed to add-ons (at this time; personally, I value add-on writing and reviews as just as important as pull requests for NVDA). Note that what I'm saying should not be taken as discouraging people - I'm writing this to help you understand what you are about to get into.

Hope this helps.

@codeofdusk wrote:

I'm not sure of any other role_TABLE controls in the MMC, but if this patch can improve their accessibility I don't see the issue (unless, of course, it breaks more than it fixes).

Maybe I am overcautious, but we cannot be sure about this, unless someone reports the breakage of course. Better to be safe than sorry in my opinion.

I specifically used ui.message since it seems to report in both speech and Braille, but I'm new to NVDA development so might be doing it wrong. I replaced the ui.message call with eventHandler.executeEvent("gainFocus", i), but the experience (at least in speech) has significantly degraded. Is there anything else I should be doing?

The problem with ui.message is the fact, that it shows in braille only for a 4 seconds by default. It can be changed of course, but user must know about it. Furthermore when executing gainFocus the actually selected control would be reported when user presses NVDA+Tab or NVDA+up arrow. Executing this event alone wouldn't be sufficient, because it simply moves focus to the control. This makes selected object an active control not the table, so the child of the control aren't what we are interested in. In this case I suggest creating second class for the table child monitor for the key presses there also, and when the key is pressed move focus back to the table, and start looking for selected child then.

@LeonarddeR Thanks for looking at this. I agree that overriding _get_value is a better approach than that taken by the 2015 patch my PR was based on. After your suggested changes, NVDA+tab and NVDA+up arrow now correctly report the focused cell, and the currently selected cell is read when I tab to the table. However, the newly-focused cell is no longer announced when I arrow around. I've tried looking at event_setValue, but for me it doesn't appear to fire at all on focus changes. Is there some other event or something else I should be looking at?

Ugh, you're right. Actually, event-valueChange is fired on a totally unrelated vertical scrollbar, not on the table itself.

There is another event, event_selection, called on the object that receives the selection. This turns out to be a pretty strange control after all.

I have an idea to fix this, but it would mean a rewrite of your code. Note that this is not because your code is bad at all, it is just because the control behaves in a complex way.

Oops, my editor turned tabs to spaces. Fixing.

@lukaszgo1 I think all of your comments have been addressed.

@codeofdusk They definitely are. Thanks.

@michaelDCurran should I update what's new on this one, or is that done by the person who merges if it goes to master? Thanks.

PR rebased on master. Thanks.

Any updates on this one?