Skip to content
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

widget-driver: Support for sending future events through the widget api. #3600

Merged
merged 7 commits into from
Jul 10, 2024

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Jun 23, 2024

This adds support to parse widget actions that contain a future_timeout
as described in MSC matrix-org/matrix-spec-proposals#4157
those actions need to be handled using the custom send_future and state_future cs api endpoint described in: matrix-org/matrix-spec-proposals#4140.

This is supported by ruma and here we only handle the actions from the widget.

This is the core piece of code for that change:

/// Sends a given `event` to the room.
  pub(crate) async fn send(
      &self,
      event_type: TimelineEventType,
      state_key: Option<String>,
      content: Box<RawJsonValue>,
      future: Option<future::FutureParameters>,
  ) -> Result<SendEventResponse> {
      let type_str = event_type.to_string();
      Ok(match (state_key, future) {
          (None, None) => SendEventResponse::from_event_id(
              self.room.send_raw(&type_str, content).await?.event_id,
          ),
          (Some(key), None) => SendEventResponse::from_event_id(
              self.room.send_state_event_raw(&type_str, &key, content).await?.event_id,
          ),
          (None, Some(future)) => {
              let r = future::send_future_message_event::unstable::Request::new_raw(
                  self.room.room_id().to_owned(),
                  TransactionId::new().to_owned(),
                  MessageLikeEventType::from(type_str),
                  future,
                  Raw::<AnyMessageLikeEventContent>::from_json(content),
              );
              self.room.client.send(r, None).await.map(|r| r.into())?
          }
          (Some(key), Some(future)) => {
              let r = future::send_future_state_event::unstable::Request::new_raw(
                  self.room.room_id().to_owned(),
                  key,
                  StateEventType::from(type_str),
                  future,
                  Raw::<AnyStateEventContent>::from_json(content),
              );
              self.room.client.send(r, None).await.map(|r| r.into())?
          }
      })
  • Public API changes documented in changelogs (optional)

Signed-off-by:

@toger5 toger5 force-pushed the toger5/futures-widget-api branch 3 times, most recently from 1ae780a to c7c11e0 Compare June 23, 2024 14:43
@toger5 toger5 force-pushed the toger5/futures-widget-api branch 2 times, most recently from e6f2f48 to 0506014 Compare July 4, 2024 16:30
@toger5 toger5 marked this pull request as ready for review July 4, 2024 17:38
@toger5 toger5 requested a review from a team as a code owner July 4, 2024 17:38
@toger5 toger5 requested review from andybalaam and removed request for a team July 4, 2024 17:38
@toger5 toger5 force-pushed the toger5/futures-widget-api branch from 1edd88d to 1d3b270 Compare July 4, 2024 17:44
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 93.47826% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.28%. Comparing base (b80c2f7) to head (48ca240).
Report is 54 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk/src/widget/mod.rs 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3600      +/-   ##
==========================================
+ Coverage   84.27%   84.28%   +0.01%     
==========================================
  Files         259      259              
  Lines       26621    26658      +37     
==========================================
+ Hits        22435    22470      +35     
- Misses       4186     4188       +2     

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

@toger5 toger5 force-pushed the toger5/futures-widget-api branch from 043c9ac to fbb6589 Compare July 5, 2024 11:55
@toger5 toger5 requested a review from zecakeh July 5, 2024 21:00
Copy link
Member

@AndrewFerr AndrewFerr left a comment

Choose a reason for hiding this comment

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

LGTM but I'm not an expert on this codebase :)

crates/matrix-sdk/src/widget/machine/driver_req.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/from_widget.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/from_widget.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/from_widget.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/from_widget.rs Outdated Show resolved Hide resolved
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

I also don't know much about the widget code but this looks OK to me.

crates/matrix-sdk/src/widget/machine/tests/send_event.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/tests/send_event.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/tests/send_event.rs Outdated Show resolved Hide resolved
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Can you follow the contribution guidelines for good PR titles, please? 🥺

@toger5 toger5 force-pushed the toger5/futures-widget-api branch from d9fed69 to 99dacc6 Compare July 10, 2024 09:53
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Also, since future has a very specific meaning in Rust as a name, can you precise everywhere that future refers to a future event (rather than a Future as in async/await) instead, please? 🙏

@toger5 toger5 force-pushed the toger5/futures-widget-api branch from 99dacc6 to 20027c1 Compare July 10, 2024 10:02
@toger5 toger5 changed the title Support for sending futures events through the widget api. widget-driver: Support for sending future events through the widget api. Jul 10, 2024
@toger5 toger5 force-pushed the toger5/futures-widget-api branch from 20027c1 to 7ecb9d7 Compare July 10, 2024 10:04
@toger5 toger5 force-pushed the toger5/futures-widget-api branch 2 times, most recently from a5f9f6d to 95edb89 Compare July 10, 2024 11:12
@toger5 toger5 requested a review from bnjbvr July 10, 2024 11:44
crates/matrix-sdk/src/widget/machine/driver_req.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/from_widget.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/machine/from_widget.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/matrix.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/widget/mod.rs Outdated Show resolved Hide resolved
@toger5 toger5 force-pushed the toger5/futures-widget-api branch 2 times, most recently from b199a08 to 48c471b Compare July 10, 2024 15:00
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

rustfmt found a trailing whitespace 👀 But then, good to go, thanks!

@toger5 toger5 force-pushed the toger5/futures-widget-api branch from 48c471b to a92cb4b Compare July 10, 2024 16:00
@bnjbvr bnjbvr removed the request for review from zecakeh July 10, 2024 16:01
@bnjbvr bnjbvr enabled auto-merge (rebase) July 10, 2024 16:02
@bnjbvr bnjbvr disabled auto-merge July 10, 2024 16:02
@bnjbvr
Copy link
Member

bnjbvr commented Jul 10, 2024

(To run the rustfmt we're using in the codebase, use cargo xtask fixup style.)

…re events.

We need to disambigute future events and rust futures.
@toger5 toger5 force-pushed the toger5/futures-widget-api branch from a92cb4b to 48ca240 Compare July 10, 2024 16:03
@bnjbvr bnjbvr merged commit f4078fd into main Jul 10, 2024
40 checks passed
@bnjbvr bnjbvr deleted the toger5/futures-widget-api branch July 10, 2024 16:27
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