Make signal API identify signals by their signal data and not arbitrary strings #1495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The old signal API relies on plugins connecting to the same signal as described by its signal name. This is very error-prone: 1. Typos can be hard to debug. 2. If the signal changes/is removed/was not added in this version of Wayfire, everything may still compile. 3. We need to cast the untyped signal data to the proper type. 4. We need to match the string name to the type. C++'s templates offer a good solution by allowing us to replace string signal names with structs of different types and identifying different structs by their type_index. With the new API, we know at compile time which signal the connection_t should connect to, which effectively eliminates any duplication (e.g. manual casts, specifying the stringified signal name, etc). This also results in more efficient signals, as we do not have to hash a string every time, instead, we hash only a type_index. One last consideration which has also been painful in the past is mapping all structs to C or other languages. The new signal structs should no longer use inheritance and also export an optional `::cname` static member so that a signal is emitted to special callbacks in a signal provider. These callbacks can be used by C or other languages to connect to all signals on a given provider.
The syntax is a bit cleaner.
Nobody seems to be needing it, so remove it. It can always be reintroduced later when there are actual users for it.
Otherwise, the CI breaks, as Alpine does not have execinfo anymore. If not present, we cannot generate a backtrace, so we just print that.
ammen99
force-pushed
the
signal-revamp
branch
from
4f25189
to
517e880
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@Javyre thoughts ?
Eventually, this should fix #1307
If anybody is interested in why I want to do this change, some of the rationale is described here: 2274148
Also, this new implementation can be improved even further to use static storage for different signals (thus eliminating the unordered_map lookup), also we can use a simpler
safe_vector_t-like type to make iteration cache-efficient and lower the overhead of signals even further, if need be. Currently, signals are not that critical for performance and I have opted for a simpler (and a bit slower) implementation.