Comment 1 by aleksey_s on 2010-02-06 12:32
I'll be glad to work myself on this feature; however i would like to hear your opinions onpossible side effects.
I consider reloading appModules with python reload() buildin function together with cleaning runningTable dictionary. One problem I see is that all NVDAObject instances own a pointer to their appModule, which, in fact, will mean that old modules will be not garbage collected. Your thoughts?

Comment 2 by jteh on 2010-02-06 21:52
I'll comment more on this later, but one thing to consider is that I think reload() was removed in Python 3 due to problems. I think you can manually reload by manipulating sys.modules and the like, though.

Comment 3 by jteh on 2010-02-06 23:08
NVDAObject instancdes have a weakref to their appModule, not a strong reference, so this isn't an issue.

There are other issues, though. An app module (and in future, a global plugin) isn't restricted in terms of what it can do within NVDA. That is, there is no separate scripting environment like there is with a lot of other ATs. This gives us a lot of power and flexibility, but it also means that reloading could be dangerous or, at the very least, very tricky. For example, an appModule might add something to some internal structure, but when it is reloaded, it will then add it again. You might be able to get around this by having a function which gets called just before the module unloads.

I wonder, though: would a keystroke to restart NVDA suit your purposes? At present, you have to press a few keystrokes, but I've often thought having a restart keystroke within NVDA. It's not quite as fast, but it's not particularly slow either.

Comment 4 by pvagner on 2010-02-06 23:50
Huh, I'll make it even more complicated, sorry.
Often while trying to create an appmodule I am reading and searching the web for code examples, possible hints and related materials.
In between I am frequently restarting NVDA which very often resets my position in the virtual buffer.
So if I'll take this request from this point of view reloading of the appmodules really makes sense. However then we will really need to make AppModule classes more robust and introduce a initialize() and terminate() pair of functions or something similar which will allow appmodules to tidy up what they have altered while running in order to be reloaded.

Comment 5 by aleksey_s on 2010-02-07 00:08
Jamie,
We then need to design a specification for appModules (and possible plugins in future) and mention about what they are allowed to do. For example, appModules need to make all internal changes only in the constructor of appModule class, and clean in the destructor. I am not sure about plugins, because I don't have a clear image of the interface yet. Do they be class instances too?

Peter,
Yes, I also consider your use case. That's why i stay for appModule reload, not the whole NVDA.

Comment 6 by jteh on 2010-03-20 05:57
We can provide a terminate() method for app modules and note that all cleanup must be done there. If the app module leaves any references around, it will never be garbage collected and will therefore cause a memory leak, but that's not our problem. :) Does this sound okay?

Comment 7 by aleksey_s (in reply to comment 6) on 2010-03-20 06:20
Replying to jteh:

We can provide a terminate() method for app modules and note that all cleanup must be done there.

It is OK. However, why do we need yet another function in appModule: we already have del method of appModule instance. Introducing a new global module function breaks object model a bit.

Comment 8 by jteh on 2010-03-20 12:43
Err, sorry, I meant a terminate() method on the !AppModule class. !del() is not good enough because it only gets called when all references to the object are released. If you've somehow referenced your !AppModule instance from somewhere else, you might need to forceably remove it before !del() will be called. Most !AppModules should never have to implement terminate().

Having said that, I can't think of any reason why someone should be saving a reference to an !AppModule instance, so perhaps !del() is enough for now until we come up with use cases where it is a problem.

Comment 9 by aleksey_s (in reply to comment 2) on 2010-09-26 06:29
Replying to jteh:

one thing to consider is that I think reload() was removed in Python 3 due to problems.

It is moved to imp.reload: http://docs.python.org/dev/py3k/library/imp.html#imp.reload
So I assume clearing running table in appModuleHandler.py will be enough.

Comment 10 by aleksey_s on 2010-09-26 10:29
See BZR branch: http://bzr.nvaccess.org/nvda/ticket544 for proposed implementation.

Comment 11 by jteh on 2010-09-27 22:12
Lex, just so you know, I've reviewed this and will provide feedback in the next day or two. It looks good for the most part. Thanks.

