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

Update StatusBar library to use the X monad instead of IO. #878

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

Chobbes
Copy link
Contributor

@Chobbes Chobbes commented Mar 9, 2024

Description

This change allows dynamic status bars to pull information out of the X monad, which can be really useful for status bars. For instance, you can now query the screen width in order to set the width of status bars appropriately.

Existing configurations may need to be updated in order to lift an IO StatusBarConfig to an X StatusBarConfig. This can be done using either the io function provided by XMonad.Core, or liftIO from base in Control.Monad.IO.Class

Checklist

  • I've read CONTRIBUTING.md

  • I've considered how to best test these changes (property, unit,
    manually, ...) and concluded: this is a minimal change to the types of some functions which does not break any existing tests. I have used these changes in my own XMonad configuration to great success.

  • I updated the CHANGES.md file

This change allows dynamic status bars to pull information out of the
X monad, which can be really useful for status bars. For instance, you
can now query the screen width in order to set the width of status
bars appropriately.

Existing configurations may need to be updated in order to lift an
`IO StatusBarConfig` to an `X StatusBarConfig`. This can be done using
either the `io` function provided by `XMonad.Core`, or `liftIO` from
`base` in `Control.Monad.IO.Class`

- https://hackage.haskell.org/package/xmonad-0.18.0/docs/XMonad-Core.html#v:io
- https://hackage.haskell.org/package/base-4.19.1.0/docs/Control-Monad-IO-Class.html#v:liftIO
Copy link
Member

@TheMC47 TheMC47 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Looks good, but I want to wait for a second opinion because this is a breaking change

Copy link
Contributor

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Looks good, but I think we need to hear from users who would be affected by it. You might want to post to /r/xmonad.

@Chobbes
Copy link
Contributor Author

Chobbes commented Mar 9, 2024

Okay, I don't really use reddit, so hopefully I did this right:

https://old.reddit.com/r/xmonad/comments/1baovjf/proposed_improvements_to_statusbar_library/?

@geekosaur
Copy link
Contributor

I don't use it a whole lot either, but it's more active than the IRC channel and certainly more active than expecting xmonad users to be monitoring github.

Copy link
Member

@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.

The breaking change is quite severe, but also trivial to fix, so I think I'm going with "yes" on this one. We just need to properly point this out to people in an eventual release announcement.

Out of curiosity: do you have a usage example?

@Chobbes
Copy link
Contributor Author

Chobbes commented Mar 10, 2024

The breaking change is quite severe, but also trivial to fix, so I think I'm going with "yes" on this one. We just need to properly point this out to people in an eventual release announcement.

Yeah, that's been my opinion. The other alternative is to just add X versions of the functions in addition to the existing ones, but it doesn't really seem worth the duplication.

It seems like a good time to do it with DynamicBars being deprecated recently...

https://hackage.haskell.org/package/xmonad-contrib-0.18.0/docs/XMonad-Hooks-DynamicBars.html

Out of curiosity: do you have a usage example?

Here's an example of using this to build a status bar out of three components proportionally:

mainBarWidth :: Int -> Int
mainBarWidth fullWidth = floor $ 0.4 * fromIntegral fullWidth

conkyBarOffset = mainBarWidth

conkyBarWidth :: Int -> Int
conkyBarWidth fullWidth = (fullWidth - (conkyBarOffset fullWidth)) - (trayerBarWidth fullWidth)

trayerBarWidth :: Int -> Int
trayerBarWidth fullWidth = floor $ fromIntegral fullWidth * 0.05

-- Command to launch the bar.
mainBar :: ScreenId -> X StatusBarConfig
mainBar screen = do
  let S id' = screen
  let id = id' + 1
  (x, _) <- withWindowSet (getScreenDims screen)
  let barWidth = mainBarWidth x
  let dzencmd = "dzen2 -dock -e 'onstart=lower' -w " ++ show barWidth ++ " -h 15 -ta l -fn 'Helvetica:size=11' -xs " ++ show id
  liftIO $ statusBarPipe dzencmd (return myPP)

conkyBar :: ScreenId -> X StatusBarConfig
conkyBar screen = do
  let S id' = screen
  let id = id' + 1
  (x, _) <- withWindowSet (getScreenDims screen)
  let barOffset = conkyBarOffset x
  let barWidth = conkyBarWidth x
  let conkycmd = "conky -c ~/.xmonad-conky"
  let dzencmd = "dzen2 -dock -e 'onstart=lower' -x " ++ show barOffset ++ " -w " ++ show barWidth ++ " -h 15 -ta r -fn 'Helvetica:size=12' -xs " ++ show id
  return $ statusBarGeneric (conkycmd ++ " | " ++ dzencmd) mempty

