nvda icon indicating copy to clipboard operation
nvda copied to clipboard

Add feature to restart with both disable add-ons and debug logging enabled from the exit dialog.

Open mzanm opened this issue 2 years ago • 22 comments

Link to issue number:

https://github.com/nvaccess/nvda/issues/11538

Summary of the issue:

It is not possible to restart using the exit dialog with both all add-ons disabled and debug logging enabled. one option must be chosen.

Description of how this pull request fixes the issue:

Move restart with add-ons disabled and restart with debug logging enabled and make them checkboxes to a group box that gets enabled once the restart option from the combo box is selected.

Testing strategy:

Tested restarting with both options checked and one of them checked and in all 3 situations everything works as expected. Also, tested installing a pending update and it installs, no issues. Tested AppX using config.isAppX = True in the console and disable add-ons checkbox is hidden as expected. Restarting with debug logging works.

Known issues with pull request:

I'm not completely sure how the changes look visually.

Change log entries:

For Developers It is now possible to restart NVDA with both Restart with Add-ons disabled and Restart with debug logging enabled at the same time

Code Review Checklist:

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

mzanm avatar Jan 03 '22 20:01 mzanm

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ms43ofoqvhuru5yc/artifacts/output/nvda_snapshot_pr13214-24409,7ff4617b.exe

See test results for failed build of commit 7ff4617ba6

AppVeyorBot avatar Jan 04 '22 11:01 AppVeyorBot

Hello. I looked at the failing system test and it seems it's related to Chrome, I'm not sure how did that happen even though I made no changes to that at all.

mzanm avatar Jan 04 '22 12:01 mzanm

  1. Did you test the Windows store app case and the "pending updates" case? If yes, indicate your test strategy and results. Hello, Yes, I tested the install pending updates options and it does work after your change, I didn't notice that, so thanks. I'm not sure how to build an AppX package for NVDA though, couldn't find how to do that. Sorry about that. If you could let me know how, I'd be able to test.

mzanm avatar Jan 04 '22 12:01 mzanm

Hello @mazen428

I just realize that this is your first PR in NVDA project. Welcome and congratulations!

I'm not sure how to build an AppX package for NVDA though, couldn't find how to do that. Sorry about that. If you could let me know how, I'd be able to test.

I have no experience either in building or using the appX. However, I think that you could just test by entering the following instruction in NVDA console: config.isAppX = True This way, you could simulate the behaviour in this part of the program. Just indicate that you have tested it this way in the test section.

I looked at the failing system test and it seems it's related to Chrome, I'm not sure how did that happen even though I made no changes to that at all.

If that's completely unrelated, I think you can ignore it. There has been issues with Chrome and system tests in the past. @feerrenrut could you confirm this?

When you're done, set this PR as ready for NVAccess to review it. Also let me know so that I can update my review.

CyrilleB79 avatar Jan 04 '22 13:01 CyrilleB79

I wonder whether this change justifies a restructure of the dialog that changes debug logging and add-ons disabled to check boxes.

LeonarddeR avatar Jan 05 '22 10:01 LeonarddeR

Hello. Sorry, I didn't get what you meant exactly, if you mean there should be a fifth option that is restart with add-ons disabled and debug logging enabled in the "What would you like to do?" choice box then I could do that, but I think like that way is better in my opinion because more options can be added in the future like restart selecting add-ons to be enabled for example without the choice box being full of options ETC. Thank you.

mzanm avatar Jan 05 '22 15:01 mzanm

f that's completely unrelated, I think you can ignore it. There has been issues with Chrome and system tests in the past. @feerrenrut could you confirm this?

Yes, occasionally the first chrome test fails. We intend to investigate further when we can prioritize it. For now either ask us to restart the build or push another commit (even empty) to restart it.

feerrenrut avatar Jan 19 '22 04:01 feerrenrut

I wonder whether this change justifies a restructure of the dialog that changes debug logging and add-ons disabled to check boxes.

I'd be cautious about this. This is quite developer-centric. I'm hesitant to draw attention to options (that are hopefully) rarely used by the majority of end users.

feerrenrut avatar Jan 19 '22 04:01 feerrenrut

Ah, in that case, would it be better to add another script, maybe NVDA shift q, to show restart options? Thanks.

mzanm avatar Jan 19 '22 06:01 mzanm

After thinking about this for a while, my preference would be to:

  • Add a "restart NVDA in safe mode" (combining the "with debug logging" and "with addons disabled") to the exit dialog.
  • Add an option to tools, as well as a command with no default gesture, which will open a "advanced" or "developer restart dialog" that has all the options.

This keeps the interface minimal for the majority of users, and still makes it easy to explain to users how to get to the full set of options. My argument for keeping an option for restarting NVDA in "safe mode" is to make it easier and more obvious to users that the first thing they should try when something is going wrong is disable addons and enable debug logging. This hides the details of that.

feerrenrut avatar Jan 19 '22 10:01 feerrenrut

After rereading both the discussion in this PR and comments in #11538 I respectfully disagree with the @feerrenrut 's suggestion since this PR offers UX which is way superior to what has been proposed above. There are several user stories which should be considered here and @feerrenrut's proposal makes each of them worse IMO.

  1. As a user when debugging an issue with NVDA I sometimes need to restart with both add-ons disabled and debug logging enabled when requested by NVDA developers. It is important to allow enabling both of these in one step but at the same time I want to understand what exactly I am enabling.
  2. As an user I sometimes have issues with an add-on and I am being requested by its author to restart both with debug logging and add-ons enabled. It is important that this process is as easy as possible since I'm a beginner.
  3. As an NVDA developer I want to restart with the exact options I need without necessarily remembering command line switches.
  4. As an add-on developer I want to be able to restart with debug logging and add-ons both enabled.
  5. As an existing user I don't want changes to the exiting interface which don't benefit me.
  6. Finally as a new user I don't want to see too many options in the exit dialog but the options which are there should be, to the extend possible, self explanatory.

With the code from this PR:

  1. Is solved pretty nicely - there are check boxes for both options and their labels are pretty self-explanatory.
  2. Is as easy with this PR as it is with the current master.
  3. This PR is a great step forward - I can check exactly options I need from the same dialog I'm using for ordinary restarts.
  4. Is as easy with this PR as with the current master.
  5. The dialog stays largely the same as it was - some options become check boxes but the same flexibility is provided.
  6. Option names in the current master are pretty self-explanatory - with the code from this PR the more developer centric options are moved from the combobox to the separate check boxes and therefore they are not getting in the way of the ordinary user.

With the @feerrenrut 's suggestion:

  1. While I have the new option called "safe mode" I have to read the manual to understand what it does exactly.
  2. To fulfill the request of the add-on author I have to go to the separate dialog which, for existing users, creates an unnecessary changes.
  3. Again changes just for the sake of the change benefits no one.
  4. Same as above - the proposed change provides me with no benefit. Furthermore if I'm testing against several older versions of NVDA in a row I have to pay attention to the fact that at some point the location of these options has changed.
  5. The proposal replaces clear wording for "restar with add-ons disabled" and "restar with debug logging" with some "Restart in safe mode" which gives no clue as to what this option does.
  6. Same as above really.

To summarize my issues with the alternative UX proposed above:

  • Safe mode is unclear - for starters I see no relation between it and enabling debug logging .
  • Making additional options a check boxes does not "draw attention to them" On the contrary given they're not shown unless "restart" is selected and need to be tabbed to makes them invisible for an average user.
  • This PR improves UX for developers both for people developing add-ons and for core programmers.

lukaszgo1 avatar Jan 19 '22 14:01 lukaszgo1

@lukaszgo1 thanks for this summary and the very useful 6 user stories.

I agree that "safe mode" is not self-explanatory. This terminology is used by Windows and some applications, but frankly, even having a little technical background, I do not know exactly what it does. Grossly, I imagine that it disable some optional features and components in order to allow the OS or program to start without problem. I guess starting the program or OS in safe mode is dedicated to troubleshouting, i.e. allow then to make some change in the config, (e.g. remove a plug-in, re-install a driver) with the hope that it will solve the problem.

For NVDA, "disable add-on" option can help minimize the probability of errors. But debug logging does not fall in the "safe mode" category IMO. Thus I would avoid this terminology.

I agree globally with @lukaszgo1's analysis. There is just one point I would like to highlight. Probably in user stories 5 and 6, you seem to consider that users open the restart dialog, select "Restart" in the combobox and press Enter to trigger "OK". Maybe that's not the case of all users. An other set of users may not be aware that Enter triggers the "OK" button. Thus they open the restart dialog, select "Restart" in the combo-box and then tab to "OK" to press it (by Enter or Space). In this case, they have to tab through the checkboxes until they reach the "OK" button; that will be more specifically the case for new users who want to explore an unknown dialog.

We also have to keep in mind that more restart options can be added in the future. For example I have a prototype in my personal NVDA Debug & Test Tools add-on. Try the NVDA+shift+Q script (only present in latest master, not yet in a release and not yet documented, sorry). The goal is not to follow what is done in the add-on but just to give an idea of more options that could be supported.

Thus if we want to keep the options in the usual restart dialog but make the option less visible, we may have a "More option" button allowing to show or hide the checkboxes. This button should be enabled only if "Restart" is selected in the checkbox.

CyrilleB79 avatar Jan 19 '22 16:01 CyrilleB79

I agree globally with @lukaszgo1's analysis. There is just one point I would like to highlight. Probably in user stories 5 and 6, you seem to consider that users open the restart dialog, select "Restart" in the combobox and press Enter to trigger "OK". Maybe that's not the case of all users. An other set of users may not be aware that Enter triggers the "OK" button. Thus they open the restart dialog, select "Restart" in the combo-box and then tab to "OK" to press it (by Enter or Space). In this case, they have to tab through the checkboxes until they reach the "OK" button; that will be more specifically the case for new users who want to explore an unknown dialog.

This is possible, but most users restarting NVDA probably do so to disable add-ons or enable debug logging.

We also have to keep in mind that more restart options can be added in the future. For example I have a prototype in my personal NVDA Debug & Test Tools add-on. Try the NVDA+shift+Q script (only present in latest master, not yet in a release and not yet documented, sorry). The goal is not to follow what is done in the add-on but just to give an idea of more options that could be supported.

Sure we can. For now though there are no open proposals as to what options should be added there and therefore we should not over engineer the implementation just to account for an eventual additions.

Thus if we want to keep the options in the usual restart dialog but make the option less visible, we may have a "More option" button allowing to show or hide the checkboxes. This button should be enabled only if "Restart" is selected in the checkbox.

I'm not aware about any complaints from users about "restart with add-ons disabled" or "restart with debug logging enabled" being problematic so I don't think we should simplify a dialog which is not complex in the first place unless there is an actual need which I'm not convinced about.

lukaszgo1 avatar Jan 19 '22 18:01 lukaszgo1

Actually, right now, it doesn't hide the restart grouping box, it just disables it, partly because it's just kinda a prototype right now to see what people think, and also it was at the time breaking the layout, leaving empty space when it's hidden but I've figured out why that was happening. And will push a commit to fix.

mzanm avatar Jan 19 '22 18:01 mzanm

@lukaszgo1 Thanks for taking time to describe your concerns here, at this point I'm not convinced adding more developer options to this dialog is in the best interest of NVDA.

There are several user stories

Note that the first 4 are developer-centric user stories. These are way less regularly a concern for users under normal circumstances. I want to put the most frequent user stories first, currently they are at 5 and 6.

