Simplify the design of the manage hook #435
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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`.
`ManageHook` as `Query (Endo WindowSet)` is a holdover from The Old Way.
`Query` wraps the `X` Monad, which already collects and composes changes
to the windowset. As such, the `Endo WindowSet` is redundant. Worse, the
way these changes compose is not at all obvious or intuitive.
Suppose you compose two ManageHook values:
manageHook = (qF $> Endo f)
<> (qG $> Endo g)
The result of running the `manageHook` on some `w` is not, as one might
guess,
do
runQuery qF w
windows f
runQuery qG w
windows g
nor is it
do
runQuery qF w
runQuery qG w
windows f
windows g
Unfortunately, it is:
do
runQuery qF w
runQuery qG w
windows g
windows f
This is a wart, and it's one we can now excise.
Hence the `ManageHook` type synonym is simplified to `Query ()`, and
`manage` runs the hook as-is. It also performs the initial handling of
the window beforehand, so it's fully visible within the hook. As such,
`willFloat` (which repeats the indirect logic in `manage`) is deprecated
in favour of `isFloat`, which directly references the model.
Note that this change also allows cleaner syntax in configuration. E.g.
manageHook = composeAll
[ test1 --> fooHook
, test2 --> barQueryAction >> idHook
, test3 --> fooHook <> bazHook <> quuxHook
]
can become
manageHook = do
test1 --> fooHook
test2 --> barQueryAction
test3 --> do
fooHook
bazHook
quuxHook
To accomodate the change, `doF` must become a lifted `windows`, but that
and monoid-polymorphism take care of adjusting the rest of the core
interface transparently.
As such, simple configs will continue to compile, but extensions that
directly interfered with the old `Endo WindowSet` won't.
Further, due to `doF`s now composing forwards rather than backwards,
many user configs will break silently. The solution is to reverse the
order that `doF`, `doFloat`, `doIgnore` and `doShift` appear in,
extending that list to include anything else built on `doF`, like
`doShiftTo` or `doFullFloat` from `X.H.ManageHelpers`.
slotThe
reviewed
Dec 23, 2022
There was a problem hiding this comment.
Unlike the other PRs, I'm not actually sure this one is advisable. It may be "morally correct", but the degree of config breakage may just be too high for it to be worth it.
This is what I'm thinking as well. Seems like this would break half of contrib (not to mention user configs); probably not advisable at this point
I second this, this seems like a big change where it's hard to predict how things might break. However, I'm gonna try and advocate for merging this at some point:
I'm not a big fan of guaranteeing backward compatibility at all costs, especially if the cost is disregarding a better alternative. Perfect software doesn't exist, so things will break at some point. However, We need to be careful and mindful on how and when to break things |
|
We did get a number of complaints about backward compatibility breaking after 0.17.0 came out, though. I'm tempted to say we should schedule a 1.0 release at some point, then save this for the 2.0 series. Possibly the same for the other PRs, since I know I've pointed lots of people to |
This sounds like a reasonable way to go forward |
aavogt
reviewed
Feb 12, 2023
| deriving (Semigroup, Monoid) via Ap X a | ||
|
|
||
| instance Default a => Default (X a) where | ||
| def = return def | ||
|
|
||
| type ManageHook = Query (Endo WindowSet) | ||
| type ManageHook = Query () | ||
| newtype Query a = Query (ReaderT Window X a) | ||
| deriving (Functor, Applicative, Monad, MonadReader Window, MonadIO) | ||
| deriving (Semigroup, Monoid) via Ap Query a |
There was a problem hiding this comment.
Further, due to doFs now composing forwards rather than backwards,
many user configs will break silently.
You could keep the old order by making it Dual (Ap Query a), but then it becomes even harder to learn.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Based on
mvdt/#432.Commits
MVDT: Simplify the design of the manage hook
Commentary
An early incarnation of these changes was tested for some months.
Unlike the other PRs, I'm not actually sure this one is advisable. It may be "morally correct", but the degree of config breakage may just be too high for it to be worth it.
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 intended.
I updated the
CHANGES.mdfile