-
Notifications
You must be signed in to change notification settings - Fork 17
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
First attempt at basic axum-style state management #138
Conversation
fb58126
to
3e8392c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #138 +/- ##
==========================================
- Coverage 18.90% 10.19% -8.72%
==========================================
Files 20 37 +17
Lines 984 1825 +841
==========================================
Hits 186 186
- Misses 798 1639 +841 ☔ View full report in Codecov by Sentry. |
d19748f
to
7c32b7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relatively good so far, except the few points I outlined in the comments which should now hopefully be visible
odilia/src/tower.rs
Outdated
|
||
pub struct Handlers<S> { | ||
state: S, | ||
atspi_handlers: HashMap<(String, String), Vec<BoxService<Event, Response, Error>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps break this down with a type alias or something? that type signature is very verbose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Clippy did tell me about this. Still working on exactly which types I want so I didn't want to make them aliased yet.
odilia/src/tower.rs
Outdated
); | ||
let input = ev.into(); | ||
// NOTE: Why not use join_all(...) ? | ||
// Because this drives the futures concurrently, and we want ordered handlers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use the futures_unordered crate? produces a stream of futures, which if I remember correctly, you can get the results of concurrently and in order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did mention in there that I can not have them compute concurrently, and why. The Vec<Command>
may need to modify state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this the purpose of this refactor, to remove state handling within handlers, but modify state externally outside the handler? if we modify global state in there, we're back to no testability or very hard testability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A set of Command
s are instructions on how to modify the state. Yes, the idea of this is to move the handling of state separate from the handling of actions. For example, an event may update speak some text, update the cache, fire a key event, or open a remote connection.
The implementations of these actions should be separate from their intention. If I want to speak x and y, then open a remote connection, that should be done through a set of commands, which are applied onto the state.
This way, we de-duplicate code, and de-couple the implmentation. So now, an addon can say "Hey, if I get this event, then I saw to beep at this frequency, or play this sound if some other condition is matched", and Odilia can respond without trying to give direct access to resources across FFI boundaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then, state still doesn't have to be updated in the handler, no? and then, event handlers can indeed run in paralell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State will (eventually) be applied within a single call to a boxed service. But, since getting partial state is required (mostly) for each function, the functions that need state will need to be run in series to avoid sending out copies of stale data. Imagine that F1 updates the currently focused item, and F2 depends on the state of the currently focused item. If run in parallel, these functions will both get a copy of the current focused item at time 0, F1 will then run, producing a command to update the currently focused item, then F2 runs saying to speak the currently focused item and its role. This will result in speaking the last focused item instead of the current one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, reviewed the latest changes and left some feedback
odilia/src/tower.rs
Outdated
@@ -27,17 +33,71 @@ type Response = Vec<Command>; | |||
type Request = Event; | |||
type Error = OdiliaError; | |||
|
|||
pub struct SerialFutures<'a, T, O> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from stackoverflow
you can directly use a stream of streams of future, where each future is converted to a stream and then flattened to force it to be poled to completion before anything else is. For that, you can use iter_ok, like so...
iter_ok(tasks).map(|f|f.into_stream()).flatten()
or this, where you get a single future which poles all of them to completion
let tasks = vec![ok(()), ok(()), ok(())];
let combined_task = iter_ok::<_, ()>(tasks).for_each(|f| f);
though I still question the doing of such serially, but this is an approach with mucbh less code, if it can be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! But we need concrete types since we need to return this from a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried really hard to use something more similar to this, especially in relation to not rolling our own SequentialFutures
. That does seem silly after all.
I got really close but got slapped by the borrow checker. I can come back to this one.
Blocked on odilia-app/atspi#176 |
Clippy
Adapt to i32 -> usize changes fmt
- Rename `get_children_caret` to `get_children_from_cursor` - Remove `async_trait` in favour of dep-less solution.
My argument against this is that the stabilization PR is already open and is one of the major hurdles in getting 2024 capture rules merged; the Rust team clearly has a pretty massive weight on them for this issue, and I have very strong faith this is very much in the "almost done" category—especially if it's going to be done by the end of this year. |
well, another issue is that our CI doesn't build nightly as far as I know for one, and for another, distros such as debian and ubuntu package relatively old versions of rustc, rustup, etc, and having to be on the peek latest and greattest for this feature isn't the best user experience, especially if one has to actively opt into nightly to build this. Then, perhaps we should delay merging this till it's stabilised in rustc, so that people don't have to use nightly or RUSTC_BOOTSTRAP=1 to build odilia? |
Fixed by a recent commit.
We already depended on features less than 6 months old. This is only marginally different, and it's one feature.
We are not production software and the difference between using a temporarily unstable feature will not be the difference between us and beung "production ready" in the next 6 months. It compiles, passes linting, and works. |
…sted by running benchmarks
First attempt at Axum-style state management. The final PR should have:
atspi::Event
atspi::Event
OdiliaCommand
OdiliaCommand
All should be able to implement some kind of state extraction, through types like the following:
LastFocusedItem
CurrentItem
CaretPosition
LastCaretPosition
Feel free to add additional items in the comments of this PR.
For right now, I've put together a basic
EventHandlers
type, that is able to connect up atspi events to various routers. At this time, I haven't seen how to add all the various parameters to a function yet, that's still to work on.I'm hoping this makes for a more readable codebase. It sort of hides some of the complexity.