My opinion is based on ordering use cases by frequency, roughly:

  • As a user, I want to confirm before NVDA exits or restarts, so that I don't accidentally exit NVDA by mistake.
  • As a user, I want to be able to select between exit / restart NVDA via a dialog so I don't have to remember two shortcuts.
  • As an NVDA core developer I want to be able to easily instruct users to restart NVDA with debug logging and add-ons disabled. So I can diagnose issues in NVDA using a detailed log without any interference from add-ons. As an add-on developer I want to confirm an issue is caused by an addon.
  • As an add-on developer, I want to be able to easily instruct users to restart NVDA with debug logging enabled. So that I can get a detailed log to diagnose issues with an add-on. (what about interference from other add-ons?)
  • As an add-on developer, I want to be able to easily instruct users to restart NVDA with debug logging and add-ons disabled. So I can confirm an issue is caused by an addon.
  1. As an existing user I don't want changes to the exiting interface which don't benefit me

While changes do require users to adapt, I'm suggesting taking two options out of combo box that are rarely useful for an end user. Those options don't disappear, I'm suggesting they are put somewhere else.

  1. The dialog stays largely the same as it was - some options become check boxes but the same flexibility is provided.

This leads to a dialog with controls which have a primary use case of helping developers, not regular users. Two developer-centric options exist already, rather than adding a third, or making the options more obvious, I think we have hit the "critical mass" required to reconsider where these belong. This does mean changes to parts of the UI not used regularly by most users.

With the code from this PR: ... 6. Option names in the current master are pretty self-explanatory - with the code from this PR the more developer centric options are moved from the combobox to the separate check boxes and therefore they are not getting in the way of the ordinary user.

