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.

Sign up for GitHub

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

Jump to bottom

Relocate the log hook #433

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Relocate the log hook #433

wants to merge 7 commits into from

Conversation

LSLeary
Copy link
Contributor

@LSLeary LSLeary commented Dec 19, 2022

Description

Based on mvdt/#432.

Commits

Shift the logHook into handleRefresh before render

Using handleRefresh as we are, a longstanding issue with the logHook
is fixed, or rather, transformed: running windows within it will not
cause an infinite loop; any requests it issues are silently discarded.
That, however, is not ideal either: unhandled changes may be introduced
to the windowset.

We could run the logHook via handleRefresh, but that would put us
back where we started with infinite loops. Alternatively, we could treat
a request in the logHook as an error and discard its changes à la
catchX.

However, the way that gives the best bang for buck is to move the
logHook from render into handleRefresh, after the user effects
have run but before the refresh itself. Like this, any changes it makes
are seamlessly handled in the refresh it accompanies. This is simple,
efficient, and best of all: it transforms a mere logHook into a fully
fledged "refreshHook".

While the logHook is seeing essentially the same state as far as the
internal model goes, there's one key observable difference: it runs
before the effects in render, and thus it does not see any changes
that might result from running the layout. This could break some
existing usage.

Commentary

  • This has also been tested on my system for some months with no issues, but it may break contrib modules I don't use.

  • Defeats the delay-to-logHook strategy used to set window properties in the manageHook. A remedy is provided in a followup PR.

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.md file

@LSLeary
Track Model--View Deviation in the X monad
b15c97b 
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`.
@LSLeary
MVDT: Propagate changes through Operations and Main
2d6ef0f 
`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.
@LSLeary
MVDT: Declare every deviation; deprecate the undeclaring
3325706 
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.
@LSLeary
Clean up sendMessage, setLayout & manage
2fff2a0 
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.
@LSLeary
Support nesting of handleRefresh
4f50c84 
`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`.
@LSLeary
Document MVDT changes; declare authorship
506b13c
@LSLeary
Shift the logHook into handleRefresh before render
a0e7b63 
Using `handleRefresh` as we are, a longstanding issue with the `logHook`
is fixed, or rather, transformed: running `windows` within it will not
cause an infinite loop; any requests it issues are silently discarded.
That, however, is not ideal either: unhandled changes may be introduced
to the windowset.

We could run the `logHook` via `handleRefresh`, but that would put us
back where we started with infinite loops. Alternatively, we could treat
a request in the `logHook` as an error and discard its changes à la
`catchX`.

However, the way that gives the best bang for buck is to move the
`logHook` from `render` into `handleRefresh`, after the user effects
have run but before the refresh itself. Like this, any changes it makes
are seamlessly handled in the refresh it accompanies. This is simple,
efficient, and best of all: it transforms a mere `logHook` into a fully
fledged "refreshHook".

While the `logHook` is seeing essentially the same state as far as the
internal model goes, there's one key observable difference: it runs
before the effects in `render`, and thus it does not see any changes
that might result from running the layout. This could break some
existing usage.
@LSLeary LSLeary mentioned this pull request Dec 19, 2022
Open
4 tasks
This was referenced Apr 1, 2023
Merged
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@LSLeary