API change suggestion: make the downs/ups/presses function selective via Maybe

[jvoigtlaender]

This was discussed on the mailing list, in this thread: https://groups.google.com/forum/#!topic/elm-discuss/Ud7WzAhRsqE

There was a lot of support, and no detractors I think.

---

The package currently contains these functions:

Keyboard.downs : (KeyCode -> msg) -> Sub msg
Keyboard.ups : (KeyCode -> msg) -> Sub msg
Keyboard.presses : (KeyCode -> msg) -> Sub msg

Common uses (I'll point to several repositories below) are such that only some keys are relevant for an application. My proposal is to replace the above functions by:

Keyboard.downs : (KeyCode -> Maybe msg) -> Sub msg
Keyboard.ups : (KeyCode -> Maybe msg) -> Sub msg
Keyboard.presses : (KeyCode -> Maybe msg) -> Sub msg

where the semantics is that if a given KeyCode is mapped to Nothing by the tagger, then no message gets sent along the subscription; otherwise the Just is peeled off and the message gets sent. (Thus, the current behavior could simply be recovered by unconditionally throwing in Just, if that's what a programmer really wants in a specific situation. I posit that this is almost never the case.)

---

Let's look at a practical case, https://github.com/arpad-m/dontfall. It's a game, where the player uses the keyboard for part of the control. Important excerpts from the code are:

The message type (in https://github.com/arpad-m/dontfall/blob/master/src/BaseStuff.elm):

type GameMsg = NothingHappened | ... several other messages ...

The subscriptions definition (in https://github.com/arpad-m/dontfall/blob/master/src/main.elm):

subscriptions : GameData -> Sub GameMsg
subscriptions d =
     Sub.batch
         ([ Keyboard.downs (\c -> if Char.fromCode c == 'P' then PauseToogle else NothingHappened) ] ++
             if d.state == Running then
                 [ AnimationFrame.diffs Tick
                 , Keyboard.downs (\c -> if Char.fromCode c == ' ' then JumpDown else NothingHappened)
                 , Keyboard.ups (\c -> if Char.fromCode c == ' ' then JumpUp else NothingHappened)
                 ]
             else
                 [])

The main case distinction in the main update function (in https://github.com/arpad-m/dontfall/blob/master/src/main.elm):

updateScene : GameMsg -> GameData -> (GameData, Cmd GameMsg)
updateScene msg d =
     (case d.state of
         ...
         Running -> case msg of
             MouseMove (x,_) -> { d | characterPosX = min x d.flWidth}
             Tick t -> stepTime d t
             PauseToogle -> { d | state = Paused }
             JumpDown -> { d | jumpPressed = True }
             JumpUp -> { d | jumpPressed = False }
             _ -> d
     , Cmd.none
     )

Given the keyboard API change I propose above, the code could instead look as follows:

type GameMsg = ... only the other messages, no NothingHappened ...

subscriptions : GameData -> Sub GameMsg
subscriptions d =
     Sub.batch
         ([ Keyboard.downs (\c -> if Char.fromCode c == 'P' then Just PauseToogle else Nothing) ] ++
             if d.state == Running then
                 [ AnimationFrame.diffs Tick
                 , Keyboard.downs (\c -> if Char.fromCode c == ' ' then Just JumpDown else Nothing)
                 , Keyboard.ups (\c -> if Char.fromCode c == ' ' then Just JumpUp else Nothing)
                 ]
             else
                 [])

updateScene : GameMsg -> GameData -> (GameData, Cmd GameMsg)
updateScene msg d =
     (case d.state of
         ...
         Running -> case msg of
             MouseMove (x,_) -> { d | characterPosX = min x d.flWidth}
             Tick t -> stepTime d t
             PauseToogle -> { d | state = Paused }
             JumpDown -> { d | jumpPressed = True }
             JumpUp -> { d | jumpPressed = False }
     , Cmd.none
     )

Advantages:

1. simpler message type, no special role no-op constructor needed
2. no spurious update and render cycles while the game is running
3. less room for bugs in the update logic

Some additional comments on the latter two of these points:

Re 2., given the current implementation, whenever a key is hit that is not relevant, the update function is still called and produces an unchanged model, which is then rendered, which is extra/useless work. Since the game uses Graphics.*, no use can be made of Html.Lazy.* to avoid the re-rendering. Even if something like Graphics.Lazy.* were available, having to use it would not be as nice/pure as not causing those spurious updates in the first place.

Re 3., given the current implementation, there is both more room for bugs in the now and in a potential later, when extending the game. In the now, the programmer has to make sure that NothingHappened does indeed not change the model. Concerning later, imagine that the programmer extends the message type for some reason. With the current version of updateScene, the programmer might forget to actually add a branch for handling the new message, and the compiler would not catch that, because of the _ -> d branch that will silently catch not only NothingHappened but also the new message which was actually supposed to make something happen. With the version of updateScene after the proposed change, the situation would be different. Since there is no _ -> d branch in that Running -> case msg of ... part anymore (thanks to NothingHappened not being a thing), the compiler will immediately complain if the message type is extended but the new message is not handled there. Bug prevented.

---

It's not only this single project. I have observed students applying different strategies to deal with "Not all keys are relevant to my program". In each case, using an API with functions of type (KeyCode -> Maybe msg) -> Sub msg instead of (KeyCode -> msg) -> Sub msg would have been conceptually nicer and would have simplified things.

Some more example repos:

• https://github.com/chemmi/elm-rocket, uses type Key = Left | Right | ... | NotBound and keyBinding : KeyCode -> Key and then needs to make sure to correctly (non)-deal with NotBound in functions like updateKeyDown; whereas just not having NotBound, but having keyBinding : KeyCode -> Maybe Key and using that in a call to a (KeyCode -> Maybe msg) -> Sub msg function would simplify things with the same benefits as in the above more fully elaborated example case.
• https://github.com/Dinendal92/Abschlussprojekt-DP2016, less complete project, but with same approach and issues as in the preceding example, using type Key = Space | Unknown and fromCode : Int -> Key. Here, since eliminating Unknown would turn Key into a type with only one constructor, even more conceptual simplifications would be enabled after a switch to the (KeyCode -> Maybe msg) -> Sub msg approach.
• https://github.com/Shaomada/Elm-Project, quite elaborate project, structured according to TEA, uses no special Key type, instead maps with Char.fromCode in the calls to the keyboard subscriptions, then has to case dispatch on actual Chars at several places distributed over the update functions of the TEA subcomponents. Subscribing with (KeyCode -> Maybe msg) -> Sub msg functions should allow to eliminate branches at some of those places, removing redundancies and room for bugs.
• https://github.com/Sulring/elmaction, similar story (without TEA)

---

Bottom line claim:

One almost never really wants to capture all keys. Usually, one wants to be selective. That selectiveness has to be expressed somewhere, and doing it with the Maybe type that is designed for such purpose is generally better than going for something like extra NoOp or NothingHappened or NotBound constructors that then need to be handled in a special way in the app’s update logic, without help from the type system. Making consideration of the selectiveness part of the modelling up front would lead to better design. That’s at least the case in the projects/repositories I have pointed to.

[jtanguy]

If it helps, I made the suggested changes to my fork at jtanguy/elm-keyboard. The changes were made naively by adding a Maybe and fixing errors with the compiler's help.
I tried to look for some documentation on effect modules, but did not find any, so I cannot tell if there is a better implementation of this.

I'd be happy to continue this discussion in a PR if need be; the docs might need some update to explain the KeyCode -> Maybe msg part.

[jvoigtlaender]

Thanks for doing this. I'm not sure, though, whether it helps in the sense of moving this issue along and getting the changed (I claim: improved) API accepted into this package here. According to @evancz, code is the easy part. So what keeps this matter stalled here is not that there is no PR with an implementation, but instead that he has either not yet come around to consider the issue at all, or not made up his mind about the change. Or that he is waiting for or considering some other, more radical API change, or ... I don't know.

[jtanguy]

I understand, I was hoping to help move this issue forward.
This proposal is great for selecting keys and reduce the amount of messages in the debugger (espacially if you maintain a game, the list grows quite rapidly).

Thinking of it, I see one potential problem with subscribing with a Keycode -> Maybe msg function.
If you forget to specifically add a new key to the captured keys, its presses will never show up in the debugger.

There are alternative design possibles though, for instance providing the list of captured Keycodes when subscribing, or making a more general filter function in core, along those lines

-- In Platform.Sub
filter : (msg -> Bool) -> Sub msg -> Sub msg

[jvoigtlaender]

Let me just beg anyone coming by here to not engage in discussion of the "general filter function" alternative. That's what another thread on the mailing list was about, while the thread I referenced at the very top here, as well as this whole issue here, deliberately does not want to consider such a general functionality.

[jtanguy]

I'm sorry, I must have misunderstood the wish to avoid the general filter functionality. I evoked this idea because I don't see what's preventing this issue from moving forward with the current proposal.

Is there something I can do to help with this issue? I'll try to rewrite all the parts of the aforementioned projects, plus other public projects for comparison if that helps

[jvoigtlaender]

It depends on @evancz is all I can say.