The options are obvious to a developer, I don't think this is true of an end user. We frequently have to explain how addons that don't seem related might be causing a bug in NVDA. Debug logging comes with privacy, security and performance concerns. These aren't options users should be encouraged to play with. I think it is premature to say these extra controls don't get in the way of the ordinary user:

  • There are more controls to get through before getting to 'ok', we can't expect everyday users to just shortcut with enter.
  • Visually (don't forget low vision users), there are more options on the screen.

With the @feerrenrut 's suggestion:

  1. While I have the new option called "safe mode" I have to read the manual to understand what it does exactly.

In this scenario the user is directed by developers. User on there own also have to read the manual to understand why they should disable add-ons (eg why would this affect NVDA?), and what are the implications of debug logging (eg privacy, security, passwords) A term like "safe mode", gives a single, easy to describe setting for NVDA developers to ask users to enable the most commonly requested debug mode.

2,3,4: This opposition seems like "I don't like change", which isn't constructive. Developing good UX requires changes, as new options or features are added, old approaches cease to make sense, and the presentation to the user must be reconsidered. Making primary use cases obvious requires making low frequency use cases less obvious.

Safe mode is unclear - for starters I see no relation between it and enabling debug logging.

Perhaps we can develop alternative wording, however, I think opaque naming here is a "good thing". Users should read the manual before using this, or should only use it when directed by a developer / support.

On the contrary given they're not shown unless "restart" is selected and need to be tabbed to makes them invisible for an average user.

Regular users will tab through them, there are also low-vision users to consider, as well as fully sighted users. NVDA is assessed for usage within organizations, likely at least in part by sighted users. NVDA should appear professional, debugging and developer-centric options don't give a good impression.

If the debug options can be hidden behind a collapsible "advanced" group, that might be ok. I'm not sure about using "advanced" for this, and would need to review the GUI.

feerrenrut avatar Jan 20 '22 01:01 feerrenrut

Another thing to consider for the use-case of a core or add-on developer asking a user to restart with specific debug options, nvda+q may not work if they have set nvda to quit immediately, having a separate way to get to those options (a gesture, or via tools) makes providing instructions simpler.

feerrenrut avatar Jan 20 '22 03:01 feerrenrut

Note that the first 4 are developer-centric user stories.

That depends on the definition of the developer-centric - for me if the option can be used by the end user- even only when requested by the developer we cannot classify it as such.

(what about interference from other add-ons?)

I'd say this is a separate concern which we should not try to address here.

Perhaps we can develop alternative wording, however, I think opaque naming here is a "good thing". Users should read the manual before using this, or should only use it when directed by a developer / support.

Opaque naming is never a good thing assuming we want to thread our users as an intelligent adults which I believe we should do.

If the debug options can be hidden behind a collapsible "advanced" group, that might be ok. I'm not sure about using "advanced" for this, and would need to review the GUI.

This seems to be a reasonable compromise i.e.:

  • Don't introduce this "safe mode" thingy as it brings no benefit and is confusing as hell.
  • Remove "restart with debug logging" and :restart with add-onns disabled" from the combobox.
  • When "restart" is selected in the dialog show the new "advanced" button collapsed by default
  • When it is expanded show the check boxes for the options removed from the combobox.

The biggest technical hurtle we would need to overcome here would be using a right control from the wx for the collapsible button - that would require someone sighted to look into this.

lukaszgo1 avatar Jan 23 '22 20:01 lukaszgo1

That depends on the definition of the developer-centric - for me if the option can be used by the end user- even only when requested by the developer we cannot classify it as such.

I classify these as developer-centric. Following the "5 whys" approach, it quickly points out these options only exist to cater to the needs of developers. If developers had perfect information already, there would be no need for users to collect logs or try to isolate the cause of a bug, then these options wouldn't exist. An end user who isn't involved in development shouldn't be required to know what these options are for, ideally they would never encounter a bug that required them to use these options.

Opaque naming is never a good thing assuming we want to thread our users as an intelligent adults which I believe we should do.

I'm not suggesting otherwise. It is common to introduce a new concept with a new name that users have to learn. There are many ways we can make it easy to learn what this name means. It could be be called "restart (debug logging and no addons)" but that is very verbose. SafeMode is not a new concept, it has existed in Windows for decades, it's general use-case is well understood amongst support / technical users. Using a name like that gives a strong hint about what the option does. We could worry that it doesn't give enough details about what will happen, but the same is true for "Debug level logging", this requires a development background to have a good idea about what that might include.

I'm still happy for the collapsible approach to be explored. I'm happy to look at screenshots, if more assistance is required I'll have to add this work to our backlog and can't promise when we can look into it.

feerrenrut avatar Jan 24 '22 03:01 feerrenrut

Hello Regarding the collapsible section that I support, I will be able to help for a visual feedback and iterate during development and test if needed. When all is ready, a final visual feedback from someone else is still recommended however given my low level of vision.

CyrilleB79 avatar Jan 24 '22 07:01 CyrilleB79

SafeMode is not a new concept, it has existed in Windows for decades, it's general use-case is well understood amongst support / technical users. Using a name like that gives a strong hint about what the option does. We could worry that it doesn't give enough details about what will happen, but the same is true for "Debug level logging", this requires a development background to have a good idea about what that might include.

My main problem with the safemode is that in all other programs I know (Windows, Firefox) it does not enable any additional logging, and therefore our safemode would behave somewhat differently from the way users are used to.

I'm still happy for the collapsible approach to be explored. I'm happy to look at screenshots, if more assistance is required I'll have to add this work to our backlog and can't promise when we can look into it.

@mazen428 Is this something you would like to pursue?

lukaszgo1 avatar Jan 24 '22 11:01 lukaszgo1

Yes, I'm not sure of what widget to use for this though, as far as I know there is no expander widget in WX, maybe a toggle button could work?

mzanm avatar Jan 25 '22 12:01 mzanm

I found. https://docs.wxpython.org/wx.lib.agw.pycollapsiblepane.PyCollapsiblePane.html I've tried it and I think it has some issues like the collapsed content still can be accessed with keyboard navigation.

mzanm avatar Jan 25 '22 12:01 mzanm