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

doc: XMonad.Layout.Inspect #2

Open
wants to merge 4 commits into
base: layout-inspect
Choose a base branch
from

Conversation

ivanbrennan
Copy link

Description

Describe and document usage of the XMonad.Layout.Inspect module.

Describe and document usage.
@ivanbrennan ivanbrennan marked this pull request as draft March 6, 2023 00:54
-- > l <- asks (layoutHook . config)
-- > case getFoo l wsp of
-- > Nothing -> pure ()
-- > Just s -> xmessage s
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liskin @mislavzanic I encountered some unexpected behavior while testing this. I captured my test setup here, and the most relevant part is this section.

In brief, I created a simple layout modifier:

newtype Foo a = Foo String deriving (Read, Show)
instance LayoutModifier Foo a

foo :: LayoutClass l a => String -> l a -> ModifiedLayout Foo l a
foo = ModifiedLayout . Foo

data GetFoo = GetFoo
type instance InspectResult GetFoo = Alt Maybe String

instance InspectLayout GetFoo Foo a where
  inspectLayout GetFoo (Foo s) = pure s

getFoo :: (LayoutClass l Window, InspectLayout GetFoo l Window)
       => l Window -> WindowSpace -> Maybe String
getFoo l = getAlt . inspectWorkspace l GetFoo

and used it in a layoutHook:

myLayoutHook = foo "test" (Tall nmaster delta ratio)
  where
     nmaster = 1
     ratio   = 1/2
     delta   = 3/100

main = xmonad $ def { keys = myKeys, layoutHook = myLayoutHook }

If I pass myLayoutHook as the first argument, getFoo returns Just "test" as expected. E.g.

do
  wsp <- withWindowSet (pure . W.workspace . W.current)
  getFoo myLayoutHook wsp
  -- => Just "test"

However, if I retrieve the layout hook by other means, such as asks, then getFoo returns Nothing:

do
  wsp <- withWindowSet (pure . W.workspace . W.current)
  l <- asks (layoutHook . config)
  getFoo l wsp
  -- => Nothing

Any idea what I'm missing?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The layoutHook you pull out of the config record at runtime is wrapped in the Layout existential. However, the actual purpose of the l a argument to inspectWorkspace and co. is to unwrap that existential by casting its contents to the supplied type, so giving it the wrapped layout doesn't help at all; the cast should fail with a call to error.

The OVERLAPPABLE instance for InspectLayout is then selected, narrowly preventing the error from blowing up in your face, relegating it to a Nothing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That explains it, thanks 👍

I can update the usage documentation accordingly (see below). I'd be interested to know if there's another way to call this correctly.

diff --git a/XMonad/Layout/Inspect.hs b/XMonad/Layout/Inspect.hs
index 18b93eb8..ddea14a0 100644
--- a/XMonad/Layout/Inspect.hs
+++ b/XMonad/Layout/Inspect.hs
@@ -69,10 +69,11 @@ import qualified XMonad.StackSet as W
 -- An end-user can then call getFoo by passing it the appropriate layout and a
 -- given workspace:
 --
+-- > myLayoutHook = ...
+-- >
 -- > xFoo :: WindowSpace -> X ()
 -- > xFoo wsp = do
--- >   l <- asks (layoutHook . config)
--- >   case getFoo l wsp of
+-- >   case getFoo myLayoutHook wsp of
 -- >     Nothing -> pure ()
 -- >     Just s -> xmessage s

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, as myLayoutHook is AFAIK the only place where the actual layout type (not wrapped in an existential) is present

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's present in the XConfig as well, so we could possibly add something like an inspectableLayout :: Proxy (l Window) → XConfig l → XConfig l and then have people do

main = xmonad $ inspectableLayout $ \il -> def{ layoutHook = myLayoutHook,  }

That might be marginally less error-prone, as some XConfig combinators like withEasySB change the layout, we'd just need to tell people that inspectableLayout needs to be the very last. But they'd need to pass the il as a parameter every time they need it outside of main

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I really think you'd want to be capturing the layout type immediately before xmonad eats it. I suggest something like:

-- > main = xmonadIL \i@Inspect{current,tag,workspace} ->
-- >   ...
xmonadIL
  :: (LayoutClass l Window, Read (l Window))
  => (Inspect l -> XConfig l) -> IO ()
xmonadIL = xmonad . inspectableLayout

