Remove unused ShieldStateCode::SentInClear#5959
Conversation
richvdh
force-pushed
the
rav/clean_up_shield_state
branch
from
239eebb to
1344cb5
Compare
CodSpeed Performance ReportMerging #5959 will not alter performanceComparing Summary
|
richvdh
force-pushed
the
rav/clean_up_shield_state
branch
from
1344cb5 to
e871001
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5959 +/- ##
==========================================
- Coverage 88.58% 88.57% -0.02%
==========================================
Files 363 363
Lines 104268 104282 +14
Branches 104268 104282 +14
==========================================
- Hits 92371 92366 -5
- Misses 7536 7555 +19
Partials 4361 4361 ☔ View full report in Codecov by Sentry. |
|
This will need complement-crypto updates, but maybe someone could give it a review for sanity before I invest time in that. |
stefanceriu
left a comment
stefanceriu
left a comment
There was a problem hiding this comment.
Makes sense to me 👍 I'll leave it to you to deal with my comments in whatever way you see fit.
| impl From<ShieldState> for TimelineEventShieldState { | ||
| fn from(value: ShieldState) -> Self { | ||
| match value { | ||
| ShieldState::Red { code, message } => { |
There was a problem hiding this comment.
Random point: I've said this before and I'll keep saying it every time I see these things: Red and Gray have no intrinsic meaning and depending on the culture you come from might mean the exact opposite (e.g. China). Naming them after a color does by no means guarantee that they will actually be colored that way in the UI (being in the UI crate makes it worse). I would much rather see them call for what they are BigProblem, LegacyProblem, NoProblem or whatever. Thanks for coming to my TED Talk.
There was a problem hiding this comment.
yup, absolutely. It's also worth noting that we don't actually use shields in the UI for this (rather, padlocks, /!\ symbols, anything other than a shield), so the name is crap twice over.
But again, I didn't really want to do a big refactor here.
There was a problem hiding this comment.
omg, didn't even think of that 🤦
richvdh
force-pushed
the
rav/clean_up_shield_state
branch
from
8660615 to
dace4da
Compare
The `ShieldState` enum has a `None` variant, so we don't need an `Option` on top of it.
Again, there is no need for an `Option` as well as a `None` variant
Since this can't be localised, apps shouldn't be using it.
richvdh
force-pushed
the
rav/clean_up_shield_state
branch
from
65fa14d to
5df4ddf
Compare
Separate the shield types between common and UI, so that we can change common without breaking UI. The new type does not include a `message` field: since it cannot be localised, clients should not be using it.
richvdh
force-pushed
the
rav/clean_up_shield_state
branch
from
5df4ddf to
b5f2128
Compare
matrix-org/matrix-rust-sdk#5959 has changed the API for `LazyTimelineItemProvider::get_shields`. Update accordingly.
|
matrix-org/complement-crypto#220 contains the fixes to complement-crypto |
|
This will need a matching complement-crypto PR. |
It has one, at matrix-org/complement-crypto#220. |
matrix-org/matrix-rust-sdk#5959 has changed the API for `LazyTimelineItemProvider::get_shields`. Update accordingly.
Some typos in the #5959 changelog
Some typos in the #5959 changelog
3 participants
VerificationState::to_shield_state_{strict,lax}return a typeShieldState, whose inner typeShieldStateCodecurrently includes a variantSentInClearfor "unencrypted event". This is very misleading, because those functions never actually set that variant.Rather, there is a separate method in
matrix-sdk-ui,EventTimelineItem::get_shield, which uses the same type, but does set that variant where appropriate.As a user of matrix-sdk-common, without the matrix-sdk-ui layer, this is dangerously misleading: it gives the impression that we are checking for unencrypted events, when in fact we are not.
The solution seems to be to use different types for the different levels of the stack.
While we're at it, we fix up some of the confusion of methods that return an
Optionof an enum type which itself has aNonevariant.