There's one screw case: If the user sets scrollevents manually, and then adds and removes a listener, scrollevents will get turned off. So I think you need an internal flag 'scrollEventsSetManually' or something, that gets ORed in to the notifier's setting, to make sure it stays on.
Otherwise, approved! On 2010-06-24, at 21:07, Max Carlson wrote: > Change 20100623-maxcarlson-H by maxcarl...@friendly on 2010-06-23 18:10:50 PDT > in /Users/maxcarlson/openlaszlo/trunk-clean > for http://svn.openlaszlo.org/openlaszlo/trunk > > Summary: UPDATED: Automatically turn on scrollevents when dependent events > are registered > > Bugs Fixed: LPP-9146 - Use notifyingevents to automatically turn on scroll > events on text > > Technical Reviewer: ptw > QA Reviewer: hminsky > > Details: Updating to address Tucker's comments: > 1) Since LzTextScrollEvent is so intimately tied with LzText, I would prefer > it were _not_ in a separate file. If we had inner classes, I would make it > one. We don't, but at least let's not put it in its own file because that > gives the illusion it is standalone. > > Done. > > 2) Rather that LzTextScrollEvent having to know all the the potential set of > scroll event listeners, I suggest we just add a slot to LzText like > `scrollEventListenerCount`. Each time a LzTextScrollEvent gets a notify, it > does > > scope.scrollEventListenerCount += (ready ? 1 : -1) > var anyready = scope.scrollEventListenerCount > 0; > if (anyready != scope.scrollevents) { > scope.$lzc$set_scrollevents(anyready); > } > > Done. > > 3) LzText#344 "NOTE: Using the onyscroll" instead of "using" say something > like "a constraint on `yscroll`... or a handler for `onyscroll`... will > automatically enable `scrollevents`. Ditto for the other NOTEs in each of > the attributes. A constraint or handler will automatically enable updating > of the attribute. The only time you need to set scrollevents directly would > be if you were, say, trying to poll the attribute without a constraint or > handler (which would be very un-Laszlo-rific). > > Done. > > 4) If you make LzTextDeclaredScrollEvent a static property of > LzTextScrollEvent, then you should be able to declare the events in the > class. It really is a shame to re-initialize them in every instance. If > that doesn't work, just make it a global, like we do with LzDeclaredEvent. > > I was able to find a compromise that works across runtimes. Done. > > 5) Let's just delete `maxlines`. It is an attractive nuisance. > > Done. > > Otherwise the same: > > LzText - Create shared LzDeclaredEventClass instance for use with events that > need scrollevents turned on. Update documentation. Remove unused > methods/properties. Add LzTextscrollEvent class - registering for any > dependent event turns on scroll events, and unregistering checks for other > dependent events with listeners before unregistering. > > newcontent/scrollingtext - No need to explicitly turn on scrollevents. > > Tests: Debugger works as before. > > Files: > M WEB-INF/lps/lfc/views/LzText.lzs > M lps/components/debugger/newcontent.lzx > M lps/components/debugger/scrollingtext.lzx > > Changeset: > http://svn.openlaszlo.org/openlaszlo/patches/20100623-maxcarlson-H.tar
