Added a condition that checks if a addon directory is empty when loading it initially. by awalvie · Pull Request #9372 · nvaccess/nvda

awalvie · 2019-03-13T14:02:08Z

Link to issue number:

Summary of the issue:

Currently NVDA's addonHandler only checks whether an addon directory exists or not, if it does, then it tries to load the addon from the directory. The issue arises when files are manually deleted from a Addon folder except from the folder itself. As the addon still has a directory it's empty and hence does not contain the necessary files, which throws an error that stop NVDA from starting.

Original Issue

Description of how this pull request fixes the issue:

As suggested in the issue conversation I am just checking if the addon directory has any files inside it.

Testing performed:

Installed the BrowserNav Plugin
Restarted NVDA with addons Disabled and removed all files in BrowserNav's Folder
Restarted NVDA with addons Enabled

(Exptected)

NVDA Fails to start as the addon directory has no files.

(After Fix)

NVDA Does indeed restart with a log.error() specifying that the addon at the following location could not be loaded.

Known issues with pull request:

It fixes the issue but dose not inform the user that the addon has been deleted

Section: Bug fixes

awalvie · 2019-03-13T16:31:35Z

I don't understand why the test build fails. Can someone help me out with this and suggest better changes ?

LeonarddeR · 2019-03-13T16:43:30Z

The failing tests aren't caused by your code. This will be dealt with in another ^pr.

josephsl · 2019-03-13T16:50:40Z

Hi, no, it has nothing to do with our pull requests – it has to do with dependencies (there is a pull request to rectify this). Thanks.

awalvie · 2019-03-13T16:57:12Z

Alright, thanks for the quick replies guys, this is my first pull request so I thought I might've made a mistake.

feerrenrut · 2019-03-20T14:58:40Z

Because this issue is indicative of a broader error state, we should not merely hide this situation. I would recommend adding a log message at the error level to warn the user / developer that this has occurred. NVDA should then be able to continue, excluding this addon.

awalvie · 2019-03-20T15:03:51Z

Got it i'll try adding that in. Can someone tell me how to access the access to the log file works in NVDA. Can I just use a with open block to write the error to it ?

feerrenrut · 2019-03-21T17:45:40Z

Can someone tell me how to access the access to the log file works in NVDA.

There are lots of examples in the file you are editing. For instance the line after your edit is:
log.debug("Loading add-on from %s", addon_path)

Essentially, you can call log.error() passing a string for the message. Coming up with a good message is the hard part. There are several log levels, you can set the log level of NVDA on the general settings panel (for development you generally want debug level logging), or with the command line option --debug-logging.

…y is empty.

awalvie · 2019-03-23T09:37:01Z

Can you check if this is alright ? @feerrenrut

feerrenrut

Looks like you are on the right track. A few suggestions here, after you have made these changes and tested again, could you also please update the testing performed section to show the steps you took to test this and how you verified that the result was correct.

feerrenrut · 2019-03-23T21:08:12Z

+				except:
+					log.error("Error loading Addon from path: %s", addon_path, exc_info=True)
+			else:
+				log.error("Error loading Addon from path: %s, try reinstalling: %s", addon_path, name,exc_info=True)


I don't think there is an exception active at this stage, I dont think the parameter exc_info is necessary. As mentioned in the other comment, I think it would improve the readability if this block was moved under the if statement.

I haven't yet tried this out, is it true that reinstalling will work?

Yes reinstalling works. What happens when you delete all the files is that the addon disappears from the addon list and you have to reinstall it. And I have changed the if condition around as you have suggested.

Thanks @awalvie. Could you please update the "testing performed" section of the initial description of this PR. Please see our wiki page explaining the PR template for further information and examples on this

Yes definitely.

Any other changes you'd recommend making ?

Nope, just waiting on the changes to the "testing performed" section of the initial description of this PR. Don't forget to read the wiki page.

Really sorry, forgot about it. I'll do this ASAP

@feerrenrut Just updated it. Let me know if any other changes need to be done. Thanks for the feedback.

feerrenrut

I'll merge this in. Thanks for your work on this.

No longer crash when an addon directory is empty.

awalvie · 2019-03-27T11:11:01Z

Thanks for all the help and suggestions.

Fix for #7686

bfad3a2

awalvie changed the title ~~Fix for #7686~~ Added a condition that checks if a addon directory is empty when loading it initially. Mar 13, 2019

Merge remote-tracking branch 'origin/master' into HEAD

a156d82

This comment has been minimized.

Sign in to view

awalvie added 4 commits March 23, 2019 11:00

Now writes to the log file in case of failing the condition check

0868773

Merge branch 'master' of https://github.com/nvaccess/nvda into i7686

dcbd2e2

Merge branch 'i7686' of https://github.com/Awalvie/nvda into i7686

cc0d3ad

Added a condition that gives an error in the log file if the Director…

9adadee

…y is empty.

feerrenrut reviewed Mar 23, 2019

View reviewed changes

awalvie and others added 2 commits March 24, 2019 15:33

Changed the if conditions around as suggested

3cf8041

Deleted VsCode file

a2b7c52

feerrenrut approved these changes Mar 27, 2019

View reviewed changes

feerrenrut merged commit d04614a into nvaccess:master Mar 27, 2019

nvaccessAuto added this to the 2019.1 milestone Mar 27, 2019

feerrenrut added a commit that referenced this pull request Mar 27, 2019

Update changes file for PR #9372

d9dde22

No longer crash when an addon directory is empty.

michaelDCurran modified the milestones: 2019.1, 2019.2 Mar 27, 2019

Uh oh!

Conversation

awalvie commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

(Exptected)

(After Fix)

Known issues with pull request:

Uh oh!

awalvie commented Mar 13, 2019

Uh oh!

LeonarddeR commented Mar 13, 2019 via email

Uh oh!

josephsl commented Mar 13, 2019 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awalvie commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Mar 20, 2019

Uh oh!

awalvie commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

feerrenrut commented Mar 21, 2019

Uh oh!

awalvie commented Mar 23, 2019

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feerrenrut Mar 23, 2019

Choose a reason for hiding this comment

Uh oh!

awalvie Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feerrenrut Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

awalvie Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

awalvie Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

awalvie Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

awalvie Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

awalvie commented Mar 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

awalvie commented Mar 13, 2019 •

edited

Loading

josephsl commented Mar 13, 2019 via email •

edited by feerrenrut

Loading

awalvie commented Mar 13, 2019 •

edited

Loading

awalvie commented Mar 20, 2019 •

edited

Loading

awalvie Mar 24, 2019 •

edited

Loading