Shields: Add a code field for easier mapping of the state.

[pixlwave]

Following on from the conversation over on element-hq/element-x-ios#3051 (comment) this PR adds a code field to the ShieldState so that it can be easily mapped by apps that need to provide a translated description to the user.

Note: This PR also adds uniffi to the matrix_sdk_common crate to avoid a duplicate type in the FFI for the code.

[BillCarsonFr]

Draft for now more for discussion, this is an exploration following the conversation over on element-hq/element-x-ios#3051 (comment)

It makes the ShieldState strongly typed so clients don't need to switch over raw strings until a decision is made about how i18n should be handled. The string is still available via ShieldState::message (or message_for_shield_state in the FFI) so we're not losing anything here, plus the tests now assert that the right message is being chosen.

If this is considered a valid direction, then I would also like to suggest the idea of renaming ShieldState to something like EncryptionAuthenticity, considering shields are basically a UI element and it doesn't sound like we'll be using a shield icon for these warnings in Element.

Note: This PR also adds uniffi to the matrix_sdk_common crate to remove the duplicate mapping in the FFI.

As an alternative I was thinking about having the shield state having two fields, a code(machine readable) and a description human_readable string.
Like that you could do i18n on the code and if code is unknown just use the raw human_readable.
Could even be done in a non API breaking way for existing clients?

[pixlwave]

Updated to do it that way and have marked as ready for review to let CI run on it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.02%. Comparing base (aee8728) to head (d2c553f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3786      +/-   ##
==========================================
+ Coverage   84.00%   84.02%   +0.02%     
==========================================
  Files         259      259              
  Lines       27159    27156       -3     
==========================================
+ Hits        22815    22818       +3     
+ Misses       4344     4338       -6     

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

[jmartinesp]

LGTM, thanks!

[jmartinesp]

@BillCarsonFr does the new implementation work for you?

[pixlwave]

Force-pushed to rebase on main and add a missing code field in the test from #3788.

Edit: Helps if I actually commit the missing part 🤦‍♂️

[BillCarsonFr]

So yes, I think I prefer the human/machine readable approach.
I just let a comment regarding the sent in clear

[BillCarsonFr]

Sent in clear in an encrypted room is high level warning, should be red

[pixlwave]

Ah my bad, updated 👍

[BillCarsonFr]

If some client app persist that and then later we add a new code, how will this behave? When deserializing an old version of this enum?

[pixlwave]

I'm no Rust expert but the below seems to indicate that it will be fine if serialised to JSON:

use serde::{Serialize, Deserialize};
use serde_json;

#[derive(Serialize, Deserialize, Debug)]
enum Color {
    Red,
}


#[derive(Serialize, Deserialize, Debug)]
enum ColorPop {
    Yellow,
    Red,
    Green,
}


fn main() {
    let color = Color::Red;
    let json_string = serde_json::to_string(&color).unwrap();
    println!("Serialized color: {}", json_string); // Serialized color: "Red"
    
    let color: ColorPop = serde_json::from_str(&json_string).unwrap();
    println!("Deserialized color: {:?}", color); // Deserialized color: Red
}

How come the ShieldState is serialisable in the first place? Shouldn't the app always ask the SDK for the state to be up to date?

[poljar]

It might be for the WASM use-case where we sometimes, instead of adding a new type in the bindings which the SDK type gets converted into, we return a JSON object. This is using serde-wasm-bindgen.

If that's not the case, and nobody needs the implementations, perhaps we should just remove the Serialize and Deserialize implementations since they might have been cargo culted.

[pixlwave]

Got it, I didn't know that about WASM 👍