Model–View Deviation Tracking #432
Conversation
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`.
For what it's worth, I've run xmonad on 4GB and even 2GB configs with no problem. (Chrome was visibly more difficult, especially on the 2GB machine. 😄 ) Granting that was without your patches. |
|
This is cool! I did a quick (more like 30 minutes) dive into the PR, and if I'm to summarize the value proposition: by design, we can't avoid rendering intermediate states of a bigger transformation, which could cause problems. Your suggestion is to add a "writer" capability to The commits explain the reasoning well, but I found the documentation a bit lacking in that regard. Maybe I'm just sleepy though, I'll definitely give this another go again. I'm not a fan of the name however, we should think of an alternative
Let's be careful about this, people tend to not be happy when a window manager that's supposed to be solid and lightweight suddenly has a space leak. I wouldn't be surprised if we have users with an uptime of a couple of months. Awesome work 🎉 |
|
@TheMC47 Thanks. :)
Yes, that's essentially it. I had wanted to avoid writing a PR message by writing good commit messages instead and letting a cute shell script do the rest, but I see now that I should have included a summary too. I've rectified that with an edit.
This can wait until after you've had another look at it, but please expand on that—I've been working on this for too long, so it's hard for me to tell what is and isn't clear to others.
I considered dirty, but it's not really accurate: any change to the windowset since the last refresh would render it dirty, but computing the view does not demand all of the windowset. The
There are some mitigating factors:
As something of an aside, I actually have a strange anti-leak. I see 0.4% after restart, then some time in the following hours it falls down to 0.2%. No idea why. |
There was a problem hiding this comment.
I had another look at the PR, and it only now clicked that windows was doing too much. It's amazing how strictly defining the responsibility/scope of a piece of code (whether a function in our case or a class in the OOP world) just makes the code better. The single responsibility principle is crucial.
Regarding the MVDT: I think we can entirely avoid explicitly naming the approach we're using. The MVC world doesn't really name this deviation, and it's just accepted that the view and the model will fall out of sync at some time. AFAIK, There are two ways to update the view:
- the model itself triggers the views to update.
- the views themselves poll the model for changes
Our model is just data so it can't request an update, and we have one view so it takes care of fetching the changes whenever needed.
I looked through the docs again and I don't see any sentence where we need to name the "deviation", we can always express our intent explicitly: "requesting a refresh", "rendering the changes", or "whether a refresh is required" etc. Let's see what others think
I'm probably going to use this PR on my daily driver and then my work laptop (since I have longer sessions there and I don't run any desktop environment). I think the changes here are necessary, but it's such an important change that we need to test it thoroughly.
This is cool 🎉 and probably warrants a release 😄
| -- Dynamic components may be retrieved with 'get' and 'listen', static | ||
| -- components with 'ask'. With newtype deriving we get readers, writers and | ||
| -- state monads instantiated on 'XConf', 'Any' and 'XState' automatically. | ||
| -- | ||
| newtype X a = X (ReaderT XConf (StateT XState IO) a) | ||
| deriving (Functor, Applicative, Monad, MonadFail, MonadIO, MonadState XState, MonadReader XConf) | ||
| newtype X a = X (RWST XConf Any XState IO a) | ||
| deriving | ||
| ( Functor, Applicative, Monad, MonadFail, MonadIO | ||
| , MonadReader XConf, MonadWriter Any, MonadState XState | ||
| , MonadRWS XConf Any XState | ||
| ) |
There was a problem hiding this comment.
This is a good place to explain what the Any represents
| -- Return the result, and final state | ||
| runX :: XConf -> XState -> X a -> IO (a, XState) | ||
| runX c st (X a) = runStateT (runReaderT a c) st | ||
| -- Return the result, final state and model--view deviation. |
There was a problem hiding this comment.
| -- Return the result, final state and model--view deviation. | |
| -- Return the result, final state, and whether a refresh is needed. |
|
|
||
| -- | Run in the 'X' monad, and in case of exception, and catch it and log it | ||
| -- to stderr, and run the error case. | ||
| catchX :: X a -> X a -> X a | ||
| catchX job errcase = do | ||
| st <- get | ||
| c <- ask | ||
| (a, s') <- io $ runX c st job `E.catch` \e -> case fromException e of | ||
| (a, s', mvd) <- io $ runX c st job `E.catch` \e -> case fromException e of |
There was a problem hiding this comment.
| (a, s', mvd) <- io $ runX c st job `E.catch` \e -> case fromException e of | |
| (a, s', r) <- io $ runX c st job `E.catch` \e -> case fromException e of |
| Just (_ :: ExitCode) -> throw e | ||
| _ -> do hPrint stderr e; runX c st errcase | ||
| tell mvd |
There was a problem hiding this comment.
| tell mvd | |
| tell r |
| respace :: WorkspaceId -> (WindowSpace -> WindowSpace) -> X () | ||
| respace i f = do |
There was a problem hiding this comment.
I'm curious, why is this called respace?
|
|
||
| module XMonad.Internal.Operations | ||
| ( rendered, unsafeLogView | ||
| ) where | ||
|
|
||
| import Control.Monad.Reader (asks) | ||
| import XMonad.Internal.Core (readView, unsafeWriteView) | ||
| import XMonad.Core (X, WindowSet, internal, io, withWindowSet) | ||
|
|
||
| -- | Examine the 'WindowSet' that's currently rendered. | ||
| rendered :: X WindowSet | ||
| rendered = asks internal >>= io . readView | ||
|
|
||
| -- | See 'unsafeWriteView'. | ||
| unsafeLogView :: X () | ||
| unsafeLogView = do | ||
| i <- asks internal | ||
| withWindowSet (io . unsafeWriteView i) |
There was a problem hiding this comment.
It might be a good idea to mention that these are used in render and specify again in the comments that these are internal.
| @@ -0,0 +1,32 @@ | |||
| {-# LANGUAGE NamedFieldPuns #-} | |||
There was a problem hiding this comment.
What's the reasoning behind having a separate module for this? (I'm not opposed to it)
| -- | Perform an @X@ action, updating the view if it's no longer consistent with | ||
| -- the model. |
There was a problem hiding this comment.
| -- | Perform an @X@ action, updating the view if it's no longer consistent with | |
| -- the model. | |
| -- | Perform an @X@ action, re-rendering if a refresh is required |
|
FWIW I've been referring to |
There was a problem hiding this comment.
I'll be giving this a few more looks, but so far I think the general idea is really neat! I'll be using this as my daily driver for a while, but I don't expect to see any issues with the base changes here. For anyone wanting to do the same, you'll need
diff --git a/XMonad/Util/DebugWindow.hs b/XMonad/Util/DebugWindow.hs
index 90fe5f29..88cfce1d 100644
--- a/XMonad/Util/DebugWindow.hs
+++ b/XMonad/Util/DebugWindow.hs
@@ -145,7 +145,7 @@ catchX' :: X a -> X a -> X a
catchX' job errcase = do
st <- get
c <- ask
- (a, s') <- io $ runX c st job `E.catch` \e -> case fromException e of
+ (a, s', _) <- io $ runX c st job `E.catch` \e -> case fromException e of
Just x -> throw e `const` (x `asTypeOf` ExitSuccess)
_ -> runX c st errcase
put s'Big props for the commit messages; they explain what's going on very clearly. Regarding what @TheMC47 wrote above
The commits explain the reasoning well, but I found the documentation a bit lacking in that regard.
This can wait until after you've had another look at it, but please expand on that—I've been working on this for too long, so it's hard for me to tell what is and isn't clear to others.
I think the point is that when one starts to just read Core.hs and encounters
newtype X a = X (RWST XConf Any XState IO a)it's pretty clear which roles XConf and XState have, but Any seems a bit out of left field. I think that transferring a portion of the commit messages directly to the respective pieces of code would completely solve this issue, however. One thing I always liked about e.g. the StackSet is that it very clearly states its design upfront, I think we should continue this tradition with this change.
- Non-CPS
Writer/RWSis controversial due to possible space leakage, but it does not seem to be an issue here; I don't think I've ever seen xmonad using more than 0.4% of my meagre 8GB RAM. We can eventually swap in the CPS version just to be sure, but to do so now would constrain our dependencies too much.
I might be the wrong person to ask here (and people who usually have an uptime on the order of months, like @liskin, should test this thoroughly), but I don't think this'll cause any issues. As noted below, Any is pretty tiny. Further, not only Writer can leak space, StateT can too, and XConfig is much bigger.
| -- encapsulating the window manager configuration and state, | ||
| -- respectively. | ||
| -- | The X monad; 'RWST' transformer over 'IO' encapsulating the window manager | ||
| -- configuration, model--view deviation and state, respectively. |
There was a problem hiding this comment.
| -- configuration, model--view deviation and state, respectively. | |
| -- configuration, model–view deviation and state, respectively. |
We're Haskell programmers, we don't fear the unicode :)
There was a problem hiding this comment.
While testing xmonad/xmonad-contrib#766 in order to hopefully merge it soon, I realised that the "preview" that what's now XMonad.Actions.Repeatable implements is broken with this PR (that is, the view doesn't update until one releases the respective key). This is probably fixable, just pointing it out :)
|
@LSLeary does this build with contrib? |
This is the only issue with contrib (see #432 (review)) |
Description
Summary
Create a layer of indirection between needing a refresh and performing one, such that no more than one refresh is performed per event handled. This is implemented by extending the X monad to write a "dirty bit". The change improves not only efficiency and UX, but also code reuse and elegance.
Commits
Track Model–View Deviation in the X monad
MVDT: Propagate changes through Operations and Main
MVDT: Declare every deviation; deprecate the undeclaring
Clean up
sendMessage,setLayout&manageSupport nesting of
handleRefreshDocument MVDT changes; declare authorship
Commentary
I don't know if there's established MVC jargon for this strategy, so I'm just calling it MVD and MVDT. Edit: Other possible names are dirty and dirtying.
I've been building my config on top of (some incarnation of) these changes for several months now, if not a year—they're well tested. The only exceptions are a few brand new changes to the Declare every deviation commit:
Erased an erroneous
norefreshfrom the definition ofbroadcastMessage. It should perhaps benorefreshed at certain usage sites instead.Wrote
respace, in terms of whichupdateLayoutwas rewritten.Non-CPS
Writer/RWSis controversial due to possible space leakage, but it does not seem to be an issue here; I don't think I've ever seen xmonad using more than 0.4% of my meagre 8GB RAM. We can eventually swap in the CPS version just to be sure, but to do so now would constrain our dependencies too much.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 function as expected.
I updated the
CHANGES.mdfile