Cyrille,

Many thanks for making this very useful add-on more official than it was!
While I'm using some parts of it pretty regularly I never had time to
look at the code, or to familiarize myself with all its features. Some
review points below:

- In the beepError file you may consider using globalVars.appDir if it
is defined when creating path to the error sound. That would allow
people on never versions of NVDA who are using Ivona SAPI voices (they
tent to change CWD when loading them for the first time) to have the
sound played.
- in the myHandle function there is unnecessary import of api
- in the fileOpener when retrieving path to the nVDA source, and when
user is running a source build you're setting the path to
globalVars.appDir even for versions of NVDA where it is not yet
implemented. An alternative approach which would handle the case where
CWD has been changed to something silly would be to return
__main__.__file__ in such case.
- In the logReader classes such as LogMessageHeader and LogMessage don't
inherit from object - this causes them to be old style classes under
Python 2. While I cannot point out any real issues due to this in code
which targets both Python 2 and 3 it is better to inherit from object
explicitly.
- In the makeFromTextInfo method you're raising NotImplemented instead of
NotImplementedError - this is a common Python gotcha NotImplemented
cannot be raised it is a singleton returned when the given class does not
support a particular arithmetic operation on the provided argument.
- Not an error as such but when checking if the given window is the log
viewer you can avoid comparing window names by checking if the window
handle equals gui.logViewer.logViewer.GetHandle() (don't forget to
handle the case where log viewer is not yet initialized or closed).
- It is unclear to me why you're setting directory to None in the
restartWithOptions for Python 2 versions of NVDA, so a comment in code
here would be nice to have.
- The design of the restartWithOptions is... shall we say, suboptimal:
- The OPTION-LIST would greatly benefit from usages of named tuples, so
that you do not need to remember what indexes for the given suboptions
are
- Since each option has a type of allowed values you may avoid these
extended isinstance checks, implement a method called createControls or
similar for each of the classes such as FolderStr, and use polymorphism
to create the control for each of the options.
- If the default value is neither FolderStr nor FileStr you're using a
bare raise statement effectively throwing whatever exception would be
returned by sys.exc_info at this point - that is almost certainly not
what you want to do - when I've done this for testing I got exception
from the touchHandler as my machine is not touch capable.
- In the script_toggleStackTraceLog _originalFunction does not need to
be defined as global
- While certainly not something to implement for this version it would
be interesting to introduce a gui for specifying what function should be
patched for stack tracing.
- I have some reservations about the way in which compatibility for
controlTypes is handled. Essentially it is never a good idea to modify
the original module with the new code unless you have to monkey patch it.
Additionally more and more add-ons seems to add their own implementation
of controlTypes.Role and controlTypes.State and it is only a matter of
time when, depending on the order in which add-ons are loaded some
implementation becomes incompatible with whatever the next add-on
decided to implement causing no end of troubles. For an alternative,
much safer, implementation, you can take a look at my add-on for Becky!
in particular class ControlTypesCompatWrapper. It does not support
output reasons, but adding them should be trivial.


--
Regards
Lukasz