nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.09k stars 631 forks source link

Improve appModules to be able to maintain support for several versions of an app #10248

Open LeonarddeR opened 5 years ago

LeonarddeR commented 5 years ago

Is your feature request related to a problem? Please describe.

For many applications, we have to support many different versions. Most important and major examples of this are Microsoft Outlook and Microsoft Visual Studio. The outlook appModule, for example, contains very old code of which it is yet unknown whether it can be removed or not. If new code is introduced, it is hard to know whether it will break support for older versions of the application.

Additionally, applications like skype and poedit got entire rewrites.

Describe the solution you'd like

I think that using overlay/mixin classes for version specific functionality can solve this problem. This allows for much flexibility, i.e. you can provide generic functionality in the base appModule and add version specific workarounds in an overlay. I know that @JulienCochuyt ported the overlay classes support for NVDAObjects to the textInfos system as part of the WebAccess add-on. It should be possible to come up with an approach that is more general to AutoPropertyObject.

Describe alternatives you've considered

I think there are several alternatives to make it possible for an appModule to be version specific:

Additional context

JAWS introduced support for app version specific scripts pretty recently. Dolphin SUpernova has this for ages. It's not that we should blindly copy others, but it looks like the others have seen shortcomings in a system where one module has to support multiple versions of an app.

JulienCochuyt commented 5 years ago

I know that @JulienCochuyt ported the overlay classes support for NVDAObjects to the textInfos system as part of the WebAccess add-on.

Thanks for following my work on this subject. We indeed implemented on-the-fly overlay of both TextInfos and TreeInterceptor for this add-on. It is already pretty successful in corporate environments, but as you know, there still is quite some work to be done before a proper general public end-user launch.

It should be possible to come up with an approach that is more general to AutoPropertyObject.

I'm afraid that generalizing this potentially heavy process might lead to performance penalty.

  • Bundling Appmodule subclasses for multiple versions in one python module / package.

I would guess this is the simplest approach.

I think that, while at the design table, we should also take care to allow an add-on to extend an existing AppModule (#9845)

but it looks like the others have seen shortcomings in a system where one module has to support multiple versions of an app.

Indeed. Whatever the final design is, I am strongly in favor of the feature you wisely propose here.

LeonarddeR commented 5 years ago

Thanks for following my work on this subject. We indeed implemented on-the-fly overlay of both TextInfos and TreeInterceptor for this add-on. It is already pretty successful in corporate environments, but as you know, there still is quite some work to be done before a proper general public end-user launch.

I would say that it would be beneficial for you guys to have this in core, right? Could you outline some of the benefits that dynamic overlay classes give you for tree interceptors and textInfos?

It should be possible to come up with an approach that is more general to AutoPropertyObject.

I'm afraid that generalizing this potentially heavy process might lead to performance penalty.

Ah, I"m sorry. I wasn't intending to say that this should be functionality on AutoPropertyObject. Rather, a separate class/metaclass in baseObject that incorporates this. However, NVDAObjects still have some unrelated magic, such as automatically choosing the best api class for an object, which we wouldn't need for other object types, I think.

  • Bundling Appmodule subclasses for multiple versions in one python module / package.

I would guess this is the simplest approach.

Probably the simplest, yes. However, I"m afraid that it will cause problems at some point. Most importantly, it might be too prescribing. An overlay class approach would also allow the user to choose an overlay based on something unrelated to the version of an application. For example, an appModule could have a generic part, and overlay appModule classes for an UIA implementation or native implementation of a set of controls.

I think that, while at the design table, we should also take care to allow an add-on to extend an existing AppModule (#9845)

Note that you can already import built-in appModules using something like from nvdaBuiltin.appModules import firefox as originalFirefoxModule. That case doesn't apply to #9845, and I think especially this case is a good example of why overlay appModule classes could make sense, as in that case, the several overlays don't necessarily have to be aware of each other.

JulienCochuyt commented 5 years ago

Could you outline some of the benefits that dynamic overlay classes give you for tree interceptors and textInfos?

In short, we currently use this technique to

We previously implemented the first point in this list by other means, but dynamic overlays quickly unleashed the power to implement all the rest much more easily, both for no-code webModules and Python ones.

I would say that it would be beneficial for you guys to have this in core, right?

Sure, but keep in mind that we currently maintain backward compatibility to 2016.4 by contract, to 2016.3 effectively. This constraint will most likely be lessen in the future, though, and the sooner things evolve, the sooner they will be included in the oldest release we support.

An overlay class approach would also allow the user to choose an overlay based on something unrelated to the version of an application. For example, an appModule could have a generic part, and overlay appModule classes for an UIA implementation or native implementation of a set of controls.

Very good point. I'm convinced. Kind of behavioral overlays, but for whole applications rather than just controls.

Note that you can already import built-in appModules using something like from nvdaBuiltin.appModules import firefox as originalFirefoxModule

Shouldn't this be mentioned in the developer guide by the way? A lot of people (including my younger self) loose backward compatibility by copying the whole stock appModule just to extend it.

That case doesn't apply to #9845, and I think especially this case is a good example of why overlay appModule classes could make sense, as in that case, the several overlays don't necessarily have to be aware of each other.

Have you already thought about a concrete way to discover/register appModules overlays? I guess retaining backward compatibility to the current model is quite crucial not to break all appModule add-ons at once.

As a side note, appModule overlays also could offer a mean to support console programs appModules by allowing eg. to define a gesture that would add the overlay at will. As you maybe know, I've been working for some time on Vim support. The GUI version can be interfaced, but the console version provided with Git Bash cannot yet.

josephsl commented 5 years ago

Hi,

Extending built-in modules: yes, this can be done (I don't think it is documented in development guide either, but need to double-check; I know it is documented in the add-on development guide).

Version-specific code in app module classes: the way I do it is checking product version field and taking needed code path within methods of overlay classes (StationPlaylist Studio app module does this extensively not only because of behavioral differences, but due to app API differences between versions; emoji panel support in Core and my own add-on follows a similar story, but a better approach (UIA automation ID) was found which is awaiting another round of review). I once considered having a methods collection that represents routines found in a specific version of an app and choosing them at startup, but it was scrapped due to complexity.

As for web modules: I believe there is a GitHub issue (rather from Trac) that requests such a functionality - I recommend discussing the proposal for that one there (and we need a more descriptive name for such a tool anyway).

Thanks.

JulienCochuyt commented 5 years ago

As for web modules: I believe there is a GitHub issue (rather from Trac) that requests such a functionality - I recommend discussing the proposal for that one there (and we need a more descriptive name for such a tool anyway).

@josephsl, I'm sure there is, but the term webModule in my earlier comments refers to the equivalent of an appModule for web applications, as implemented in the WebAccess add-on. This is an add-on we maintain here at Accessolutions, developed in coordination with the French administration. It's already been successfully deployed in several corporate environments, but as mentioned earlier, it's not quite ready yet for general public end-user launch - as it's evolving much faster than documentation and required refactoring can keep up. Leonard and Michael already know about it because we had the opportunity to demonstrate a few of its potential at SightCity in 2018 in the NVDA workshop organized by Babbage.

LeonarddeR commented 5 years ago

@JulienCochuyt It turned out to be easier than I thought to implement this, it took me around half a day to create a working prototype. see https://github.com/BabbageCom/nvda/tree/overlay

The overlay system works for TextInfos, TreeInterceptors not tested.

Have you already thought about a concrete way to discover/register appModules overlays?

Just some brainstorming. I've been thinking about adding an extensionPoints.Action to every DynamicObject which is called findOverlayClassesRegistrar. findOverlayClasses can then call findOverlayClassesRegistrar.notify with the instance and the clsList. Handlers can add themselves to the clsList of an object if they so desire.

One advantage I see is that in the ideal situation, we could drop the chooseNVDAObjectOverlayClasses functions on globalPlugins. Especially on global plugins, it is currently a waste of time to throw every single object at a chooseNVDAObjectOverlayClasses handler that only handles a handful of cases. NVDA Remote is a good example of this, it has a chooseNVDAObjectOverlayClasses just because it needs to see when the remote desktop is active. It would be much cleaner if the NVDA Remote globalPlugin could register a handler for the SecureDesktop object instead.

A disadvantage might be that this is too powerful and that we really need to think about limitations. For example, appModules shouldn't be able to register overlays for objects that are created outside the app.

I'm seeing that we're getting in depth here without actually discussing this with the core devs. I'd like to point out that this is not at all set in stone, nor that I insist on having this feature in core. I however think that a system like this really increases the flexibility of NVDA scripting/programming.

Really interested in what @derekriemer, @jcsteh, @michaelDCurran and @feerrenrut think about this proposal.

derekriemer commented 5 years ago

We need to make sure we don't break backwards compat. Perhaps, we can make some sort of new style of addon, or addon api compat field, where if you are marked api-compatable with v2, or your minimum version is > x, we make it so you can no longer use chooseNVDAObjectOverlayClasses, and have to use the registrar.

On Wed, Sep 25, 2019 at 7:30 AM Leonard de Ruijter notifications@github.com wrote:

@JulienCochuyt https://github.com/JulienCochuyt It turned out to be easier than I thought to implement this, it took me around half a day to create a working prototype. see https://github.com/BabbageCom/nvda/tree/overlay

The overlay system works for TextInfos, TreeInterceptors not tested.

Have you already thought about a concrete way to discover/register appModules overlays?

Just some brainstorming. I've been thinking about adding an extensionPoints.Action to every DynamicObject which is called findOverlayClassesRegistrar. findOverlayClasses can then call findOverlayClassesRegistrar.notify with the instance and the clsList. Handlers can add themselves to the clsList of an object if they so desire.

One advantage I see is that in the ideal situation, we could drop the chooseNVDAObjectOverlayClasses functions on globalPlugins. Especially on global plugins, it is currently a waste of time to throw every single object at a chooseNVDAObjectOverlayClasses handler that only handles a handful of cases. NVDA Remote is a good example of this, it has a chooseNVDAObjectOverlayClasses just because it needs to see when the remote desktop is active. It would be much cleaner if the NVDA Remote globalPlugin could register a handler for the SecureDesktop object instead.

A disadvantage might be that this is too powerful and that we really need to think about limitations. For example, appModules shouldn't be able to register overlays for objects that are created outside the app.

I'm seeing that we're getting in depth here without actually discussing this with the core devs. I'd like to point out that this is not at all set in stone, nor that I insist on having this feature in core. I however think that a system like this really increases the flexibility of NVDA scripting/programming.

Really interested in what @derekriemer https://github.com/derekriemer, @jcsteh https://github.com/jcsteh, @michaelDCurran https://github.com/michaelDCurran and @feerrenrut https://github.com/feerrenrut think about this proposal.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nvaccess/nvda/issues/10248?email_source=notifications&email_token=ABI2FPOX4BJJZYBDGCDCWHLQLNRWFA5CNFSM4IYHSLWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7R44PA#issuecomment-535023164, or mute the thread https://github.com/notifications/unsubscribe-auth/ABI2FPPITOPRKXQCICZMBJLQLNRWFANCNFSM4IYHSLWA .

--

Derek Riemer: Improving the world one byte at a time!

Personal website http://derekriemer.com

derekriemer commented 5 years ago

Yeah, this is cool. I haven't seen exactly how you're doing the version specific stuff yet. I'll try to take another look soon.

derekriemer commented 5 years ago

Your DynamicScriptableClass has ... as its body. What's that doing?

LeonarddeR commented 5 years ago

We need to make sure we don't break backwards compat.

Yes, this is very important to me as well.

Perhaps, we can make some sort of new style of addon, or addon api compat field, where if you are marked api-compatable with v2, or your minimum version is > x, we make it so you can no longer use chooseNVDAObjectOverlayClasses, and have to use the registrar.

I think both methods can stay side by side. We can introduce the new behaviour in a similar way as how we introduced the script decorator. We want new style scripts to use the script decorator, but there's nothing holding you back from using the old system. We can at some point deprecate it and then remove it altogether with a major API change similar to threshold.

Your DynamicScriptableClass has ... as its body. What's that doing?

I believe it is similar to pass in a class body. It's just saying that the class does nothing special yet, other than being a wrapper class to deal with underlying meta classes. You can't inherit from multiple classes with different meta classes, as that causes a conflict on the meta class level.

LeonarddeR commented 4 years ago

@JulienCochuyt wrote:

* add a whole lot of dynamic key bindings such as place markers and the like

This might come in handy for #9527

jcsteh commented 4 years ago

First off, I'm still a huge fan of the NVDAObject overlay classes functionality. I have no regrets. It was a pain to implement, it's really quite nasty wen you think about it (mutating classes after construction? Seriously?), but it has stood the test of time and allowed us to do a lot of things we could never have reasonably done otherwise. All of that said:

I really worry that this is too complex for AppModules and there's a serious risk of over-engineering here. NVDAObjects are a bit of a different case because we are dealing with multiple APIs, common base behaviour we want to pull in for those different APIs, etc. Also, the decisions as to what you choose are much more complicated.

  1. Yes, there can be common code in AppModules for different versions of an app, but the sphere of influence is much less. We're talking about one app, as opposed to support for an entire accessibility API or a base behaviour which is used by many different apps, not to mention used by both core and add-ons.
  2. As you pointed out, there can be huge differences between versions of an AppModule. How much code will you just end up overriding most of the time? And if you do have common NVDAObject classes/code, why not just import them yourself from another module in your package?
  3. Ideally, there shouldn't be a huge amount of code in an AppModule anyway. Most of the really complicated stuff should be farmed out to NVDAObject implementations, at which point you can just import them as per point 2.
  4. We already get enough complaints about the complexity of writing add-ons for NVDA (at least, I assume we still do). I stand by NVDAObject overlay classes, but I think we want to be very careful about introducing more complexity.

I think it's worth considering whether this could be achieved more simply using (and perhaps extending) the getAppnameFromHost functionality introduced for Java, etc. (Take a look at appModules/javaw.py.) Do we really need to construct an AppModule to answer the kinds of questions we want to branch on here? Right now, you can't get the product version without instantiating, but we could add utilities to appModuleHandler to get around that. or we could have some sort of info dict passed to getAppnameFromHost which contains commonly needed stuff like product version.

An important disclaimer: this is just my opinion and I'm not sure how much it should count. For one, I'm no longer a core dev. Also, I've been out of touch with the "daily reality" of NVDA development for some time. Perhaps things are just getting too complicated for the kind of model I'm proposing and we do need something more advanced. That's entirely possible.

jcsteh commented 4 years ago

I've been thinking about adding an extensionPoints.Action to every DynamicObject which is called findOverlayClassesRegistrar. findOverlayClasses can then call findOverlayClassesRegistrar.notify with the instance and the clsList. Handlers can add themselves to the clsList of an object if they so desire. ... It would be much cleaner if the NVDA Remote globalPlugin could register a handler for the SecureDesktop object instead.

I do kinda like this idea, though. chooseNVDAObjectOverlayClasses was implemented well before extensionPoints. extensionPoints really does allow for much cleaner API in a lot of cases, and this might be one of them.

LeonarddeR commented 4 years ago

Thanks for sharing your opinion Jamie. While this issue was first opened specifically for appModules, I think the scope of it is now broader, as it also mentions using overlay classes for other objects, like tree interceptors, etc. I agree that the design of the overlay classes system has its complexity, and if we aim at changing the way how to develop appModules,add-ons/overlay classes, we should carefully think about how to make things easier instead of increasing the learning curve.