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

Fix undeclared var "br" in Search bindings and function callbacks #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DiegoPino
Copy link

Same as pull: #49 but for master branch after merge with V 2.x

There are a number of places where variable br is called (used as a local context alias to this) without being declared or assumed as a global without being such. This breaks the binding between Search form and their callbacks. The only other workaround without this patch is to copy the affected methods and reimplement them in your own extension.

Seems like this is because of some left overs/partial update from an upstream merge.

at #49 there where some concerns:

  1. I think it would be better to change the br. to just this.

Probably in some places. The reason i did not go for that is that the current coding style( legacy but still present after the big merge) was to copy this context to a local scope var to ensure context separation and correct reference in case the overriding functions and build in callback scopes (even if partial) If br should be changed by this completely, some extra testing and a global find and replace could be needed but also a few are not removable at all ( all the ones refered by (https://github.com/internetarchive/bookreader/pull/49/files#diff-4817873ad739c71edef8536417a6c04dR4234)

  1. I am worried that this might not be correctly bound in some of these functions. Some of them appear to be callbacks, and might not have the right this.

Local tests passed: That is where the decision on keeping 1. as it is right now makes more sense: br is bound to this before the callbacks are defined. In that way, when used it is accessed as a local global with the correct parent context already saved as in https://github.com/internetarchive/bookreader/pull/49/files#diff-4817873ad739c71edef8536417a6c04dR4242 there the binding happens before the callback so br refers to the parent this.

Interested parties:
@rchrd2

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.

1 participant