-
Notifications
You must be signed in to change notification settings - Fork 104
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
feature: remove mir cookie #3176
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3176 +/- ##
==========================================
- Coverage 77.85% 77.81% -0.05%
==========================================
Files 1070 1062 -8
Lines 74893 74626 -267
==========================================
- Hits 58311 58072 -239
+ Misses 16582 16554 -28 ☔ View full report in Codecov by Sentry. |
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.
This looks good to me.
We might want something similar for xdg-activation, but since that has a client-requests-token -> server-generates-token loop we shouldn't need any of the cookie-for-every-event infrastructure this removes.
If some of the code turns out to be useful there, we can easily fish it out of history.
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.
- This breaks ABI
We need to do some ABI bumping then. I don't mind if that is a separate PR, but needs to follow shortly.
As most of our libraries use events, I imagine they all need bumping. (mircore is probably OK)
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 think this header belongs in include/common
, not include/core
- see where the function is implemented.
@@ -411,7 +411,6 @@ def _report(publish, symbol): | |||
miral::toolkit::mir_input_event_get_pointer_event*; | |||
miral::toolkit::mir_input_event_get_touch_event*; | |||
miral::toolkit::mir_input_event_get_type*; | |||
miral::toolkit::mir_input_event_has_cookie*; |
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.
We should be bumping ABI, not dropping symbols without changing soname
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.
Yeah this might be one that I don't understand. Should I leave all of the .map
files unchanged, or am I supposed to remove the symbols from them now?
Also, is there any simple mechanism for bumping the so names that need to be bumped?
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.
This looks good except for the symbol/soname versioning. That is addressed in a dependent PR: #3178.
I think the best approach is to merge that into this PR before landing. But marking my approval
Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
@AlanGriffiths FYI, it appears that the failing Fedora tests are failing on main too. |
What's new?