First attempt at basic axum-style state management #138

TTWNO · 2024-03-11T02:31:19Z

First attempt at Axum-style state management. The final PR should have:

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.

codecov · 2024-03-18T18:48:16Z

Codecov Report

Attention: Patch coverage is 0.28944% with 689 lines in your changes missing coverage. Please review.

Project coverage is 10.19%. Comparing base (fa09dab) to head (9a1e8e0).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
odilia/src/tower/state_changed.rs	0.00%	84 Missing ⚠️
odilia/src/tower/handlers.rs	0.00%	72 Missing ⚠️
common/src/cache.rs	1.69%	58 Missing ⚠️
common/src/command.rs	0.00%	53 Missing ⚠️
odilia/src/tower/cache_event.rs	0.00%	51 Missing ⚠️
odilia/src/main.rs	0.00%	48 Missing ⚠️
odilia/src/state.rs	0.00%	47 Missing ⚠️
odilia/src/tower/choice.rs	0.00%	45 Missing ⚠️
odilia/src/tower/service_ext.rs	0.00%	38 Missing ⚠️
odilia/src/tower/async_try.rs	0.00%	33 Missing ⚠️
... and 11 more

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.
📢 Have feedback on the report? Share it here.

odilia/src/main.rs

albertotirla

relatively good so far, except the few points I outlined in the comments which should now hopefully be visible

odilia/src/main.rs

odilia/src/state.rs

odilia/src/tower.rs

albertotirla · 2024-04-07T10:06:20Z

odilia/src/tower.rs

+
+pub struct Handlers<S> {
+	state: S,
+	atspi_handlers: HashMap<(String, String), Vec<BoxService<Event, Response, Error>>>,


perhaps break this down with a type alias or something? that type signature is very verbose

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

albertotirla · 2024-04-07T10:19:02Z

odilia/src/tower.rs

+		);
+		let input = ev.into();
+    // NOTE: Why not use join_all(...) ?
+    // Because this drives the futures concurrently, and we want ordered handlers.


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

I did mention in there that I can not have them compute concurrently, and why. The Vec<Command> may need to modify state.

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

A set of Commands 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.

but then, state still doesn't have to be updated in the handler, no? and then, event handlers can indeed run in paralell

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.

odilia/src/tower.rs

odilia/src/main.rs

albertotirla

alright, reviewed the latest changes and left some feedback

odilia/src/tower.rs

albertotirla · 2024-04-15T19:03:42Z

odilia/src/tower.rs

@@ -27,17 +33,71 @@ type Response = Vec<Command>;
 type Request = Event;
 type Error = OdiliaError;

+pub struct SerialFutures<'a, T, O> {


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

I like this! But we need concrete types since we need to return this from a function.

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.

odilia/src/tower.rs

TTWNO · 2024-04-18T08:44:40Z

Blocked on odilia-app/atspi#176

cargo fmt

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.

TTWNO · 2024-06-21T03:05:35Z

we should definitely get rid of the nightly requirement

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.

albertotirla · 2024-06-21T13:36:58Z

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?

TTWNO · 2024-06-21T14:19:22Z

our CI doesn't build nightly as far as I know

Fixed by a recent commit.

Debian, Ubuntu

We already depended on features less than 6 months old. This is only marginally different, and it's one feature.

keep unmerged until it lands in stable

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.

…convenience

…sted by running benchmarks

TTWNO marked this pull request as draft March 11, 2024 05:47

This was linked to issues Mar 16, 2024

Purify Event Dispatching and Handling #118

Closed

Use The Type System to Hold Cache Consistency #122

Closed

Use Axum-style State Management #133

Closed

TTWNO force-pushed the axum-style-handlers branch from fb58126 to 3e8392c Compare March 18, 2024 18:45

TTWNO force-pushed the axum-style-handlers branch 3 times, most recently from d19748f to 7c32b7b Compare March 21, 2024 17:15

albertotirla reviewed Apr 7, 2024

View reviewed changes

odilia/src/main.rs Outdated Show resolved Hide resolved

albertotirla requested changes Apr 10, 2024

View reviewed changes

albertotirla reviewed Apr 15, 2024

View reviewed changes

TTWNO added 17 commits April 26, 2024 13:31

Additional type fixups, handling of legacy cache item

ac469e7

cargo fmt

Desugar AFIT for send bounds enforcement

eac59eb

children_num i32 -> usize

11e1c28

Clippy

Re-add old text interfaces to cache

b2522e1

i32 -> usize for string positioning

00244c5

Adapt to i32 -> usize changes fmt

Add clippy exceptions for cache crate

83df5ac

Fix clippy issues

2c24792

Fix test compiles

a6cf72e

Fix deps

af3766c

Remove the old code in state

4dc666f

Fix tests

bcc54e1

formatting

6592710

Various cleanups

371145e

- Rename `get_children_caret` to `get_children_from_cursor` - Remove `async_trait` in favour of dep-less solution.

Change debug level, and use question mark syntax

e0a9830

Remove unused trait

f8f0849

Remove unused dep

40300f7

Remove config from state as it is unused

dd20e58

TTWNO added 26 commits June 25, 2024 19:52

Add StateChanged newtype with predicate trait implementations

3967432

Add new error for predicate failure

5882697

Add the refinement crate for additional types, and derived-deref for …

31e6a72

…convenience

fmt

694adba

Add state_changed crate to tower subcrate

1fa8224

fmt

3d5c9db

Add TODO on error variant

de67721

Add focused/unfocused handlers using new StateChanged<S, E> type

ef76cf6

Update lock file

c69f9da

Use latest atspi branch in object handlers

9d05d11

Fix bench imports

dbde75e

Update dashmap and use inlining for about a 10% speed improvement; te…

cbd80ab

…sted by running benchmarks

Fix formatting

abfb9d9

Add more type aliases for simplicity

e36b569

Use enabled field as bool

12d4897

Use bool as predicate for state changed instead of i32

8e61371

Rename predicates

60f834c

Use deref on CacheEvent

83a95c6

Use specific Focused/Unfocused types

3e4514e

Move CacheEvent into its own file; move Choice impls into choice

b82789e

Add CacheEvent<E> types

6512db9

Remove TryFromState for CacheEvent<E> from state

4a0171f

Use bleeding-edge atspi

cedfdb4

Use main atspi

5ffc25a

Only speak 'doc loaded' on focused applications

d70656a

Add specific nightly toolchain (for now)

9a1e8e0

albertotirla merged commit 6c50f0c into main Sep 1, 2024
6 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First attempt at basic axum-style state management #138

First attempt at basic axum-style state management #138

TTWNO commented Mar 11, 2024 •

edited

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading

albertotirla left a comment

albertotirla Apr 7, 2024

TTWNO Apr 10, 2024

albertotirla Apr 7, 2024

TTWNO Apr 10, 2024

albertotirla Apr 15, 2024

TTWNO Apr 16, 2024

albertotirla Apr 16, 2024

TTWNO Apr 16, 2024

albertotirla left a comment

albertotirla Apr 15, 2024

TTWNO Apr 15, 2024

TTWNO Apr 16, 2024

TTWNO commented Apr 18, 2024

TTWNO commented Jun 21, 2024

albertotirla commented Jun 21, 2024

TTWNO commented Jun 21, 2024

First attempt at basic axum-style state management #138

First attempt at basic axum-style state management #138

Conversation

TTWNO commented Mar 11, 2024 • edited Loading

codecov bot commented Mar 18, 2024 • edited Loading

Codecov Report

albertotirla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertotirla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TTWNO commented Apr 18, 2024

TTWNO commented Jun 21, 2024

albertotirla commented Jun 21, 2024

TTWNO commented Jun 21, 2024

TTWNO commented Mar 11, 2024 •

edited

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading