Use bracket pattern in XMonad.Prompt

[mgsloan]

Also deduplicates some code shared by mkXPromptWithReturn and
mkXPromptWithModes.

Description

See issue #180. I noticed 3 obvious places where Prompt can benefit from the bracket pattern, and this PR implements that. This should help avoid failure modes like the keyboard being grabbed forever, or leaving a prompt window open forever.

Checklist

• I've read CONTRIBUTING.md

• I tested my changes with xmonad-testing

• I updated the CHANGES.md file

[byorgey]

Thanks @mgsloan ! This certainly seems reasonable on its face, but I feel like I have no way to really verify whether this works as advertised. Anyone else have any thoughts?

[mgsloan]

Thanks for reviewing @byorgey ! The issue described in #180 can be reproduced by causing an error after the xprompt has initialized:

data ErrPrompt = ErrPrompt
instance XPrompt ErrPrompt where
  showXPrompt _ = error "err prompt"

Then, having a keybinding bound to mkXPrompt ErrPrompt xpconfig (\_ -> return []) $ \_ -> return ()). Without this patch, keyboard input gets permanently grabbed by the prompt. XMonad bindings still work, but keystrokes don't make it to applications. The reason that the error needs to be thrown from showXPrompt rather than, say, the completion function, is that it is wrapped in a catch-all:

  io $ (srt q <$> compl q) `E.catch` \(SomeException _) -> return []

I'm actually in favor of deleting this use of catch, or at the very least, display the error to stderr. It could be fairly befuddling why completion isn't working if it's just silently throwing an exception.

If it aids in review, I could split this into two commits - one to extract out the commonality between mkXPromptWithReturn and mkXPromptWithModes - and the other adding use of bracket.

[byorgey]

If it aids in review, I could split this into two commits - one to extract out the commonality between mkXPromptWithReturn and mkXPromptWithModes - and the other adding use of bracket.

If you're willing to do that I think it would be really helpful. I was having trouble even figuring out what I was looking at when trying to parse the diff.

[mgsloan]

Also, note that I didn't encounter this issue in the wild, but I have encountered cases where I could not exit the prompt (#290), and so I'm interested in making the prompt more reliable

[byorgey]

Come to think of it, I have experienced the behavior in #290 too.

[mgsloan]

If you're willing to do that I think it would be really helpful. I was having trouble even figuring out what I was looking at when trying to parse the diff.

Yep, the same occurred to me now that I'm looking at the diff a while after writing it. Pretty inscrutable! I've pushed separate commits that make it a bit clearer

[mgsloan]

Ideally, destroying the completion window would be in a finally, but unfortunately that resource is tracked via StateT. Handling that properly would be a larger change.

[mgsloan]

While reproducing #217 , I found that it was pretty gnarly if the completion window wasn't destroyed. So I've added a commit resolving that.
Admittedly the IORef solution feels pretty hacky, but the only alternatives I can think of are equivalent to adding such an IORef in some other way, and those alternatives require a lot more plumbing than just adding it to the StateT.

[mgsloan]

@byorgey I've rebased this atop master. Is this splitting up of the change sufficient for review?

[byorgey]

@mgsloan Thanks, that was helpful! Sorry for the delay.

[mgsloan]

@byorgey No problem, thanks for merging :)

…

On Tue, Feb 12, 2019, 07:14 Brent Yorgey ***@***.*** wrote: @mgsloan <https://github.com/mgsloan> Thanks, that was helpful! Sorry for the delay. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#287 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABYKn_1TFVCEcBPjLzth6tiqpAeGaHYks5vMtpVgaJpZM4ZkWTR> .