On Tue, Feb 28, 2012 at 1:00 PM, Tony Yu <ts...@gm...> wrote: > > > On Tue, Feb 28, 2012 at 11:44 AM, Benjamin Root <ben...@ou...> wrote: > >> On Mon, Feb 27, 2012 at 9:45 PM, Tony Yu <ts...@gm...> wrote: >> >>> >>> >>> On Fri, Feb 17, 2012 at 12:17 PM, Benjamin Root <ben...@ou...> wrote: >>> >>>> >>>> >>>> On Fri, Feb 17, 2012 at 11:06 AM, Ryan May <rm...@gm...> wrote: >>>> >>>>> On Fri, Feb 17, 2012 at 10:14 AM, Benjamin Root <ben...@ou...> >>>>> wrote: >>>>> > Hello all, >>>>> > >>>>> > I tracked down an annoying problem in one of applications to the >>>>> Lasso >>>>> > widget I was using. The widget constructor lets you specify a >>>>> function to >>>>> > call when the lasso operation is complete. So, when I create a >>>>> Lasso, I set >>>>> > the canvas's widget lock to the new lasso, and the release function >>>>> will >>>>> > unlock it when it is done. What would occassionally happen is that >>>>> the >>>>> > canvas wouldn't get unlocked and I wouldn't be able to use any other >>>>> widget >>>>> > tools. >>>>> > >>>>> > It turns out that the release function is not called if the number of >>>>> > vertices collected is not more than 2. So, accidental clicks that >>>>> activate >>>>> > the lasso never get cleaned up. Because of this design, it would be >>>>> > impossible to guarantee a proper cleanup. One could add another >>>>> > button_release callback to clean up if the canvas is still locked, >>>>> but there >>>>> > is no guarantee that that callback is not called before the lasso's >>>>> > callback, thereby creating a race condition. >>>>> > >>>>> > The only solution I see is to guarantee that the release callback >>>>> will be >>>>> > called regardless of the length of the vertices array. Does anybody >>>>> see a >>>>> > problem with that? >>>>> >>>>> Not having looked at the Lasso code, wouldn't it be possible to use >>>>> one internal callback for the button_release event, and have this >>>>> callback call the users' callbacks if points > 2 and always handle the >>>>> unlocking of the canvas? >>>>> >>>>> Ryan >>>>> >>>>> >>>> The problem is that the constructor does not establish the lock. It is >>>> the user's responsibility to establish a lock and release the locks for >>>> these widgets. Plus, if the user's callback has cleanup code (such as mine >>>> did), not guaranteeing that the callback is done can leave behind a mess. >>>> >>>> Now, if we were to change the paradigm so that the Widget class >>>> establishes and releases the lock, and that the user should never handle >>>> that, then that might be a partial solution, but still leaves unsolved the >>>> user's cleanup needs. >>>> >>>> Ben Root >>>> >>>> I just started looking at the Lasso widget for some other changes, and >>> I agree: the `points > 2` check should be removed. >>> >>> As for the design of Lasso, I don't quite understand why it disconnects >>> the callbacks after a single call. If the onpress event from the lasso >>> demo<http://matplotlib.sourceforge.net/examples/event_handling/lasso_demo.html>was made a method of Lasso, then you could reuse the Lasso widget instead >>> of having to create a new one for each selection. Of course, this might >>> complicate the widget lock used in the demo, but that's true for all other >>> widgets, right? >>> >>> -Tony >>> >>> >> The Lasso disconnects itself after the button_release event because >> that's what indicates that you are done. The user gets back a single >> Line2D object that is assumed to represent a single path with no breaks. >> Reusing the Lasso widget would be a situation that would require a >> different idiom for user interaction. I wouldn't be against a "MultiLasso" >> widget that works differently from Lasso, but I really wouldn't want to >> make changes to existing user widgets. It is iffy enough about whether to >> remove the point-count-check. >> >> Ben Root >> >> > Wouldn't this argument suggest that `RectangleSelector` and `SpanSelector` > should disconnect as well? I think their functionalities and their > behaviors are (or should be) pretty similar. > Hadn't thought about that, and I don't use them, so I hadn't noticed. > > In any case, I agree that changing this behavior of `Lasso` is not > desirable for compatibility reasons. I'll write up something that looks > more like `RectangleSelector` and `SpanSelector`. Maybe `LassoSelector`? > Actually, I like that idea. It gives a consistent naming scheme. Ben Root