Generic framework for code extensibility via actions and filters

[jcsteh]

Link to issue number:

Fixes #3393.

Summary of the issue:

While we have events for NVDAObjects (and a few events for AppModules and TreeInterceptors), they are very specific to the object and there are strict limits on what can listen to them. This makes sense for those events. However, there needs to be a more generic mechanism for things that are broader in scope so that any interested party can be notified. For example, add-ons (and even core code) may wish to tweak speech output or respond to configuration profile switches. This allows us to mitigate two kinds of ugliness:

1. There are parts of the code where there is currently nasty coupling. For example, config.ConfigManager._handleProfileSwitch has to import synthDriverHandler, braille, etc. and call handleConfigProfileSwitch functions on each component. With this framework, those components can instead register to be notified, thus decoupling the code.
2. Quite a lot of add-ons are currently monkey patching our code to do things such as intercept or modify speech output, detect when beeps are played, etc. This is difficult to get right and is prone to serious breakage, especially where multiple add-ons patch the same function. Over time, we can add extension points to replace the need for this.

Description of how this pull request fixes the issue:

This PR provides a generic framework to enable extensibility at specific points in the code by way of actions and filters.

This allows interested parties to register to be notified when some action occurs or to modify a specific kind of data.

There are two types of extension points:

1. An "Action" might be used to notify interested parties that the configuration profile has been switched.
2. A "Filter" might be used to allow interested parties to modify spoken messages before they are passed to the synthesizer.

Handlers for extension points are always called in the order they were registered so that the order is determinate, which should make behaviour more predictable and easier to reproduce.

Handlers can be called with keyword arguments. If a handler doesn't support a particular keyword argument, it will still be called with the keyword arguments that it does support. This means that additional keyword arguments can be added to actions and filters in future without breaking existing handlers.

Finally, this adds a config.configProfileSwitched action and has synthDriverHandler, braille, etc. register for it instead of explicitly calling each handler from the config module. Aside from eliminating ugliness, this serves as a real world usage implementation.

Testing performed:

1. Unit tests for the extensionPoints module.
2. Tested with two different profiles with different synthesisers, braille displays and braille input tables. Switching between the profiles correctly switches synthesiser, braille display and braille input table.

Known issues with pull request:

While there are unit tests for Filter, There is no real world usage implementation, as I can't come up with one just yet and I didn't feel this was an appropriate PR in which to invent one. I have use cases in mind for later parts of speech refactor, though.

Change log entry:

Changes for Developers:

