Fixed an issue where event handlers with no return value would block … - #60
Conversation
…any further event handlers from executing. This was caused due to the return value being undefined, and !undefined evaluates to true. Fixing that alone would have caused the event stack to never get smaller however, so an else was added to the if statement.
|
Thank you for your PR. I will review the change and test here (at a conference so may be a few days). Your fix and explanation looks pretty clear and tidy so I don't anticipate any issues. And congratulations on your first PR :) |
| catch(strExcept) { result=false; }//don't know whether to use true or false here | ||
| if(!result) { util.currentEvents.pop(); return false; } | ||
| if(result === false) {return false;} | ||
| else {util.currentEvents.pop();} |
There was a problem hiding this comment.
I don't think this is quite correct. I think what we want is:
if(result === false) { util.currentEvents.pop(); return false; }
Because otherwise the loop is going to possibly pop too many events off the currentEvent stack. It may also be important to review the existing event handlers to make sure this doesn't change their behaviour adversely where more than one event handler is registered. I'll check that separately.
There was a problem hiding this comment.
I think you are correct there. Looking back at my change, I think I only added the else statement because I misread how the function worked.
Unfortunately you won't be able to properly test behavior when multiple events are defined because that didn't work in the first place. Most UI functions called by events have no return value which meant that (!result) evaluated to true. This would make the callEvent function return before it finished executing all events in the array.
Using util.currentEvents.pop() didn't break anything in my own use of the keyboard, but I think that's only because the handlers are called directly from the events array instead of the currentEvents array. I'll change my local implementation to what you've suggested above and let you know if anything breaks. I suspect it will work fine though.
There was a problem hiding this comment.
Yes, looking at those UI event function callbacks, I see what you mean. Something that should be explicitly addressed in the future. We'll address that in a code review.
Once you can commit the change to your branch I'll merge the PR.
|
Made the change and pushed it. Don't know if I need to tell you about it but I'm new to this so I will just in case. |
|
Thank you -- I do get notified on pushes to a PR :) Merged now, preparing to push to live servers. |
remerge feat/improve css
2 participants
…any further event handlers from executing.
This was caused due to the return value being undefined, and !undefined evaluates to true.
Fixing that alone would have caused the event stack to never get smaller however, so an else was added to the if statement.
I opted to fix it this way when I realized that changing all of the calls to addEventHandler in the toggleUI to include a return statement just wasn't going to be a feasible solution.