Change release button handling to issue passthrough events #524
Conversation
|
Maybe another way to solve this issue would be to allow a hook to be registered as a sort of "master hook", holding processing of any other hooks. In that case, this would obviate the need for having a sluice: buttons like |
|
I revisited this question and made another attempt at finding a cleaner solution that would work also with layers switches. My current proposed solution introduces an additional hooks layer after the sluice in the pull chain and a WrappedEvent data type that can hold either KeyEvents or Uniques (hook tags from expired timers). Regular hooks are registered in this second hooks layer, while any hook that makes use of the These changes mean that processing of any kind of event can now be paused, including key release events and timers. When paused, only the priority hooks layer will process these events, taking care of eventually re-opening the sluice. @david-janssen @slotThe @thoelze1 @MuhammedZakir Let me know what you think! |
|
I haven't looked into KMonad's internals nor do I have necessary experience, so I can't comment on your code. But I will test it later; may take a few weeks though. It looks like you accidentally modified |
Thanks, that would be great!
Yeah, I don't know what's going on there, it somehow automatically did this whenever I made a new branch. I suspect something weird is going on with my git on Windows. If you check the actual branch, the file is still there… Edit: Windows had decided to drop the executable flagged, should be fixed now. |
|
Thanks a lot for working on this. I was able to test this out and seems to be working well for me. I'll keep using it and let you know if I run into any issues. |
|
@jchtt sorry, life got in the way. I will definitely try to get to it this week! Thanks already for the contribution! |
|
Thanks @slotThe, no rush! Not sure this is the best way to solve this problem, happy to think about other approaches as well. |
There was a problem hiding this comment.
Just checking in to say that I haven't forgotten about this! I've been running on this PR for a few days now and I quite like the behaviour (and haven't noticed any drawbacks so far).
I would agree with your assessment that it's probably the least invasive solution with the current way that things work. Having this two-hook solution is perhaps conceptually a bit "meh", but in this case I think perfection can wait until after model-refactor is not just a distant dream anymore :)
I plan to read the code in more details (hopefully soon...) but I think the overall idea is sound and from a glance I don't suppose will change much aside from some style considerations. Thanks!
|
Thanks @slotThe. One style question I have is the following: if we keep the strategy as is, any hook function that calls |
|
I think we should document when to use |
Beyond making it a completely new type? Probably not. A sleep-deprived, rambling, thought: we could perhaps make this work with phantom types, exporting not the data constructors themselves but constructor functions for them. This needs some further deliberation but it's probably worth it
Definitely yes! |
|
Just popping in to give a strategic answer: if you managed to identify a problem with the pullchain model and found a way to fix it, then I applaud that greatly. If we test things and they concretely fix a problem without introducing any new ones, I am all for it. I have 0 attachment to the direction the pullchain model is taken into, because I think that at its core, it's a very flawed solution written by someone who was still thinking imperatively. I have loads of work to do in IO at the moment, so that's where I'll be focusing my efforts. If I do ever manage to wrap up that work, I'm turning my attention to a completely different way of handling the model. Until that time, any duct-tape on the pullchain is deeply appreciated :-) |
There was a problem hiding this comment.
This is what I would have personally changed. Keep in mind, I'm just a contributor I cannot actually merge this.
If I understand it correctly you also did a little refactoring where you touched the code
with which function handles timeout (missing understanding, not against it).
Nitpicking aside, nice work!
| | OutputHook -- ^ Install the hook just before emitting a 'KeyEvent' | ||
| = InputHookPrio -- ^ Install the hook immediately after receiving a &# E7EE 39;KeyEvent' | ||
| -- and before the sluice | ||
| | InputHook -- ^ Installthe hook after the sluice |
There was a problem hiding this comment.
you missed a space
| | InputHook -- ^ Installthe hook after the sluice | |
| | InputHook -- ^ Install the hook after the sluice |
| itr <- atomically $ newEmptyTMVar | ||
| dsp <- Dp.mkDispatch (awaitKey src) itr | ||
| ihkPrio <- Hs.mkHooks (Dp.pull dsp) itr | ||
| slc <- Sl.mkSluice $ Hs.pull ihkPrio | ||
| ihk <- Hs.mkHooks (Sl.pull slc) itr |
There was a problem hiding this comment.
I would give itr as the first parameter since it doesn't require parenthesis.
You would also need to change the functions below.
| itr <- atomically $ newEmptyTMVar | |
| dsp <- Dp.mkDispatch (awaitKey src) itr | |
| ihkPrio <- Hs.mkHooks (Dp.pull dsp) itr | |
| slc <- Sl.mkSluice $ Hs.pull ihkPrio | |
| ihk <- Hs.mkHooks (Sl.pull slc) itr | |
| itr <- atomically $ newEmptyTMVar | |
| dsp <- Dp.mkDispatch itr $ awaitKey src | |
| ihkPrio <- Hs.mkHooks itr $ Dp.pull dsp | |
| slc <- Sl.mkSluice $ Hs.pull ihkPrio | |
| ihk <- Hs.mkHooks itr $ Sl.pull slc |
| = InputHookPrio -- ^ Install the hook immediately after receiving a 'KeyEvent' | ||
| -- and before the sluice | ||
| | InputHook -- ^ Installthe hook after the sluice |
There was a problem hiding this comment.
I would have added a parmater to the InputHook to differentiate whether before or after the sluice.
This would make the within function more sensical as within OutputHook is a bit wierd.
Though this is the change I'm least sure about
|
|
||
| -- Initialize the button environments in the keymap | ||
| phl <- Km.mkKeymap (cfg^.firstLayer) (cfg^.keymapCfg) | ||
|
|
||
| -- Initialize output components | ||
| otv <- lift . atomically $ newEmptyTMVar | ||
| ohk <- Hs.mkHooks . atomically . takeTMVar $ otv | ||
| otr <- atomically $ newEmptyTMVar | ||
| ohk <- Hs.mkHooks ((atomically . takeTMVar) otv >>= pure . (WrappedKeyEvent NoCatch)) otr |
There was a problem hiding this comment.
When changing the parameter order of mkHooks as described above this too would be more readable*:
| ohk <- Hs.mkHooks ((atomically . takeTMVar) otv >>= pure . (WrappedKeyEvent NoCatch)) otr | |
| ohk <- Hs.mkHooks otr $ pure . WrappedKeyEvent NoCatch <=< atomically . takeTMVar $ otv |
* readablitiy is subjective so this is just my opinion.
EDIT:
Or even better don't use monadic stuff when you use pure, just use the functor.
| mkDispatch' :: MonadUnliftIO m => m KeyEvent -> TMVar Unique -> m Dispatch | ||
| mkDispatch' s itr = withRunInIO $ \u -> do |
There was a problem hiding this comment.
As mentioned above I would change the order of parameters like this and also for mkDispatch, mkHooks, ... (and maybe Dispatch, ...)
| mkDispatch' :: MonadUnliftIO m => m KeyEvent -> TMVar Unique -> m Dispatch | |
| mkDispatch' s itr = withRunInIO $ \u -> do | |
| mkDispatch' :: MonadUnliftIO m => TMVar Unique -> m KeyEvent -> m Dispatch | |
| mkDispatch' itr s = withRunInIO $ \u -> do |
| @@ -82,7 +85,7 @@ mkDispatch = lift . mkDispatch' | |||
| -- 1. The next item to be rerun | |||
There was a problem hiding this comment.
I would add the timer in the comments of what is happening.
| dsp <- Dp.mkDispatch $ awaitKey src | ||
| ihk <- Hs.mkHooks $ Dp.pull dsp | ||
| slc <- Sl.mkSluice $ Hs.pull ihk | ||
| itr <- atomically $ newEmptyTMVar |
There was a problem hiding this comment.
Apply hint
| itr <- atomically $ newEmptyTMVar | |
| itr <- newEmptyTMVarIO |
This is a problem at a few points.
Even better rebase on master since there it's already fixed.
This is a long standing issue, so it fixes a few things: - fixes kmonad#256 - fixes kmonad#466 (duplicate of kmonad#256 or kmonad#111) - fixes kmonad#523 (duplicate) - fixes kmonad#705 (already closed) - fixes kmonad#907 (duplicate) - resolves kmonad#524 (original merge request, with conflicts)
This is a long standing issue, so it fixes a few things: - fixes kmonad#256 - fixes kmonad#466 (duplicate of kmonad#256 or kmonad#111) - fixes kmonad#523 (duplicate) - fixes kmonad#705 (already closed) - fixes kmonad#907 (duplicate) - resolves kmonad#524 (original merge request, with conflicts)
This is a long standing issue, so it fixes a few things: - fixes kmonad#256 - fixes kmonad#466 (duplicate of kmonad#256 or kmonad#111) - fixes kmonad#523 (duplicate) - fixes kmonad#705 (already closed) - fixes kmonad#907 (duplicate) - resolves kmonad#524 (original merge request, with conflicts)
|
While trying to implement a fix for #955, I noticed that when we hold and catch an event via This is especially relevant if we register a hook after we process them. If we want to be pedantic about it, hooks should receive the keyevents iff they have been registered before the event. Since this is a bit harder to implement I suggest simply not reregistering events which have been caught (I hope this doesn't break other things). |
This is a long standing issue, so it fixes a few things: - fixes kmonad#256 - fixes kmonad#466 (duplicate of kmonad#256 or kmonad#111) - fixes kmonad#523 (duplicate) - fixes kmonad#705 (already closed) - fixes kmonad#907 (duplicate) - resolves kmonad#524 (original merge request, with conflicts)
This is a long standing issue, so it fixes a few things: - fixes kmonad#256 - fixes kmonad#466 (duplicate of kmonad#256 or kmonad#111) - fixes kmonad#523 (duplicate) - fixes kmonad#705 (already closed) - fixes kmonad#907 (duplicate) - resolves kmonad#524 (original merge request, with conflicts)
This is a long standing issue, so it fixes a few things: - fixes kmonad#256 - fixes kmonad#466 (duplicate of kmonad#256 or kmonad#111) - fixes kmonad#523 (duplicate) - fixes kmonad#705 (already closed) - fixes kmonad#907 (duplicate) - resolves kmonad#524 (original merge request, with conflicts)
This is a long standing issue, so it fixes a few things: - fixes kmonad#256 - fixes kmonad#466 (duplicate of kmonad#256 or kmonad#111) - fixes kmonad#523 (duplicate) - fixes kmonad#705 (already closed) - fixes kmonad#907 (duplicate) - resolves kmonad#524 (original merge request, with conflicts)
Hi all,
this is meant to address #256, release events not being held when using
tap-next-release. From studying the code, this seems to be the least invasive solution to this problem.The issue stems from the way KMonad handles key releases in relation to the pull chain. The way I understand it, the pull chain consists of dispatch, hooks, and sluice. Dispatch listens to incoming key events, passes them to the hooks layer, where key events can be caught and additional actions triggered. Events that are not caught then arrive at the sluice, where event processing can be held. If held and then released, cached events are being re-inserted into dispatch. If not held, events are being processed and lookup of buttons occurs. In this system, releases are handled via hooks: once a button press is handled after passing through the sluice, a hook is registered that emits the correct key release upon receiving the corresponding event.
The issue is that when a key has already been pressed before processing was held and the key is then released, its release event is being emitted immediately because the release hook has already been registered, and its emit statement is therefore not being held up at the sluice.
My proposed solution seems like the least invasive way to approach this and to reuse most of the existing infrastructure. Instead of having an
emitstatement in the release hook, we are injecting the corresponding key event into dispatch, therefore allowing it to be blocked by the sluice, just as presses are blocked.A potential problem with this is that a release event could pass through the hooks layer twice, so one key release could trigger multiple release hooks to be fired. My proposed solution is to flag key events specifically as already having been processed, thus safeguarding them from the hooks and any lookup in the button hashmap. This PR introduces a
WrappedKeyEventtype that wraps a key event and additionally has apassthroughfield that indicates whether an event should be handled by the hooks system and lookup up in the hashmap or not.I'm pretty inexperienced with Haskell (just started re-learning it), so the code might be pretty bad. I would consider it a starting point to discuss this or other solutions to #256 and would love some comments to improve my coding style. In particular, one issue I had is that in some places, I wasn't able to use the (lenses type?) lookup with
(^.)for the fields of the new type I introduced. I would love to know how to do that better.Edit: One thing that is not covered by this is other "Button" release actions. Maybe extending the sluice to be able to handle Actions instead of KeyEvents might be the way to go? In that case, a container like WrappedKeyEvent could contains events or actions?