Skip to content
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

Purify Event Dispatching and Handling #118

Closed
TTWNO opened this issue May 16, 2023 · 3 comments · Fixed by #138
Closed

Purify Event Dispatching and Handling #118

TTWNO opened this issue May 16, 2023 · 3 comments · Fixed by #138

Comments

@TTWNO
Copy link
Member

TTWNO commented May 16, 2023

In an effort to make state management teste, I've realized that directly modifying global state may be somewhat problematic in the long term.

This issue is to "purify" each function into pure logic, then apply a vector of modification events returned from each event handling function.
This makes Odilia much easier to test and extend.

Related to: #117, #108, and depends on odilia-app/atspi#82 being merged.

@TTWNO
Copy link
Member Author

TTWNO commented Jun 13, 2023

Here is an example of how this should work:

Instead of a function like update_string_insert(start_pos, update_length, updated_text) -> impl Fn(&mut CacheItem) + '_ this would become: update_string_insert(text_caret_moved_event) -> OdiliaEvent.

enum OdiliaEvent {
    ...
    Cache(OdiliaCacheEvent),
}
struct OdiliaCacheEvent {
    InsertAt(usize, String),
    RemoveFrom(usize, usize),
}
...

This creates a well-defined, easily testable surface area for the screen reader. We can know instantly if we've covered a certain case in our testing, how extensively it's been tested, and we don't have to check state directly (for example: checking each field in the state structure, checking that each item is changed as expected, and that no other changes were made.

Yes, we will also need to test functions which apply these effects to the state structure, but, importantly, we have already tested that the functions return the right results separately from testing that the effects are applied correctly. This separates the responsibility of the various tests, and makes it so we don't have to do the work of spinning up atspi and SSIP connections, etc. for a test of the internal functionality of a function.

Hopefully this makes sense. It'll be a ton of work, but I am absolutely convinced of this for the long-term.

@TTWNO
Copy link
Member Author

TTWNO commented Jun 13, 2023

This effectively will turn the majority of the Odilia codebase into a bunch of mapping functions from atspi events to Odilia events.

Then, state management would simply be the execution of the Odilia events. Each are tested separately.

Finally, since we have our codecov bot, any new events that are created in AT-SPI that Odilia accepts and deals with will need to have tests written for it before it can be merged, and if Odilia would like to introduce its own new internal event, it would also need to be tested before the PR can be merged. This should keep code coverage high across many contributions to come.

@TTWNO
Copy link
Member Author

TTWNO commented Jun 13, 2023

This would also make Odilia a very "accessible" codebase (pun not intended) for newcomers, since the implementation details of how to do something can quite easily be hidden from first-time implementers who want to, let's say, introduce a new type, or change the parameters in an Odilia event to cover more of the problem space.

They could modify one function, let's say "text changed" again, for example text_changed_handler(TextChangedEvent) -> OdiliaEvent, add new or modify existing tests, and call it a day—with no exposure to the direct implementation details of how something is spoken, for example.

@TTWNO TTWNO mentioned this issue Jun 13, 2023
7 tasks
@TTWNO TTWNO linked a pull request Jun 23, 2023 that will close this issue
11 tasks
@TTWNO TTWNO linked a pull request Mar 16, 2024 that will close this issue
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant