Split ChildrenChanged/TextChanged events into distinct Inserted/Deleted events #194
Conversation
|
Well, scratch that! Looks like the tests caught an error! I had the kind "delete | delete/system" deserailize into the ChildrenInserted type, which indeed is a mistake. I'm going to |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
+ Coverage 86.69% 86.81% +0.11%
==========================================
Files 39 39
Lines 3337 3412 +75
==========================================
+ Hits 2893 2962 +69
- Misses 444 450 +6 ☔ View full report in Codecov by Sentry. |
TTWNO
changed the title
|
Good you managed to catch and mash this 'deleted' -> removed' bug! From the old event table @ Linux Foundatioon: 'kind' major:minor discriminants of events that communicate some change:
This does break the 1:1 relationship between signals' member and user facing event types doesn't it? One could argue that all events with minor discriminants warrant separate events.. |
| "ChildrenChanged" => Ok(ObjectEvents::ChildrenChanged(ev.try_into()?)), | ||
| "ChildrenChanged" => { | ||
| let body: EventBodyOwned = ev.try_into()?; | ||
| match body.kind.as_str() { |
There was a problem hiding this comment.
Are these discriminants correct?
In the old LF event table it says these minor strings were 'add' and 'remove' did that change?
| "TextChanged" => Ok(ObjectEvents::TextChanged(ev.try_into()?)), | ||
| "TextChanged" => { | ||
| let body: EventBodyOwned = ev.try_into()?; | ||
| match body.kind.as_str() { |
There was a problem hiding this comment.
As above, but for TextChanged, the discriminants may be 'inserted' and 'removed'?
| fn from(event: ChildrenInsertedEvent) -> Self { | ||
| EventBodyOwned { | ||
| properties: std::collections::HashMap::new(), | ||
| kind: "insert".to_string(), |
There was a problem hiding this comment.
Should this not be "add"?
There was a problem hiding this comment.
With busmonitoring I saw ChildrenChanged: "add/system", "remove/system" and "add", "remove"
So this should probably be "add" or "add/system"
| EventBodyOwned { | ||
| properties: std::collections::HashMap::new(), | ||
| kind: event.operation, | ||
| kind: "delete".to_string(), |
There was a problem hiding this comment.
Should this not be "remove"?
I may be misunderstanding things.
There was a problem hiding this comment.
Okay, I just tried a bit of monitoring on the bus
For ChildrenChanged
Path=/org/a11y/atspi/accessible/12747 Interface=org.a11y.atspi.Event.Object Member=ChildrenChanged
"remove/system" , "add/system", "add", "remove"
Perhaps this be "remove", no?.
|
Indeed this is a case of an X/Y problem: I've proposed a solution without explaining why. In Odilia, events match based on a type (enum discriminant/member+method). Since what needs to be done is significantly different depending on inserted/deleted, it would make sense (for Odilia) if we could write event handlers like this: async fn my_handl(
ev: CacheEvent<ChildrenRemovedEvent>,
) -> impl TryIntoCommands {
// ...
}This is a minor case with
Funny you mention that. I was actually thinking about a way to be generic over different Imagine the following code: async fn focus_item(
ev: CacheEvent<StateChangedEvent>,
) -> impl TryIntoCommands {
if ev.inner.state != State::Focus { /* do I return Ok(vec![]) or Err(...) here? */ }
if ev.inner.enabled != true { /* it's not an error, but the function should not continue running... */ }
}Compare it to encoding the information at the type level: async fn focus_item(
ev: CacheEvent<StateChangedEvent<State::Focus, true>>,
) -> impl TryIntoCommands {
// ...
}This way, the function only gets called when it's already known as the right event. Presumably I could find a way to do this with generics of |
I think this works better than adding an enum like "Inserted/Deleted"; they really are separate events. This is obvious if you look at the name of the other events like "ColumnInserted"/"ColumnDeleted", etc. The only thing I'm curious about is how to test this; our automated tests do not allow modification of the body. We can add manual test as well: of course there is nothing wrong with that!
Commits:
TODOs: