-
Notifications
You must be signed in to change notification settings - Fork 96
Remove existential types in TUI #1217
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
Comments
Ideally the need for lenses should go away, along with the existential types.( Other good to have things in the advanced install/compile menus.
|
TLDR; Removing existentials would require a whole rewrite of the menus, because current implementation models a menu as a Form, i.e. a list of fields. That list must be heterogenous if we want to pair each field in the form with a field in a record type. Other desing options is having two separated structures and match manually input with field in a record. This implies having a bunch of logic for each menu and lack of re-usability Just a brief understanding of this. That module is a copy/adaptation of Brick's Form widget, but with optics instead of microlenses (Brick uses microlenses). If I remmeber correctly, there was a good reason to do so. I am talking from memory but I thing the argument goes like this The general argument goes like this
There are two obvious ways of implementing this.
I decided to go with the first one because the idea was to have many menus, so we need a way to write those manus easily. The only problem was that Brick uses microlenses, so we needed to add it as a dependency, something undesired back then. Hence, I went for the adaptation of 1. but in reallity we are pretty much doing the same as Brick's Forms. Let me first summarize how option two would look alike and why it is a bad option IMO, and later explain the existential types For option two (custom form-like logic) we need to follow the same workflow "take input" -> "validate" -> "build record" so -- made up types for the sake of example
data GHCCompilationOptions = {gchversionOption :: ToolVersion, isolatedpathOption :: Path, ...} -- struct to hold the options
data GHCCompilationInputs n = {ghcVersionInput :: Edit.Editor T.Text n, isolatedPathIntput :: Edit.Editor T.Text n, ...} -- A copy of the Option Struct but with brick widgets as types, so we can store user input state
-- validators fro each field
validateGHCversion :: Edit.Editor T.Text n -> Either ErrorMessage ToolVersion
validateIsolatedPath :: Edit.Editor T.Text n -> Either ErrorMessage Path
...
data GHCCompilationMenu n = {
options :: GHCCompilationOptions
, brickState :: GHCCompilationInputs
, ring :: FocusRing n}
-- This is where things get dirty.
handler :: BrickEvent n () -> EventM n b ()
handler = ...
-- my guess is that this is way more difficult and error prone with this approach,
-- because you have to match every xyzInput with the xyzOption and focus ring
-- There is no way yo ucan abstrac this away for different ManuOptions-like types,
-- you have to do for every manu one by one
draw = -- here you face the same problem as with handler So the option above, implies to define sort of the same logic (handler, drawing, focus ring, etc...) for every menu individually The option I went for allow to treat every Form as a list of field. So you implement all the logic for a list of fields instead of a record -- Alternative implementation
data GHCCompilationOptions = {gchversion :: ToolVersion, isolatedpath :: Path, ...}
data GHCCompilationInputs n = {ghcVersionInput :: Edit.Editor T.Text n, isolatedPathIntput :: Edit.Editor T.Text n, ...}
-- Current implementation (sort of)
-- Notice that a and b could be different for each field. a is the type of xyzOption and b is the type of user input
-- Hence, the fields list is herterogenous!!
data Menu s = {state :: s, fields: [ {userInput :: b, handler :: BrickEvent n () -> EventM n b (), lens :: Lens' s a } ]}
-- example
ghcversionField = {
userInput :: Text -- user inputs text in the tui
, handler :: BrickEvent n () -> EventM n Text () -- the Tui get events and modify the text
, lens: Lens' GHCCompilationOption ToolVersion -- this field can watch the tool vesion of the Manu state
} Does htis makes sense to you? Unfortunatelly, my time is scarce (recently I had a baby) and I can't build a complete example of this. But be aware of the inconvenience triying to remove existential. |
@lsmor no worries, I now have a somewhat working prototype to address this, and a few more things regarding the tui widgets implementation. This design does make use of There is now a proper abstraction to define a widget so that the instances contain both the rendering and event handling logic.
The overall structure of the app will be like a tree of widgets where each widget can have an overlay and the nesting can go arbitrarily deep. The handling of overlay and passing of events around can now be extracted into a separate instance, and removes a decent amount of mess.
The
There can be better reuse and composability of smaller widgets. The The boilerplate of drawing and event handling of these menu fields will now be handled via instances of How does this look @hasufell ? |
ghcup-hs/lib-tui/GHCup/Brick/Widgets/Menu.hs
Lines 111 to 153 in d94ce2a
These types must go. It's impossible to derive or write a lens for
fieldInput
.The text was updated successfully, but these errors were encountered: