Purify the unclean #436

LSLeary · 2022-12-19T11:38:56Z

Description

Based on mvdt/#432.

Commits

Purify the unclean by generalising type signatures

Many operations were previously impure due to direct or indirect use of
the old windows, now render. Due to MVDT, we can now purify them.

To do so, we generalise their type signatures away from X to
arbitrary monads satisfying the relevant mtl constraints. This has
further advantages in terms of code reuse (e.g. in X.U.PureX) and
possibly in testing.

However, it also causes some breakage—bindings that previously type
checked without a signature may now need FlexibleContexts to do so.

Deprecate `runOnWorkspaces` in favour of `traverseWorkspaces`

runOnWorkspaces was a questionable existence in various ways:

It traversed the workspaces in a unexpected, nonstandard order.

It needlessly utilised a non-exhaustive pattern match, abusing
first MonadFail then irrefutability to evade warning.

It was used nowhere in contrib, once in core—that sole use with a
pure function.

Even after generalisation, it still has a bad type, both misleading and
lacking in parametricity. Specialising the StackSet traversal to the
WindowSet in XState gives the function too much power, and suggests
that it's using that power to provide special support for WindowSet
modifications when in fact it does no such thing.

To resolve these issues, the workspace traversal logic is written as
traverseWorkspaces in X.StackSet, and mapWorkspaces written atop
that via Identity. The latter then replaces the sole use of
runOnWorkspaces, condemning it to deprecation.

Commentary

The first commit has been tested for some months. The second is much newer, but has been running on my system with no issue.
The second commit is another kind of hygiene. It arose on account of ugliness I was forced to confront in writing the first, but beyond that, it's not strictly related. It could be split off, but it didn't seem to deserve a PR of its own.

Checklist

I've read CONTRIBUTING.md
I've confirmed these changes don't belong in xmonad-contrib instead
I've considered how to best test these changes (property, unit,
manually, ...) and concluded: they appear to function as intended.
I updated the CHANGES.md file

Currently, composability of X actions is broken, or at least flawed. The culprits are `windows` and a misplacement of responsibility. Every operation that needs to make changes to visible parts of the windowset must run them through the core model-rendering function, yet when time comes to compose those operations, there's no way to prevent intermediate states from being rendered. All of these additional renders are potentially expensive or otherwise deleterious to user experience; they can map and unmap windows, resize them multiple times, wake sleeping or swapped processes, etc. This can easily result in major slowdowns and visible artefacts such as windows jumping around, workspaces flashing past or borders flickering. The original design mitigated these issues by having `windows` accept composable windowset-modifying functions, however, this approach only works within the scope of a given action; it doesn't help to fuse unrelated actions. Further, interleaving any kind of layout operation or IO fundamentally breaks the paradigm. Some efforts have been made to remedy the problem at the contrib level; `X.A.MessageFeedback` and `X.U.PureX` in particular. However, they cannot be considered successful: they require actions to be rewritten in their paradigm, and must foist the task of transforming composable actions into "complete" ones onto the end user. This is not the righteous way. If we instead recognise that rendering the view is the responsibility of the core, a real solution becomes clear: we must take `windows` back from both the hands of the end user and the contrib module implementor. The way to do so transparently is to replace `windows` with a function that, rather than /performing/ a refresh, merely /requests/ one. In the paradigm of `X.U.PureX`, this is achieved by combining actions that produce `Any`, however, this approach does not suffice for our end: it breaks code by changing types, it forces changes in implementation to manually combine the `Any`s, and it interferes with actual return values. The proper tool to issue and combine monoidal values in a monadic context is the writer monad, hence we extend the X monad to write `Any`.

`refresh` becomes our deviation-declaring, refresh-requesting operation, and its pair `norefresh` is introduced to deny such requests or mark them handled. As promised, we now make `windows` internal by renaming it `render`, providing a replacement that just modifies the windowset and issues `refresh`. `windowBracket` is cannibalised to become `handleRefresh`, catching requests and handling them through `render`. As with `windows`, we provide a replacement with the corresponding semantics under the new ways. Finally, we run the event handler and startup operations inside `handleRefresh`, completing the change of regime. /Note that this change will break any functionality actually requiring `windows` to perform an immediate refresh./ The solution would be to wrap any such use of `windows` in `handleRefresh`, but it currently offers poor support for nesting. This matter will be rectified in a later commit.

Under the new regime, an operation should issue `refresh` if it may directly cause the model to deviate from the view, and should avoid doing so when it knows it will not. Hence: * `updateLayout` issues `refresh` iff the target workspace is visible. For reusability, this workspace-updating logic is implemented as the more general `respace`. * `modifyWindowSet` is deprecated. * `sendMessageWithNoRefresh` is replaced by `messageWorkspace` and deprecated. * `broadcastMessage` may now (indirectly) issue `refresh`. `windowBracket` and co. were originally introduced so that windowset changes made in `sendMessage` would be properly handled in the accompanying refresh. The approach has since been adopted for use in `handleRefresh`, so these functions are no longer necessary and don't belong in the core. As such, `sendMessage` is simplified and the "bracket" functions are deprecated.

There was unnecessary noise and duplication in these functions. * `sendMessage`: Rewrite via `messageWorkspace` * `setLayout`: Rewrite via `sendMessage` and `updateLayout` * `manage`: Remove code equivalent to `W.view (W.currentTag ws) ws` The `sendMessage` rewrite also fixes a corner case misbehaviour: previously, if message handling resulted in another workspace taking focus, the layout update would mangle that workspace. This is no longer the case.

`render` requires the previously rendered state of the model---the current state of the view---to judge changes against. Originally, this was done by taking the windowset for that state and requiring changes be provided in the form of a function. Under that regime, the windowset is not a fine grained internal model---its merely a log of the view which shouldn't be exposed for editing outside of `render`. `handleRefresh` (previously `windowBracket`) used a cute trick to buy granularity within a child action by grabbing the "old" and the "new" copies of the windowset that `render` needs before and after the action runs. However, such a trick has its limits. In particular, it does not support nesting---a refresh in the child action invalidates the "old" copy. The solution is straightforward: separately from our model, keep an actual log of the state of the view, and don't expose it for editing. Hence it does not appear in the `XState`, but is secreted away in an `IORef` in a new opaque `Internal` portion of the `XConf`.

Many operations were previously impure due to direct or indirect use of the old `windows`, now `render`. Due to MVDT, we can now purify them. To do so, we generalise their type signatures away from `X` to arbitrary monads satisfying the relevant mtl constraints. This has further advantages in terms of code reuse (e.g. in `X.U.PureX`) and possibly in testing. However, it also causes some breakage---bindings that previously type checked without a signature may now need `FlexibleContexts` to do so.

`runOnWorkspaces` was a questionable existence in various ways: * It traversed the workspaces in a unexpected, nonstandard order. * It needlessly utilised a non-exhaustive pattern match, abusing first `MonadFail` then irrefutability to evade warning. * It was used nowhere in contrib, once in core---that sole use with a pure function. * Even after generalisation, it still has a bad type, both misleading and lacking in parametricity. Specialising the `StackSet` traversal to the `WindowSet` in `XState` gives the function too much power, and suggests that it's using that power to provide special support for `WindowSet` modifications when in fact it does no such thing. To resolve these issues, the workspace traversal logic is written as `traverseWorkspaces` in `X.StackSet`, and `mapWorkspaces` written atop that via `Identity`. The latter then replaces the sole use of `runOnWorkspaces`, condemning it to deprecation.

slotThe

Maybe this is unwise, but I'm taking an "assume the other PR was already merged" approach to this review (and to all others that follow) :)

All-in-all this looks fine to me. The only thing I'd change is to add SPECIALISE pragmas to the functions that are now generalised (see below), since mtl performance is heavily dependent on whether inlining/specialisation can take place (this may or may not matter, but specialising comes at basically no cost for us).

slotThe · 2022-12-23T14:27:46Z

src/XMonad/Operations.hs

-withFocused :: (Window -> X ()) -> X ()
+withFocused :: MonadState XState m => (Window -> m ()) -> m ()
 withFocused f = withWindowSet $ \w -> whenJust (W.peek w) f


E.g.

withFocused :: MonadState XState m => (Window -> m ()) -> m () withFocused f = withWindowSet $ \w -> whenJust (W.peek w) f {-# SPECIALISE withFocused :: (Window -> X ()) -> X () #-}

slotThe · 2022-12-23T14:30:20Z

src/XMonad/StackSet.hs

+-- | Map over the 'Workspace's of a 'StackSet'.
+mapWorkspaces
+  :: (Workspace i l a        -> Workspace i' l' a)
+  ->  StackSet  i l a sid sd -> StackSet  i' l' a sid sd
+mapWorkspaces f = runIdentity . traverseWorkspaces (Identity . f)
+
+-- | 'traverse' the 'Workspace's of a 'StackSet'.
+traverseWorkspaces
+  :: Applicative f
+  => (Workspace i l a        -> f (Workspace i' l' a))
+  ->  StackSet  i l a sid sd -> f (StackSet  i' l' a sid sd)
+traverseWorkspaces f s = StackSet
+  <$>          onScreen (current  s)
+  <*> traverse onScreen (visible  s)
+  <*> traverse f        (hidden   s)
+  <*> pure              (floating s)
+  where onScreen scr = f (workspace scr) <&> \w -> scr{ workspace = w }
+


Maybe we should reconsider #357 after all...

LSLeary added 8 commits December 19, 2022 22:14

Document MVDT changes; declare authorship

506b13c

LSLeary force-pushed the purify branch from e8bfb0b to 7e03677 Compare December 19, 2022 12:05

slotThe approved these changes Dec 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purify the unclean #436

Purify the unclean #436

LSLeary commented Dec 19, 2022

slotThe left a comment •

edited

slotThe Dec 23, 2022

slotThe Dec 23, 2022

Purify the unclean #436

Are you sure you want to change the base?

Purify the unclean #436

Conversation

LSLeary commented Dec 19, 2022

Description

Commits

Purify the unclean by generalising type signatures

Deprecate runOnWorkspaces in favour of traverseWorkspaces

Commentary

Checklist

slotThe left a comment • edited

Choose a reason for hiding this comment

slotThe Dec 23, 2022

Choose a reason for hiding this comment

slotThe Dec 23, 2022

Choose a reason for hiding this comment

Deprecate `runOnWorkspaces` in favour of `traverseWorkspaces`

slotThe left a comment •

edited