Comment 12 by jteh on 2010-09-28 02:36
Okay. Code review. :)

First, I'm curious as to why you decided to reload all app modules, rather than just the current app module. Having said that, this certainly isn't a problem; I'm just curious. One advantage of doing it the way you ahve is that you can reload app modules from the Tools menu, which you wouldn't be able to do if it was only the current module.

Please change all user visible occurrences of appModules to "app modules" (space between, no capital m).

=== modified file 'source/appModuleHandler.py'

• Each appModule will be reloaded imediately as a reaction on a first event coming from the process."""

Knit: Docstring close quotes should generally be on their own line. Also, immediately is spelt with a double m.

=== modified file 'source/appModules/_default.py'

• def script_reloadAppModules(self,keyPress):

• ui.message(_("Reloading appModules..."))
  

I'd remove the "..." here. It makes it seem as if the user should expect "done" or similar, which you can't do as per your comment later.

• script_reloadAppModules.doc=_("Reloads appModules from the disk without restarting NVDA, which can be Useful for developers")

I'd remove "from the disk". If you really want to keep it, change it to "from disk".

You need to add a keyboard binding to the laptop layout as well. The same one will do.

=== modified file 'user_docs/en/changes.t2t'

+- Ability to reload appModules without restarting of whole NVDA was introduced for convenience of appModule developers. Use tools->Reload appModules in the NVDA menu or NVDA+ctrl+f3. (#544)

I'd change the first sentence to "For the convenience of app module developers, app modules can now be reloaded without restarting NVDA."

You also need to add a very short section on this to the Extra Tools section of the User Guide.

Overall, great work - thanks.
Changes:
Milestone changed from None to 2010.3

Comment 13 by jteh on 2010-09-30 07:15
One more thing to check: If the current focus object is based on an overlay class from an app module, that overlay class must stay around at least until that object is disposed. This shouldn't be an issue, but please check the following:

1. Go to the system tray with win+b. This should use the !NotificationArea overlay class from explorer.
2. Reload app modules.
3. Check that you can still use the focus object; e.g. NVDA+tab.

This also raises another issue. Dynamically generated NVDAObject classes (name beginning with "Dynamic") will get cached. I suspect this includes app module overlay classes, which means they won't get released. We either need to stop caching dynamic classes that use app module overlays or come up with some way to remove them from the dynamic class cache.

Comment 14 by mdcurran on 2010-12-01 05:34
Any answer on the questions in the previous comment? if all is ok this could be merged, after any merge conflicts resulting from recent changes are fixed.
I don't think the issue about caching dynamic classes is really something to hold this feature back, it at most would only cause a small but harmless memory leak, that cached class would never get used again anyway.

Comment 15 by aleksey_s (in reply to comment 13) on 2010-12-01 18:06
Replying to jteh:

1. Go to the system tray with win+b. This should use the !NotificationArea overlay class from explorer.
2. Reload app modules.
3. Check that you can still use the focus object; e.g. NVDA+tab.

NVDA+tab works OK.

This also raises another issue. Dynamically generated NVDAObject classes (name beginning with "Dynamic") will get cached. I suspect this includes app module overlay classes, which means they won't get released. We either need to stop caching dynamic classes that use app module overlays

if only to allow nice app module reloading without glitches - then it is a bad idea :-)

or come up with some way to remove them from the dynamic class cache.

I am not sure how to achieve this for now, can you give some advice?

Comment 16 by aleksey_s (in reply to comment 14) on 2010-12-01 18:09
Replying to mdcurran:

Any answer on the questions in the previous comment? if all is ok this could be merged, after any merge conflicts resulting from recent changes are fixed.

done as well as done global plugin reloading in the same scope.

I don't think the issue about caching dynamic classes is really something to hold this feature back, it at most would only cause a small but harmless memory leak, that cached class would never get used again anyway.

Honestly, I am lost with all that dynamic class creation/manipulation stuff. If any of you gave me advice on how to do the dynamic class cache cleanup correct, i would appreciate it.

Comment 17 by jteh on 2011-01-05 22:32
Merged in 058afe8 with a few changes by me. Thanks for your work.
Changes:
State: closed