Eliminate the use of unscoped signal connections

[filip-hejsek]

Fixes #561.

To be resolved:

• Fix unscoped connections created in ToolTipManager::Bind
• Is adding the ConnectionScope helper class the right approach?
  • private or protected inheritance? A: private
• Require all connections to be scoped or allow exceptions? A: no exceptions
  • Making all connections scoped is obviously correct and doesn't require sophisticated reasoning
  • But some connections don't need to be scoped, like the one in LocalScriptManager
  • Preventing bugs seems more important than avoiding redundant Connection objects
  • A method could be added to UnscopedConnection to allow explicitly discarding it if we want to allow exceptions
• Replace more explicit Connection fields with BindConnection?

[filip-hejsek]

Forgot to mention that although this is marked as a draft, I would very much appreciate a review, at least to know if I took the right approach:

• I could have added a bunch of Connection fields to all the classes that need them, but I decided to reduce the boilerplate with a mixin class
• I made it a rule that all connections have to be assigned to a Connection object, even when it wouldn't be necessary, so that it's easier to reason about correctness of the code

[arch1t3cht]

Thanks a lot for working on this.

I made it a rule that all connections have to be assigned to a Connection object, even when it wouldn't be necessary, so that it's easier to reason about correctness of the code

Yes, I fully agree that this is the better way to go about it. In fact, if I understand signal.h correctly it could then be wise to get rid of UnscopedConnection (which may also make ConnectionToken unnecessary?) completely?

A method could be added to UnscopedConnection to allow explicitly discarding it if we want to allow exceptions

I can't think of any reasonable scenario where an exception needs to be made here. If some connection explicitly needs to live for, say, the entire program's duration then it should be a global variable or a member of AegisubApp.

I could have added a bunch of Connection fields to all the classes that need them, but I decided to reduce the boilerplate with a mixin class

Yeah, this looks good to me. The one worry I had was that this results in the destruction order of connections being unspecified now, but this should not actually be a problem since destroying a connection cannot call any signals.

Classes that need to update their connections during their lifecycle can keep a direct Connection member variable, but all connections that are purely tied to the lifecycle of the class holding them could go into a ConnectionScope.

private or protected inheritance?

I think private is better here. protected is discouraged in general and in this specific case I believe it (or public, too) has a chance of creating bugs in some edge cases: If a child class B of a class A inheriting from ConnectionScope itself binds a connection referencing members of B and the destructor of A calls a signal for that connection, then members of B would be used after B has already been destroyed (since the destructor of A is called after B is destroyed, but before ConnectionScope is destroyed).

This would probably not actually come up in practice, but it's probably better to be correct here. A consequence would be that child classes can no longer inherit from ConnectionScope (if their parent already did) and use BindConnection directly but instead need to add a ConnectionScope member, but that's not a big problem.

[filip-hejsek]

Yeah, this looks good to me. The one worry I had was that this results in the destruction order of connections being unspecified now, but this should not actually be a problem since destroying a connection cannot call any signals.

About the destruction order, if I'm being pedantic, I think even the current approach with Connection members is wrong, since they will only be destroyed after running the class destructor. Having callbacks called after entering the destructor (if the destructor makes a call that can result in a callback being called) doesn't seem like a good thing to me. Not sure what to make of this though, as we can't make the code run before the destructor.

EDIT: wxWidgets' Bind callbacks are also unregistered from the destructor, so they have the same problem.

[filip-hejsek]

I've removed the draft status because I think this is now in a mergeable state, but:

• If you want, I can rewrite it to avoid the ConnectionScope helper
• Or we can go all in and replace existing Connection fields with ConnectionScope (c94c749 is an example of that)