-- > main = xmonad $ inspectableLayout \i@Inspect{current,tag,workspace} ->
-- >   ...
inspectableLayout
  :: forall l. Typeable l
  => (Inspect l -> XConfig l) -> XConfig l
inspectableLayout k = k (makeInspect (Proxy @l))

data Inspect l = Inspect
  { current   :: forall i. InspectLayout i l Window
              => i                -> X (       InspectResult i )
  , tag       :: forall i. InspectLayout i l Window
              => i -> WorkspaceId -> X (Maybe (InspectResult i))
  , workspace :: forall i. InspectLayout i l Window
              => i -> WindowSpace ->           InspectResult i
  }

makeInspect :: Typeable l => proxy l -> Inspect l
makeInspect p = Inspect
  { current   = inspectCurrentP   p
  , tag       = inspectTagP       p
  , workspace = inspectWorkspaceP p
  }

where

inspectCurrentP :: (Typeable l, InspectLayout i l Window)
                => proxy l -> i -> X (InspectResult i)
inspectCurrentP p i = gets (inspectWorkspaceP p i . w)
    where w = W.workspace . W.current . windowset

inspectTagP :: (Typeable l, InspectLayout i l Window)
            => proxy l -> i -> WorkspaceId
            -> X (Maybe (InspectResult i))
inspectTagP p i t = gets (fmap (inspectWorkspaceP p i) . mw)
    where mw = find ((t==) . W.tag) . W.workspaces . windowset

inspectWorkspaceP :: (Typeable l, InspectLayout i l Window)
                  => proxy l -> i -> WindowSpace -> InspectResult i
inspectWorkspaceP p i = inspectLayout i . asLayout p . W.layout

asLayout :: (Typeable l, Typeable a) => proxy l -> Layout a -> l a
asLayout _ (Layout l) = cast' l

and

inspectCurrent :: forall l i. (Typeable l, InspectLayout i l Window)
               => l Window -> i -> X (InspectResult i)
inspectCurrent _ = inspectCurrentP (Proxy @l)

inspectTag :: forall l i. (Typeable l, InspectLayout i l Window)
           => l Window -> i -> WorkspaceId
           -> X (Maybe (InspectResult i))
inspectTag _ = inspectTagP (Proxy @l)

inspectWorkspace :: forall l i. (Typeable l, InspectLayout i l Window)
                 => l Window -> i -> WindowSpace -> InspectResult i
inspectWorkspace _ = inspectWorkspaceP (Proxy @l)

It can't really be helped that i needs to be passed around to be used outside of main; this is the only place we know the actual layout type. The alternative is to tell users to write their configs like:

main = xmonad myConfig

myConfig = ewmh . ... $ def
  { ...
  }

and use e.g. inspectCurrentP myConfig. Of if IO is necessary:

main = makeMyConfig >>= xmonad

makeMyConfig :: IO (XConfig L)
makeMyConfig = do
  ...

using inspectCurrentP (Compose makeMyConfig).

Attempting to read the layoutHook from config produces a layout wrapped
in the Layout existential, so it seems the only way to correctly
inspect a layout is by using the same value that was passed to the
XConfig constructor, or by recreating an identical value. Otherwise,
the type cast fails and we fallback to the OVERLAPPABLE instance for
InspectLayout, returning Nothing.
@ivanbrennan ivanbrennan marked this pull request as ready for review March 6, 2023 19:21
Copy link

@slotThe slotThe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, thanks 🎉

@@ -8,15 +8,17 @@

-- |
-- Module : XMonad.Layout.Inspect
-- Description : Inspect layout data.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps s/data/state (here and in other places)? Data sounds very static, but the main point seems to be that we want access to runtime information

Comment on lines 113 to 117
-- An overlappable instance provides sensible default behavior, returning
-- 'mempty'. (Specific @InspectResult@ instances may override this by defining
-- their own instance of 'InspectLayout'.) Additionally, an instance is provided
-- for layouts using 'Choose', provided that all the layouts in the 'Choose' are
-- capable of being inspected.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should perhaps be documentation of the respective instances? Although I guess the Choose one could be omitted entirely, as the dependency graph is right there in the type signature

Emphasize the fact that we're trying to access runtime state, as opposed
to static data.
Also, omit the Choose documentation, since the type signature pretty
much says it all.
@ivanbrennan
Copy link
Author

I'm be happy to squash the commits if that's preferred, to clean things up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants