Skip to content

Conversation

@jmartinesp
Copy link
Contributor

@jmartinesp jmartinesp commented Dec 9, 2025

Changes:

  • Rename ffi::TimelineEventType to ffi::TimelineEventContent since it had the contents inside the enum, not just the type.

  • Create a new ffi::TimelineEventType enum with just the type info. Add missing cases like the multimedia ones. For this I had to add new features to the Ruma dependency.

  • Rename ffi::TimelineEvent::event_type to ffi::TimelineEvent::content to match the renaming done above.

  • Add ffi::RoomPowerLevels::events function, which returns a HashMap<TimelineEventType, i64>. All the previous changes are so we can use something type-safe for the keys, and also to fix the event type not containing only the type issue.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.55%. Comparing base (b3f6df9) to head (fa5f499).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5937   +/-   ##
=======================================
  Coverage   88.54%   88.55%           
=======================================
  Files         363      363           
  Lines      104020   104020           
  Branches   104020   104020           
=======================================
+ Hits        92106    92112    +6     
+ Misses       7537     7530    -7     
- Partials     4377     4378    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 9, 2025

CodSpeed Performance Report

Merging #5937 will not alter performance

Comparing feat/add-ffi-power-levels-events (fa5f499) with main (b3f6df9)

Summary

✅ 50 untouched

@jmartinesp jmartinesp force-pushed the feat/add-ffi-power-levels-events branch from 76067db to bda5084 Compare December 10, 2025 07:19
@jmartinesp jmartinesp marked this pull request as ready for review December 10, 2025 08:11
@jmartinesp jmartinesp requested a review from a team as a code owner December 10, 2025 08:11
@jmartinesp jmartinesp requested review from poljar and removed request for a team December 10, 2025 08:11
@jmartinesp
Copy link
Contributor Author

FWIW all of the refactoring was done so we don't pass a raw string around for the event types in the FFI layer. If we think this is an overkill we can just keep the first commit and ignore the rest, although I think the new changes make more sense.

@jmartinesp
Copy link
Contributor Author

jmartinesp commented Dec 10, 2025

It seems like #5931 it's doing something quite similar, even providing extra functionality on some parts 🤔 . However, there we now have ffi::TimelineEventType and ffi::FFITimelineEventType, which seems quite confusing. I'm wondering if we should try to unify both PRs.

@JoFrost
Copy link
Contributor

JoFrost commented Dec 10, 2025

Hey, author of #5931 here.

Indeed, it looks like our PRs are complementary. I had planned to make a second PR to expose the power level for any given event, and you’ve essentially covered that here. Your approach is cleaner, and I can use your changes as the base for #5931.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please just add a changelog entry and docs on the method.

Comment on lines +32 to +34
fn events(&self) -> HashMap<crate::event::TimelineEventType, i64> {
self.inner.events.iter().map(|(key, value)| (key.clone().into(), (*value).into())).collect()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs please, also it's a bit weird that this isn't pub when all of our other exported methods are.

@jmartinesp
Copy link
Contributor Author

@poljar I finally got the CC tests to pass in matrix-org/complement-crypto#219. Could you also approve those?

With this we can query the power level value for any event type
…ince it also contains the event contents for some of the types
…type

Use that for `RoomPowerLevels::events` instead.
@jmartinesp jmartinesp force-pushed the feat/add-ffi-power-levels-events branch from ead5621 to a10e290 Compare December 18, 2025 09:16
@jmartinesp jmartinesp force-pushed the feat/add-ffi-power-levels-events branch from a10e290 to fa5f499 Compare December 18, 2025 09:21
@jmartinesp jmartinesp merged commit 759c5a9 into main Dec 18, 2025
54 checks passed
@jmartinesp jmartinesp deleted the feat/add-ffi-power-levels-events branch December 18, 2025 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants