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

Consolidated data store #859

Closed
7 of 20 tasks
dalyIsaac opened this issue Apr 4, 2024 · 0 comments
Closed
7 of 20 tasks

Consolidated data store #859

dalyIsaac opened this issue Apr 4, 2024 · 0 comments
Labels
core Whim refactor The code can be simplified/moved/cleaned up

Comments

@dalyIsaac
Copy link
Owner

dalyIsaac commented Apr 4, 2024

Background

Currently, Whim has Managers. Each Manager will store items of their current type (e.g., IWorkspace). There is also a Butler, which stores mappings of IWindow to IWorkspace to IMonitor. Each Manager and the Butler also expose methods for updating the stores and events for subscribing to changes.

This intermingling of data and business logic is getting messy to manage. Additionally, working on #839 has surfaced that any thread can mutate data, and locking down the entry points for these can is tiresome given the large surface area of the API.

Proposals

Proposal 1

Redux-like structure composed of slices, reducers, selectors and dispatchers.

Store

The store will contain slices. Slices will include:

  • Windows
  • Workspaces
  • Monitors
  • Window <-> workspace mappings
  • Workspace <-> monitor mappings
  • Commands
  • Filters

Unlike Redux, store will not be immutable and will not require items to be serializable. However, it will appear as immutable outside Whim's core.

It's contents will be immutable. As a result, IWorkspace, IWindow, and IMonitor will lose their mutability. This will help prevent sneaky mutations of items which need to somehow trigger events.

The store will be secured by a reader-writer lock.

Reducers

Whim's "reducers" won't be reducers in the sense of Redux reducers - they won't result in a new store instance each time. However, they will still contain the business logic of updating the store.

Reducers will be the write portion of a reader-writer lock.

Selectors

Selectors will be responsible for exposing the internals of store in a thread-safe manner. They may appear as one of:

  • Redux-style selectors with functions
  • Class getters
  • Class methods on store

Dispatchers

Dispatchers will be responsible for sending Transforms to be passed to the reducers. Dispatchers will not be thread-safe.

Events

There will be a mechanism for listeners to subscribe to events and get notified of changes. Events will include payloads. Event listeners should be able to:

  • Subscribe to all notifications from a specific event, including out of date payloads
  • Subscribe to a specific event, but:
    • take the latest value if another thread updated the value, or
    • not receive the notification if the item has been removed.

Future Work

  • This will likely simplify work required for IPC #670.
  • STA per plugin?

Concerns

  • Whim's state will necessarily span across multiple slices. Updating data across multiple slices with a single Transform is unpleasant. Redux solves this issue by having every reducer be passed actions. However, I think the coupling between Transform and multiple reducers is unpleasant.

Proposal 2

  • Single immutable store with reader-writer lock.
  • Reducers with dispatchers
  • Manually written pickers

Concerns:

  • A single store is unscalable.

Proposal 3

  • MVC model

Concerns:

  • Mapping between windows/workspaces/monitors would still be messy
  • It's more complicated to lock.

Proposal 4 (selected)

  • Have individual Transform records which implement an Execute method
  • Each slice is public, but individual fields are internal set.
  • Slices can have helper methods to get data.

Overview

Slices:

  • Slices are portions of the state of Whim.
  • Slices can only be updated by transforms.
  • Each slice represents a logical domain.

Transforms:

  • Transforms describe how the slices should be updated.
  • Transforms are executed on the store.

Picking:

  • Similar to transforms, pickers describe how to fetch data.
  • Picking is performed on the store.

Events:

  • Event managers listen to operating system events and dispatch transforms.

Misc:

  • Whim's state is protected by a reader-writer lock.
  • Slices are named as singular.

Implementation

  • MonitorSlice #870
  • WindowSlice #871
  • WorkspaceSlice #872
  • MapSlice #873
  • Multi-threading #874
  • Rename Slice to something so that SliceLayoutPlugin doesn't get confusing
  • Change as many pickers as possible to be PurePicker implementations
  • Update IRootSector to name sectors as *Sector
  • Handle unexpected events in DispatchEvents
  • Queue events and DoLayout for nested transforms
  • Switch from interfaces to records?
  • Rename Store to something else? Transforms are more generic actions now, and aren't pure. Maybe remove Store and put onto IContext?
  • Implement pickers are part of the Pickers static class.
  • Order the parameters of the transforms for consistency
  • Decide on whether to use Result (the map still uses nullable types)
    • Gone with Result - it's more expressive
  • Update docs.
  • Deprecate APIs.
  • Create static classes which expose the constructors for the transforms and pickers?
  • Command to open a workspace on a given monitor - see discussion 883.
  • Set a router to open a window in a workspace and open it in the correct monitor - see discussion 883.
  • Remove deprecated APIs.
@dalyIsaac dalyIsaac added core Whim refactor The code can be simplified/moved/cleaned up labels Apr 4, 2024
@dalyIsaac dalyIsaac mentioned this issue May 12, 2024
25 tasks
dalyIsaac added a commit that referenced this issue Aug 1, 2024
Previously, the focus indicator plugin would show and hide based on events from Whim's core. However, this had the downside that the focus indicator could suffer from flickering. This was a result of the multi-threading work for #859, as all events were being processed (as opposed to relying on the STA always providing the latest event).

The focus indicator has now switched to a polling model running on a long-running task (i.e., a different thread). There is a hardcoded sleep for 16ms between each poll (roughly corresponding to 60fps). Polling avoids the continual handling of each event Windows emits and instead relies on the **current state of Whim**. This should resolve #944.

The polling has the downside of significantly increasing the logs which are generated. Some of the more common logs have been moved to `Verbose` to ameliorate this.

The polling has the additional benefit of having the indicator follow the window as it moves around the screen. This was not feasible with the event-based model. The focus indicator also now uses the _actual_ window size, rather than the size which Whim stores. This is good for naughty windows (like Logi Options+) which prevent Whim from resizing them.

This PR also changes the focus indicator to only apply the indicator color to the border of the indicator window, resolving #669. This does not use an actual border for each window - it still uses the prior approach of using a Whim-managed window. From the brief research I did, it did not seem that Windows really supports custom borders without potentially messing with the window itself. This does not resolve #908.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Whim refactor The code can be simplified/moved/cleaned up
Projects
Status: Done
Development

No branches or pull requests

1 participant