-
Notifications
You must be signed in to change notification settings - Fork 54
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
Allow LLEventPump
listener access to its own LLBoundListener
.
#2068
Conversation
`listen()` still takes `LLEventListener`, a `callable(const LLSD&)`, but now also accepts `LLAwareListener`, a `callable(const LLBoundListener&, const LLSD&)`. This uses `boost::signals2::signal::connect_extended()`, which, when the signal is called, passes to a connected listener the `LLBoundListener` (aka `boost::signals2::connection`) representing its own connection. This allows a listener to disconnect itself when done. Internally, `listen_impl()` now always uses `connect_extended()`. When passed a classic `LLEventListener`, `listen()` wraps it in a lambda that ignores the passed `LLBoundListener`. `listen()` also now accepts `LLVoidListener`, and internally wraps it in a lambda that returns `false` on its behalf.
Remove documented `LLEventPump` support for `LLEventTrackable`. That claimed support was always a little bit magical/fragile. IF: * a class included `LLEventTrackable` as a base class AND * an instance of that class was managed by `boost::shared_ptr` AND * you passed one of that class's methods and the `boost::shared_ptr` specifically to `boost::bind()` AND * the resulting `boost::bind()` object was passed into `LLEventPump::listen()` THEN the promise was that on destruction of that object, that listener would automatically be disconnected -- instead of leaving a dangling pointer bound into the `LLEventPump`, causing a crash on the next `LLEventPump::post()` call. The only existing code in the viewer code base that exercised `LLEventTrackable` functionality was in test programs. When the viewer calls `LLEventPump::listen()`, it typically stores the resulting connection object in an `LLTempBoundListener` variable, which guarantees disconnection on destruction of that variable. The fact that `LLEventTrackable` support is specific to `boost::bind()`, that it silently fails to keep its promise with `std::bind()` or a lambda or any other form of C++ callable, makes it untrustworthy for new code. Note that the code base still uses `boost::signals2::trackable` for other `boost::signals2::signal` instances not associated with `LLEventPump`. We are not changing those at this time.
This change will allow a lambda listener to decide for itself when it can stop listening, which is important for the Lua login API. |
/// Support a listener accepting (const LLBoundListener&, const LLSD&). | ||
/// Note that LLBoundListener::disconnect() is a const method: this feature is | ||
/// specifically intended to allow a listener to disconnect itself when done. | ||
typedef LLStandardSignal::extended_slot_type LLAwareListener; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like name 'aware' might be misleading for a self terminating listener. May be LLTemporaryListener?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, Temporary sounds to me too much as though we're somehow constraining its lifespan.
What I want to convey with the name is the extended signature. LLEnrichedListener
? LLExtendedListener
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLSelfReferentialListener
? LLReflexiveListener
? (not really better than the above I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLAwareListener
is fine enough for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLExtendedListener would at least match boost's naming, but comment there is good enough so I guess name wonn't matter as much.
listen()
still takesLLEventListener
, acallable(const LLSD&)
, but nowalso accepts
LLAwareListener
, acallable(const LLBoundListener&, const LLSD&)
.This uses
boost::signals2::signal::connect_extended()
, which, when thesignal is called, passes to a connected listener the
LLBoundListener
(akaboost::signals2::connection
) representing its own connection. This allows alistener to disconnect itself when done.
listen()
also now acceptsLLVoidListener
, and internally wraps it in a lambdathat returns
false
on its behalf.The tradeoff is that the (unused)
LLEventTrackable
feature is no longer supported.That claimed support was always a little bit magical/fragile. IF:
LLEventTrackable
as a base class ANDboost::shared_ptr
ANDboost::shared_ptr
specifically to
boost::bind()
ANDboost::bind()
object was passed intoLLEventPump::listen()
THEN the promise was that on destruction of that object, that listener would
automatically be disconnected -- instead of leaving a dangling pointer bound
into the
LLEventPump
, causing a crash on the nextLLEventPump::post()
call.Unfortunately, failing to meet the above conditions meant a silent failure, resulting
in the aforementioned crash.
The only existing code in the viewer code base that exercised
LLEventTrackable
functionality was in test programs. When the viewer calls
LLEventPump::listen()
,it typically stores the resulting connection object in an
LLTempBoundListener
variable, which more reliably guarantees disconnection on destruction of that variable.
The fact that
LLEventTrackable
support is specific toboost::bind()
, that itsilently fails to keep its promise with
std::bind()
or a lambda or any otherform of C++ callable, makes it untrustworthy for new code.
Note that the code base still uses
boost::signals2::trackable
for otherboost::signals2::signal
instances not associated withLLEventPump
. We arenot changing those at this time.