- There is now a new extensionPoints module which provides a generic framework to enable code extensibility at specific points in the code. This allows interested parties to register to be notified when some action occurs (extensionPoints.Action), to modify a specific kind of data (extensionPoints.Filter) or to participate in deciding whether something will be done (extensionPoints.Decider). (#3393)
- You can now register to be notified about configuration profile switches via the config.configProfileSwitched Action. (#3393)

[jcsteh]

@michaelDCurran, even though I've requested review from @feerrenrut, I think it'd be good if you could take a look at the general concepts and let me know whether this "fits", is future proof, etc. It should be sufficient for you to look at the docstrings for the extensionPoints.Action and extensionPoints.Filter classes, as well as the docstring for the callWithSupportedKwargs function.

[jcsteh]

@tspivey, given your work on NVDA Remote and several other add-ons which involve monkey patching, I'd also like your opinion as to whether you think this could cover your various use cases. Obviously, we don't have all the extension points implemented yet, but the hope is that this will make it very easy to implement them as needed.

[josephsl]

Hi, would it be possible to allow braille output to be manipulated before being sent when a filter is in effect? This allows, for example, the replacement of state labels with specific Unicode dot patterns. Thanks. Edit: Deleted quoted email message.

[jcsteh]

Oh, I should point out that yes, this is heavily inspired by Wordpress's actions and filters. :)

[jcsteh]

@josephsl commented on 10 Aug 2017, 16:04 GMT+10:

Hi, would it be possible to allow braille output to be manipulated before being sent when a filter is in effect? This allows, for example, the replacement of state labels with specific Unicode dot patterns.

The correct solution for that specific use case would probably just be replacing the states in braille.positiveStateLabels, etc. To answer your question, though, in theory, you could do this with a filter. However, a general braille output filter would not be the ideal solution here. Instead, we might have a filter which gets applied from getBrailleTextForProperties and gets passed the relevant arguments.

[derekriemer]

@camlorn

[michaelDCurran]

Looks great to me, at least from a usage point of view.

[feerrenrut]

I think h could have a better name, is it short for handler?

[feerrenrut]

I'm surprised this isn't an int being incremented?

[jcsteh]

I'm surprised this isn't an int being incremented?

No can do. The closure captures "called", and because it's a list, we can mutate the value itself. In contrast, an int is immutable. The only way to increment an int is to assign a new value to "called", but if we do this, this only affects the closure; the outer scope (the test itself where the assertion happens) won't get the new value of "called".

[feerrenrut]

Oh interesting, I didn't know that. So if it were an int it's essentially captured by value, whereas the list is captured by reference? In C# there is a concept of boxing of value types, is there a similar concept in python?

[jcsteh]

Technically, it's still captured by reference (as is everything in Python), but because the referenced object is immutable, it's conceptually captured by value. The same is true for floats, booleans, tuples and strings.

Not 100% sure what you mean by boxing, but you could do something like this if you wanted to be able to increment an int from an inner function:

def outer():
    class data:
        intVal = 0
    def inner():
        data.intVal += 1
    inner()
    return data.intVal

This would return 1. There are more elegant, reusable ways of doing this too such as creating a MutableInt class which you instantiate and each instance has a different value. That's probably more what you mean by boxing.

[feerrenrut]

what about where no defaults are provided, the same cases as test_supportsLessKwargs and test_supportsExtraKwargs but with def h(a, b)

[jcsteh]

what about where no defaults are provided, the same cases as test_supportsLessKwargs and test_supportsExtraKwargs but with def h(a, b)

Those would be positional arguments, which are passed completely unmodified, since this function is only supposed to mess with kwargs. Still, I've added a test to at least check that positionals do get passed in case the code gets modified and someone accidentally removes the *args bit.

[feerrenrut]

Is there some mechanism for an exception such as this to become known?

[jcsteh]

Exceptions get logged. There's no way to unit test this at present, though.

[LeonarddeR]

@jcsteh commented on 10 aug. 2017 08:03 CEST:

@tspivey, given your work on NVDA Remote and several other add-ons which involve monkey patching, I'd also like your opinion as to whether you think this could cover your various use cases. Obviously, we don't have all the extension points implemented yet, but the hope is that this will make it very easy to implement them as needed.

Although I'm not claiming to be @tspivey, I know the remote code base quite good due to my work on braille support. So here is my view on the matter.

Looking at the monkey patching code of NVDA Remote, extension points need to be implemented in NVDA's code base in the following places:

1. braille.BrailleHandler.setDisplayByName
2. synthDriverHandler.setSynth: This might become obsolete due to Refactor speech to support callbacks, speech commands in control/format fields, priority output, etc. #4877
3. Synthesizer speak and cancel functions
4. tones.beep
5. nvwave.playWaveFile
6. braille.BrailleHandler._writeCells
7. inputCore.InputManager.executeGesture

For most cases, actions are sufficient.

However, I wonder whether you could consider implementing a third type of HandlerRegistrar (an evaluator). This would be similar to an Action, but it breaks from the notifying loop as soon as a callback returns False. This would be helpful in the following situations:

1. An input gesture is executed on the remote system. Now, we monkey patch inputCore.InputManager.executeGesture, and whe controlling the remote system, it doesn't execute any braille display gesture locally. So, the add-on needs to notify the input core of this
2. Muting speech, beeps and sounds on the controlled system while it is being controlled. This helps in situations where you are using RDP to control a system, in which case you have audio output from the controlled machine that Interferes with the speech and sound sent via NVDA Remote.

I know this can be done using filters, but that feels a bit ugly. Unless you think we should just go the filter way here.

[LeonarddeR]

May be allow an optional exceptionsShouldAbort argument in the constructor of Filters, which defaults to False? I can think of situations where filters depend on each other, and when one filter handler fails, it results in unexpected behavior when the subsequent handlers are allowed to run.

[jcsteh]

Filters shouldn't explicitly depend on each other. That just isn't possible in an ecosystem where you can't know what filters will be installed. Of course an earlier filter affects a later filter by changing the value, but the later filter should not depend on specific behaviour provided by a particular earlier filter. For all you know, the earlier filter didn't get registered or a filter got registered in between them. If you want that kind of explicit dependency, the correct solution is to use two filters; e.g. a pre-process filter and a post-process filter.

[jcsteh]

@leonardder commented on 17 Aug 2017, 01:20 GMT+10:

Looking at the monkey patching code of NVDA Remote, extension points need to be implemented in NVDA's code base in the following places:

1. braille.BrailleHandler.setDisplayByName

This one worries me a little. How do you currently use this?

2. synthDriverHandler.setSynth: This might become obsolete due to Refactor speech to support callbacks, speech commands in control/format fields, priority output, etc. #4877
3. Synthesizer speak and cancel functions

Speech refactor will change a lot of the way this is done. I don't think we should be doing this at the synth level; it should be just above that.

However, I wonder whether you could consider implementing a third type of HandlerRegistrar (an evaluator). This would be similar to an Action, but it breaks from the notifying loop as soon as a callback returns False.

Your use cases make sense, but this concerns me a little because it explicitly means earlier handlers can completely stop later handlers from even having a chance to know something happened. I think I'd prefer to have something where return values are collected and the caller can then make a decision based on all the return values. This might be as simple as any(not result for result in results), which is a bit redundant for your use cases, but it does prevent the problem I'm talking about. We considered doing this for Actions, actually, but we couldn't come up with a good use case. @michaelDCurran, thoughts on this versus @LeonarddeR's Evaluator idea? (See his comment for use cases.)

[jcsteh]

I discussed this a bit with @michaelDCurran. We agree having something like this is probably okay as long as we're strict with terminology.

1. I think the term Evaluator might be a bit too loose, as that could refer to other types of evaluation other than just boolean. Does the term Decider work for you?
2. We think it should stop processing handlers once a handler returns True. If there are no handlers, it would return False. The reason for this is that it's easier to understand in terms of boolean logic; it works exactly the same way as a boolean "or".
3. The method would be named Decider.decide, unless someone has a better idea.

[derekriemer]

what if some actioner can ask to be notified regardless? That way there could be different classes of notification in a sense. maybe I don't understand the use case for this exactly.

[LeonarddeR]

@jcsteh commented on 17 aug. 2017 04:00 CEST:

1. I think the term Evaluator might be a bit too loose, as that could refer to other types of evaluation other than just boolean. Does the term Decider work for you?

Sure. I assume it will use any as you suggested in #7484 (comment) ? This makes sense.

2. We think it should stop processing handlers once a handler returns True. If there are no handlers, it would return False. The reason for this is that it's easier to understand in terms of boolean logic; it works exactly the same way as a boolean "or".

Could you elaborate on why this works exactly the same way as a boolean "or"? I'm not sure whether I can follow this. Personally I'd say, especially since the class is a Decider, that things should stop if one handler decided False. Note that many Microsoft functions return True on success, and False on error. Furthermore, if logic, which is used to dosomething if a predicate is True, always executes the statement in the if block when the predicate is True :) . Note that the code in the function that handles a decider would look like:

If not decider.decide():
    # do something

3. The method would be named Decider.decide, unless someone has a better idea.

I'd decide so.

[jcsteh]

@leonardder commented on 17 Aug 2017, 14:09 GMT+10:

I assume it will use any as you suggested in #7484 (comment) ?

No, actually. It will stop as soon as any handler returns True. (In contrast, using any would require us to collect all results first.)

Could you elaborate on why this works exactly the same way as a boolean "or"? I'm not sure whether I can follow this.

When you do "a or b or c or d", the first thing to return True stops the test and returns True (short-circuit evaluation). However, I just realised you could equally argue that when you do "a and b and c and d", the first thing to return False stops the test and returns False. :)

Personally I'd say, especially since the class is a Decider, that things should stop if one handler decided False.

I guess it depends which way you look at it. I'm suggesting that the Decider decides whether something should be done that otherwise wouldn't be (default is False). For example, should I block this gesture? Should I mute these sounds? You're suggesting that the Decider is deciding not to do something that otherwise would be done (default is True). For example, should I allow this gesture? Should I allow these sounds? I guess one reason for my argument is that if you're implementing a Decider handler, you want to change the behaviour; you are making NVDA decide to do something it otherwise wouldn't. I want to mute these sounds, I want to block this gesture.

If not decider.decide():
    # do something

That depends. In the case of muting sounds, it might actually be:

if shouldMuteSounds.decide():
	return # Get out early.

[LeonarddeR]

Thanks for this clarification, this helps a lot.

One additional possibility I'd like to bring up, is to allow to specify the predicate. Like:

# Stop if one of the handlers return 42
if no42.decide(42, *args, **kwargs):
    # We do not allow galaxy hitchhikers.

[LeonarddeR]

@jcsteh commented on 17 aug. 2017 02:10 CEST:

@leonardder commented on 17 Aug 2017, 01:20 GMT+10:

1. braille.BrailleHandler.setDisplayByName

This one worries me a little. How do you currently use this?

	def setDisplayByName(self, name, isFallback=False):
		result=self.orig_setDisplayByName(name,isFallback)
		if result:
			self.call_callbacks('set_display')
		return result

