Added a specific log channel for nvwave debug message by CyrilleB79 · Pull Request #11582 · nvaccess/nvda

CyrilleB79 · 2020-09-09T14:05:10Z

Link to issue number:

Summary of the issue:

After the merge of #11531, some extra debug messages have been added in the log. They are quite specific and make the log quite noisy while debugging something else.
Since they are used for a well specific purpose and to ease the debug tasks, it was suggested and approved in #11574 (comment) that they appear under a dedicated log flag.

Description of how this pull request fixes the issue:

Added a new "nvwave" item in the "Enabled logging categories" list of the "Advanced" setting panel. This allow to log nvwave utility messages only if this item is checked.
This item is unchecked by default
All the logger calls in the nwave.py have been put under "nvwave" log flag, unless the following ones:
- log.warning(f"Unable to open WAVE_MAPPER device, there may be no audio devices.")
- log.warning("Error during feed. Resetting the device.")
- log.debugWarning("Unable to send data to audio device on second attempt.", exc_info=True)
- log.exception("Error calling onDone")
I have filtered the following log.debug call but I have a doubt on it:

log.debug(
	f"Error opening"
	f" outputDeviceName: {self._outputDeviceName}"
	f" with id: {self._outputDeviceID}"
)```

Testing performed:

Set log level to debug
Checked that some of the messages under this log channel are logged if "nvwave" log is enabled and that they are not logged if it is disabled.

Known issues with pull request:

None.

Change log entry:

No need to specify the corresponding issue (#11574) since this issue was only seen on alpha.
Is there a need to indicate this log channel in the Changes for developers section? Such other log channels are not specified in the change log.

The majority of the message logged in nvwave.py is disabled by default. To enable it, check the nvwave item in the "Enabled logging categories" list of the "Advanced" setting panel.

CyrilleB79 · 2020-09-09T14:09:22Z

@feerrenrut, may you review this one?

Could you specifically look at the following points (among other):

I have set "nvwave" as the name of this log category. Is it adapted and self-explaining enough?
I have excluded some messages from this flag, i.e. they are logged even if "nvwave" item is not checked, because they seemd more important to me. Could you check this?

I have no experience of this part of code so let me know your feedback on these two points as well as on this PR more generally.

I have targeted beta branch as you told me.
I do not know how much time these nvwave messages should remain in the general log.debug. Maybe a little time with more users/testers in the beta before merging it? It's up to you!

feerrenrut

Generally looks good, thanks @CyrilleB79

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

…ebugForNvWave in the whole file.

feerrenrut

Thanks @CyrilleB79

CyrilleB79 · 2020-09-10T20:22:37Z

I have written the following line in this PR's description:

Fixes #11574

However, #11574 does not appear linked to this PR, nor closed. Why?
I may close the issue by myself, but I would be interested to know why it did not link automatically...

lukaszgo1 · 2020-09-10T21:48:36Z

@CyrilleB79 wrote:

I have written the following line in this PR's description:

Fixes #11574

However, #11574 does not appear linked to this PR, nor closed. Why?
I may close the issue by myself, but I would be interested to know why it did not link automatically...

GitHub closes issues automatically only when the corresponding PR is opened against main branch of the repository - in this case against master. Since this one targets beta #11574 will be closed after beta containing commit from this PR would be merged to master.

CyrilleB79 · 2020-09-10T21:57:35Z

@lukaszgo1 thanks for the answer.

However in #11574, I can read:

Linked pull requests
Successfully merging a pull request may close this issue.
None yet

I would have expected the PR to be linked to the issue, even if the issue is not yet closed (since not yet merged into master branch).

lukaszgo1 · 2020-09-11T13:41:04Z

@CyrilleB79 wrote:

@lukaszgo1 thanks for the answer.

However in #11574, I can read:

Linked pull requests
Successfully merging a pull request may close this issue.
None yet

I would have expected the PR to be linked to the issue, even if the issue is not yet closed (since not yet merged into master branch).

That is quite strange indeed. I have no idea why this is happening. It would be interesting what happens after beta is merged to master.

CyrilleB79 · 2020-09-16T11:38:37Z

Oops, it seems that I have forgotten one instance of log.debug.

def _close(self):
		log.debug("Calling winmm.waveOutClose")

Should I make a PR against beta or master?

On the contrary, as stated in the description of this PR, the following log was left intentionally since it corresponds to an error:

log.debug("Error closing the device, it may have been removed.", exc_info=True)

feerrenrut · 2020-09-16T11:52:27Z

This can be made against beta. Thanks @CyrilleB79

…ave channel.

Fix-up of PR #11582

Added a specific log channel for nvwave debug message.

93f0890

The majority of the message logged in nvwave.py is disabled by default. To enable it, check the nvwave item in the "Enabled logging categories" list of the "Advanced" setting panel.

This comment has been minimized.

Sign in to view

Fixed linting issue.

b823148

feerrenrut suggested changes Sep 9, 2020

View reviewed changes

Comment thread source/nvwave.py Outdated

CyrilleB79 and others added 2 commits September 9, 2020 22:40

Address review comment.

74a82bb

Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>

Address review comment (follow-up): function _isDebug renamed to _isD…

c1f2067

…ebugForNvWave in the whole file.

feerrenrut approved these changes Sep 10, 2020

View reviewed changes

feerrenrut merged commit 7896e88 into nvaccess:beta Sep 10, 2020

nvaccessAuto added this to the 2020.4 milestone Sep 10, 2020

feerrenrut modified the milestones: 2020.4, 2020.3 Sep 10, 2020

CyrilleB79 mentioned this pull request Sep 13, 2020

Log too verbose with last alpha #11574

Closed

michaelDCurran mentioned this pull request Sep 13, 2020

alpha builds: many error messages in the log when opening Adobe Reader #11568

Closed

CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Sep 16, 2020

Fix-up of PR nvaccess#11582: filter an extra call to log.debug in nvw…

f9077a9

…ave channel.

CyrilleB79 mentioned this pull request Sep 16, 2020

Fix-up of PR #11582: filter an extra call to log.debug in nvwave channel #11614

Merged

feerrenrut pushed a commit that referenced this pull request Sep 16, 2020

Filter an extra call to log.debug in nvwave channel. (PR #11614)

39e828c

Fix-up of PR #11582

CyrilleB79 deleted the nvwaveLog branch September 22, 2020 20:22

CyrilleB79 mentioned this pull request Sep 22, 2020

Sometimes pressing shift during SayAll stops it instead of just pausing it #10946

Open

CyrilleB79 mentioned this pull request Nov 7, 2020

Add a specific debug channel for frequent synthDriver log messages #11816

Merged

Adriani90 mentioned this pull request Dec 14, 2024

Remove winmm support from NVDA #17496

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Added a specific log channel for nvwave debug message#11582

Added a specific log channel for nvwave debug message#11582
feerrenrut merged 4 commits into
nvaccess:betafrom
CyrilleB79:nvwaveLog

CyrilleB79 commented Sep 9, 2020

Uh oh!

CyrilleB79 commented Sep 9, 2020

Uh oh!

This comment has been minimized.

feerrenrut left a comment

Uh oh!

Uh oh!

feerrenrut left a comment

Uh oh!

CyrilleB79 commented Sep 10, 2020

Uh oh!

lukaszgo1 commented Sep 10, 2020

Uh oh!

CyrilleB79 commented Sep 10, 2020

Uh oh!

lukaszgo1 commented Sep 11, 2020

Uh oh!

CyrilleB79 commented Sep 16, 2020

Uh oh!

feerrenrut commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

CyrilleB79 commented Sep 9, 2020

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

CyrilleB79 commented Sep 9, 2020

Uh oh!

This comment has been minimized.

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 commented Sep 10, 2020

Uh oh!

lukaszgo1 commented Sep 10, 2020

Uh oh!

CyrilleB79 commented Sep 10, 2020

Uh oh!

lukaszgo1 commented Sep 11, 2020

Uh oh!

CyrilleB79 commented Sep 16, 2020

Uh oh!

feerrenrut commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants