Use only supported log levels for command line option #14406

CyrilleB79 · 2022-11-26T23:03:11Z

Context: issue found while reading the code for #14398.

Link to issue number:

None

Summary of the issue:

When starting NVDA with an unsupported command line parameter, you get the following suggestion for log level in the error message:

[-l {10,12,15,20,30,40,50,100}]

Launching NVDA with log level set to 30, 40 or 50 actually indicates NVDA using info level.

Description of user facing changes

Only supported log levels are indicated in the error message when a wrong parameter is passed to NVDA.
Only supported log level can now be used from command line, i.e. 10=debug, 12=I/O, 15=debugWarning, 20=info and 100=off

Description of development approach

Changed the information passed to the command line parser.
While at it, changed default from "en" to "Windows" in the parser's --lang parameter; this seem to have no impact however since "Windows" was already used when this parameter is missing.

Testing strategy:

Manual tests:

NVDA starts normally when -l parameter is not passed to NVDA
NVDA starts normally with -l command line param set to 10, 12, 15, 20 and 100 and the appropriate log level is used
NVDA does not start and displays the usage message when launched with -l set to 30, 40, 50 and 99.

Note: also tested --lang=windows; this fails but it was already failing before this PR, e.g. in NVDA 2022.4beta3. I will open an issue for that.

Known issues with pull request:

None

Change log entries:

Probably not needed.
Or maybe something in the bugfixes category? What do you think?

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
Security precautions taken.

LeonarddeR · 2022-12-02T18:42:00Z

I'm somehow missing the logging category from NVDA"s general settings. Could this pr be related?

CyrilleB79 · 2022-12-02T22:34:17Z

I'm somehow missing the logging category from NVDA"s general settings. Could this pr be related?

Could you elaborate?

When you run nvda with the --lang parameter, the logging level item is disabled in General settings; but I guess you know it.

LeonarddeR · 2022-12-05T10:14:05Z

Sure. When launching NVDA from my desktop shortcut, logHandler.isLogLevelForced() returns True. shortcut is: "C:\Program Files (x86)\NVDA\nvda_slave.exe" launchNVDA -r. Even without -r, it returns True.
globalVars.appArgs.logLevel is set to 20 and therefore the following clause applies:
or globalVars.appArgs.logLevel != 0

LeonarddeR · 2022-12-05T10:15:26Z

source/nvda.pyw

+	'--log-level',
+	dest='logLevel',
+	type=int,
+	default=20,


This line breaks it for me.

OK. I can see the issue, thanks. I will fix it.

CyrilleB79 · 2022-12-05T22:43:35Z

In addition, I just realize that in this PR, I have not updated the user guide command line option.

Moreover, I have now come across this comment in and more generally the discussion in #8596. It seems that there were (for NVDA 2022.4beta3 and before) many log levels and only a subset of them were accessible from the GUI, as explained by @josephsl .

Thus we have two solutions:

Solution 1: Fix-up this PR

Fix the issue with log level combo-box not usable anymore in the GUI: the default value in the parser is to be restored to 0.

We would end-up with the following command-line log levels being removed: 30 (= WARNING), 40 (= ERROR), 50 (= CRITICAL).

Since INFO is not so much verbose, are there really persons using the -l switch with values of 30, 40 or 50 today?

Note: DEBUGWARNING (12) remains available and is not to be confused with WARNING (30).

Solution 2: Revert this PR

On the opposite, we may revert this PR.

On the contrary to what I have written in the description of this PR, levels 30, 40, 50 seem to work correctly and NVDA does not log INFO messages at these levels.

There would be some fixes to do in the future however:

In the GUI, when -l is 30, 40 or 50, the combobox for log level is disabled and blank. Even if it is disabled, it should indicate the current log level passed as parameter.
At level 30, 40 and 50, NVDA+F1 does not allow to open the log viewer; it should instead.
Maybe indicate more explicitly in the User Guide that the levels 30 to 50 are command-line only
Refactor the code to list all the log levels in logHandler and associate a boolean value or a function indicating if the level is accessible from the GUI or not.

Conclusion

Before doing anything, I would like to hear NVAccess point of view (@seanbudd, @feerrenrut) as well as other members of the community (@josephsl, @LeonarddeR)

This reverts commit 279643e.

michaelDCurran · 2022-12-13T00:25:05Z

This pr is reverted by pr #14442
A new pr based on this one, that also fixes the identified issue/s here, and also most likely incorporates pr #14432 would be welcome.

…" (#14442) Reverts #14406 PR #14406 accidentally hid the log level combo box. Also it looks like further work was identified to provide a complete solution. All of this should be handled in a new PR.

CyrilleB79 · 2022-12-13T08:43:08Z

@michaelDCurran it is unclear which solution from #14406 (comment) NVAccess would like.

Solution 1 was fully implemented by #14406 + #14432. But you have reverted #14406 and closed #14432, which would let think that you prefer solution 2.

But on the other side you write:

A new pr based on this one, that also fixes the identified issue/s here, and also most likely incorporates pr #14432 would be welcome.

That let me think that you agree with solution 1.

So, could you then react to #14406 (comment) and indicate clearly what you are expecting? In other words, would you like WARNING, ERROR and CRITICAL to be available as NVDA log level or removed? And if you want to keep it, should these levels be available only with command line (as today) or also in the GUI?

Thanks.

michaelDCurran · 2022-12-13T12:00:04Z

Firstly, reverting the pr should not be seen as a statement on which solution I liked. NV Access's policy is to revert a pr if it directly causes an issue and a release containing the pr has not yet been made. What I wrote about making a new pr possibly incorporating #14406 and #14432 was more a general statement saying that there was a problem. We reverted. Please try again in a new pr. Sorry if I did not word this well.
Specifically considering the solutions you have provided, I would prefer solution 1, only list log levels from info down. We don't ever want users providing logs to us at level warning, error or critical as they are simply not useful as they drop too much information and therefore just waste time for the user and us.

nvaccess#14406)" (nvaccess#14442)" This reverts commit 324f6e5.

@michaelDCurran

#14444) When starting NVDA with an unsupported command line parameter, you get the following suggestion for log level in the error message: [-l {10,12,15,20,30,40,50,100}] Launching NVDA with log level set to 30, 40 or 50 leads to undesirable behaviours: • an empty value in the log level combo-box in NVDA's general settings. • NVDA+F1 does not open the log • NVAccess does not want log level WARNING, ERROR or CRITICAL as written by @michaelDCurran in Use only supported log levels for command line option #14406 (comment): We don't ever want users providing logs to us at level warning, error or critical as they are simply not useful as they drop too much information and therefore just waste time for the user and us. Also, the description of the -l option is erroneous in the user guide and in the help text for command line since it indicates that the default value for this option is WARNING. Instead, the default behaviour (i.e. when the option is not specified) is to follow what comes from NVDA config; and if there is nothing in the user's config, the default config spec specifies that the log level is INFO. Description of user facing changes • Only supported log levels are indicated in the error message when a wrong parameter is passed to NVDA. • Only supported log level can now be used from command line, i.e. 10=debug, 12=I/O, 15=debugWarning, 20=info and 100=off • The description of the -l option is updated in the user guide and in the help text for command line Description of development approach • Changed the information passed to the command line parser. • Updated the available log levels in the user guide and the command line help; also removed the indication related to "default" since it was just adding confusion. For more clarity I may have rewritten the option description in command line help and in the user guide to something like "Forces the log level". I have not done this since it is not done for other options such as -c, --log-file, etc.

Use only supported log levels for command line option.

ce4c341

CyrilleB79 marked this pull request as ready for review November 28, 2022 07:41

CyrilleB79 requested a review from a team as a code owner November 28, 2022 07:41

CyrilleB79 requested a review from seanbudd November 28, 2022 07:41

seanbudd approved these changes Nov 28, 2022

View reviewed changes

seanbudd merged commit 279643e into nvaccess:master Nov 28, 2022

nvaccessAuto added this to the 2023.1 milestone Nov 28, 2022

CyrilleB79 deleted the errorMsg branch November 29, 2022 08:27

LeonarddeR reviewed Dec 5, 2022

View reviewed changes

CyrilleB79 mentioned this pull request Dec 9, 2022

Fix-up 14406: restore combo-box to set log level in GUI parameters. #14432

Closed

4 tasks

michaelDCurran added a commit that referenced this pull request Dec 12, 2022

Revert "Use only supported log levels for command line option. (#14406)"

c368e68

This reverts commit 279643e.

michaelDCurran mentioned this pull request Dec 12, 2022

Revert "Use only supported log levels for command line option" #14442

Merged

CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Dec 13, 2022

Revert "Revert "Use only supported log levels for command line option. (

d48d477

nvaccess#14406)" (nvaccess#14442)" This reverts commit 324f6e5.

CyrilleB79 mentioned this pull request Dec 13, 2022

Use only supported log levels for command line option (second attempt) #14444

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only supported log levels for command line option #14406

Use only supported log levels for command line option #14406

CyrilleB79 commented Nov 26, 2022

LeonarddeR commented Dec 2, 2022

CyrilleB79 commented Dec 2, 2022

LeonarddeR commented Dec 5, 2022

LeonarddeR Dec 5, 2022

CyrilleB79 Dec 5, 2022

CyrilleB79 commented Dec 5, 2022

michaelDCurran commented Dec 13, 2022

CyrilleB79 commented Dec 13, 2022

michaelDCurran commented Dec 13, 2022

Use only supported log levels for command line option #14406

Use only supported log levels for command line option #14406

Conversation

CyrilleB79 commented Nov 26, 2022

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

LeonarddeR commented Dec 2, 2022

CyrilleB79 commented Dec 2, 2022

LeonarddeR commented Dec 5, 2022

LeonarddeR Dec 5, 2022

Choose a reason for hiding this comment

CyrilleB79 Dec 5, 2022

Choose a reason for hiding this comment

CyrilleB79 commented Dec 5, 2022

Solution 1: Fix-up this PR

Solution 2: Revert this PR

Conclusion

michaelDCurran commented Dec 13, 2022

CyrilleB79 commented Dec 13, 2022

michaelDCurran commented Dec 13, 2022