So, what this basically does is, it notifies the other end of the remote connection that another display has been selected. This is necessary to calculate a display size that fits both the controller and controlling braille display.

I'd propose an endpoint that is notified of braille display changes, which sends the name and number of cells for the display as kwargs. It could be triggered just before the log.info line in setDisplayByName

[jcsteh]

@michaelDCurran, would you remind reviewing this last commit implementing Decider?

[jcsteh]

@leonardder commented on 17 Aug 2017, 16:26 GMT+10:

One additional possibility I'd like to bring up, is to allow to specify the predicate.

I think this makes things potentially confusing, especially since at the end of the day, it's a boolean decision; it's either yes or no. I tried having a default= keyword argument specifying the default, but it gets really messy because returning None with a default of False would actually cause the decider to stop, even though None evaluates to False in boolean terms. At best, the documentation became insanely confusing.

In the end, I went with True being the default. It's a bit tricky to document that way, since you'd only implement one if you wanted to "prevent" default behaviour, even though the decide method decides whether something "should" be done. However, we generally prefer boolean attributes/properties/functions to reflect the positive case, not the negative, so I think I agree this is more consistent with the rest of NVDA.

[LeonarddeR]

I just noticed that lambda functions aren't supported, at least not when you do someDecider.register(lambda: return False). This is probably expected behaviour, but should this be documented?

[LeonarddeR]

@jcsteh: Discovered the following bug:

>>> def func(a, b):
... 	return a, b
... 
>>> extensionPoints.callWithSupportedKwargs(func, a=1, b=10)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
TypeError: func() takes exactly 2 arguments (0 given)

[jcsteh]

@leonardder commented on 26 Aug 2017, 03:12 GMT+10:

I just noticed that lambda functions aren't supported, at least not when you do someDecider.register(lambda: return False).

lambdas should work just fine. The reason this doesn't work is that you're not holding a reference to the lambda, so it dies as soon as the register call returns. From the RegistrarHandler docstring:

The handlers are stored using weak references and are automatically unregistered if the handler dies.

So, this should work:

l = lambda: False
someDecider.register(l)

Of course, as soon as l goes out of scope, it will die and thus be unregistered.

@leonardder commented on 26 Aug 2017, 18:28 GMT+10:

@jcsteh: Discovered the following bug:

>>> def func(a, b):
... 	return a, b
... 
>>> extensionPoints.callWithSupportedKwargs(func, a=1, b=10)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "extensionPoints.pyc", line 135, in callWithSupportedKwargs
TypeError: func() takes exactly 2 arguments (0 given)

I don't consider this to be a bug. The documentation is explicit about this only being for keyword arguments, not positional arguments. For the cases in which this is intended to be used, we do not want to risk positional and keyword arguments being conflated; there are just too many ways you can mess this up.

[LeonarddeR]

@jcsteh commented on 26 Aug 2017, 11:16 CEST:

I don't consider this to be a bug. The documentation is explicit about this only being for keyword arguments, not positional arguments. For the cases in which this is intended to be used, we do not want to risk positional and keyword arguments being conflated; there are just too many ways you can mess this up.

Agreed. However, this means that this function in its current form will be out of scope for what you suggested in #2385 (comment)

For example: "eventHandler.queueEvent("typedCharacter",focus,ch=ch)" will break if callWithSupportedKwargs would be used for the eventHandler.

[jcsteh]

No, it won't. Positional args are passed untouched. All you have to do is make sure any kwargs are declared as kwargs; e.g. def event_typedCharacter(self, ch=ch).

[jcsteh]

Oh, unless you mean it's currently declared without the =None; didn't check the code. I think we could just fix that; I'd argue it shouldn't have been positional in the first place.

[LeonarddeR]

@jcsteh commented on 27 aug. 2017 01:38 CEST:

Oh, unless you mean it's currently declared without the =None; didn't check the code. I think we could just fix that; I'd argue it shouldn't have been positional in the first place.

Yes, exactly.

What I'm intending to do is nesting the function call in a try statement, and try the callWithSupportedKwargs approach if the initial function call raised a TypeError. I don't think we should consider using callWithSupportedKwargs for every single event, as that might have performance implications due to unnecessary inspection going on under the hood.

[jcsteh]

Beware of premature optimisation. (I should know, I do it all the time. :) ) On the surface, I'd agree it seems logical this could have performance implications, but it's worth noting that exception handling can also have performance implications, especially in the case where an exception is caught. You'd need to benchmark this before you could be certain it was actually an improvement.

[derekriemer]

yay!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!