trayerBar :: ScreenId -> X StatusBarConfig
trayerBar screen = do
  let S id' = screen
  let id = id' + 1
  (x, _) <- withWindowSet (getScreenDims screen)
  let barWidth = trayerBarWidth x
  let conkycmd = "conky -c ~/.xmonad-conky"
  let trayercmd = "trayer --edge top --align right --SetDockType true --SetPartialStrut true --expand true --widthtype pixel --heighttype pixel --tint 0x002b36 --height 15 --width " ++ show barWidth ++ " --alpha 0 --transparent true -l --monitor " ++ show id'
  return $ statusBarGeneric trayercmd mempty

myBar :: ScreenId -> X StatusBarConfig
myBar screen = mainBar screen <> conkyBar screen <> trayerBar screen

-- Defaults to 1920x1080 if it can't find it.
getScreenDims :: ScreenId -> WindowSet -> X (Int, Int)
getScreenDims sid ws =
  case listToMaybe (filter ((== sid) . screen) (screens ws)) of
    Just s  -> return $ rectDims . screenRect $ screenDetail s
    Nothing -> return (1920, 1080)

Note: I just started experimenting with trayer, and it seems like you can only have one copy of the system tray, so that might not work well on multiple monitors 😅

@TheMC47
Copy link
Member

TheMC47 commented Mar 10, 2024

Looks neat. I think we just need to clearly outline the needed changes the release notes. For most configs, the signature of the function given to dynamicSBs must be changed and that's it. I think people just use pure with statusBarProp

@geekosaur
Copy link
Contributor

Agreed, it needs to be clearly documented. I'm in favor.

@Chobbes
Copy link
Contributor Author

Chobbes commented Mar 10, 2024

Looks neat. I think we just need to clearly outline the needed changes the release notes. For most configs, the signature of the function given to dynamicSBs must be changed and that's it. I think people just use pure with statusBarProp

I think that's probably the main use case, in which case nothing would have to change because you would just get pure for X instead of IO.

There is one other thing which I wondered a bit about... statusBarPipe, currently returns an IO StatusBarConfig. I left that as is because it's easy to lift it into X, but it does mean that if you passed the result of statusBarPipe directly into dynamicSBs you would need to lift it from IO to X now. If it's only ever used in conjunction with these functions, though, maybe it's convenient to just have everything in X. I don't have strong opinions about it either way.

I'm also reasonably happy if we just duplicate everything... Or another potential solution might be to have a MonadX typeclass or something which provides a toX :: MonadX mx => mx a -> X a function. Then we could have toX = liftIO for MonadIO mio => mio a -> X a and toX = id for X a -> X a. Having MonadX would mean the usage of these functions wouldn't have to change, but it would mean adding a typeclass somewhere.

@slotThe
Copy link
Member

slotThe commented Mar 11, 2024

There is one other thing which I wondered a bit about... statusBarPipe, currently returns an IO StatusBarConfig. I left that as is because it's easy to lift it into X, but it does mean that if you passed the result of statusBarPipe directly into dynamicSBs you would need to lift it from IO to X now. If it's only ever used in conjunction with these functions, though, maybe it's convenient to just have everything in X. I don't have strong opinions about it either way.

I think that a pattern like

main :: IO ()
main = do
  mySB <- statusBarPipe "xmobar" (pure myPP)
  xmonad $ withSB mySB myConf

is very common, so lifting this to X would probably cause some problems for users.

I'm also reasonably happy if we just duplicate everything...

I don't think that this is necessary, personally.

Or another potential solution might be to have a MonadX typeclass or something which provides a toX :: MonadX mx => mx a -> X a function. Then we could have toX = liftIO for MonadIO mio => mio a -> X a and toX = id for X a -> X a. Having MonadX would mean the usage of these functions wouldn't have to change, but it would mean adding a typeclass somewhere.

Now that you mention it… :) Again, no strong preference from my side.

@TheMC47
Copy link
Member

TheMC47 commented Mar 11, 2024

I think that a pattern like

main :: IO ()
main = do
  mySB <- statusBarPipe "xmobar" (pure myPP)
  xmonad $ withSB mySB myConf

is very common, so lifting this to X would probably cause some problems for users.

It was bugging me since I've seen the PR why I didn't just use X instead of IO, and now I remember. I think we should handle that case somehow. The typeclass-route seems fine.

I'm okay with the breaking change, I just don't want that pattern to break if we can help it

@Chobbes
Copy link
Contributor Author

Chobbes commented Mar 11, 2024

Now that you mention it… :) Again, no strong preference from my side.

Hmmm! I didn't see that when I went looking for something that already existed along these lines... This wouldn't work, though, would it?

class (MonadReader XConf m, MonadState XState m) => XLike m where

The constraints on the class would mean IO couldn't be XLike, no?

@TheMC47 TheMC47 merged commit 2b079bf into xmonad:master Mar 13, 2024
18 checks passed
@TheMC47
Copy link
Member

TheMC47 commented Mar 13, 2024

Thank you for your contribution 🎉 It's actually fine as-is. I created an issue so we don't forget documenting this change in the next release

slotThe added a commit that referenced this pull request Aug 27, 2024
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