Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added an @script decorator for setting script metadata to the scriptH… #6276

Closed
wants to merge 3 commits into from

Conversation

ctoth
Copy link
Contributor

@ctoth ctoth commented Aug 18, 2016

…andler module

This necessitated a change to the base ScriptableObject class, as now it has to check its methods fore defined gestures.
I added getGestures and getScripts to the ScriptableObject public API, as they are used in the constructor and seemed useful, though I can make these methods private if it is demed preferable.
Fixes #6266

…andler module

added a metaclass for collecting gestures on ScriptableObjects, this now supports getting from .gesture and .gestures set on script_ functions by the @script decorator.
Fixes #6266
continue
scriptName = name[7:]
if hasattr(script, 'gesture'):
gestures[script.gesture] = scriptName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree with having both gesture and gestures keyword arguments to the script decorator, I think the actual script functions should just have one "gestures" attribute. That is, the decorator should merge the two. Otherwise, everything that wants to look at a script's gestures has to check both attributes.

@jcsteh
Copy link
Contributor

jcsteh commented Aug 25, 2016

Otherwise, this looks good. Thanks!

Now passing gesture to the ```@script``` decorator will append the gesture to the list of gestures, rather than setting both gesture and gestures attributes on the function.
@jcsteh
Copy link
Contributor

jcsteh commented Aug 25, 2016

Ug. As discussed on IRC, I realised this won't work with overlay classes. The reason is that we actually mutate the class after construction to insert overlay classes. That means that self._gestures won't be updated for overlay classes, since the metaclass has already run. I see two possible solutions:

  1. Split out the loop in the metaclass and run it in DynamicNVDAObjectType as we process the added classes. See the code at around line 121 of NVDAObjects/__init__.py.
  2. Just have the metaclass add gestures to __gestures for this class only; i.e. don't do the MRO stuff. The code in the ScriptableObject constructor would be exactly as it is in current master. In other words, we're just plopping the new script decorator stuff on top of the existing framework. This is what I was originally thinking when we discussed the metaclass, but I didn't explain it well enough. I definitely prefer this solution, but I'm happy to discuss 1) if you think there are advantages.

@LeonarddeR
Copy link
Collaborator

@ctoth: I recall we discussed this pr some months ago but have forgotten the outcome of that discussion. Would you be able to address @jcsteh's comment? It might be good to have @jcsteh's comment also checked by a unit test, making sure that an object will also have the gestures added in an overlay.

@feerrenrut
Copy link
Contributor

Since some further action is required, and there hasn't been any movement on this for a while, I would like to suggest that if anyone is interested they can take it on. This looks like a beneficial change to the NVDA code base.

@LeonarddeR
Copy link
Collaborator

@feerrenrut: Happy to take this, but I have no rights to push to the original branch. @ctoth, would you be able to allow pushes from maintainers? If not, I"m afraid I'll just have to close this pr and open a new one. I agree that the code looks really useful.

@feerrenrut
Copy link
Contributor

@LeonarddeR closing this one, merging from the ctoth:t6266 branch and creating a new PR (linking back to this one) is probably the easiest/cleanest way to go about it. Thanks Leonard.

@LeonarddeR
Copy link
Collaborator

@jcsteh: May I ask for your assistance, as I can't get it working very well.

  1. Just have the metaclass add gestures to __gestures for this class only; i.e. don't do the MRO stuff.

Could you elaborate a bit more? Do you mean, instead of using cls._gestures, use cls.__gestures when adding the gestures from decorated scripts in the metaclass? I assume this will conflict when people would use both the script decorator and _-gestures on a class. Furthermore, it doesn't seem to work very well. The whole overlay class system is a bit hacky, and therefore I'm afraid we can't do anything else than what you suggested as per point 1:

  1. Split out the loop in the metaclass and run it in DynamicNVDAObjectType as we process the added classes. See the code at around line 121 of NVDAObjects/__init__.py.

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented May 2, 2018

Never mind, I have a working solution now that both respects self.__gestures and the gestures provided as part of a decorated script. I will first try to write proper unit tests before I'll file a new pr, this is really something that should be tested.

@LeonarddeR
Copy link
Collaborator

Superseded by #8236. @ctoth, again, many thanks for your work!

@LeonarddeR LeonarddeR closed this May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants