hledger icon indicating copy to clipboard operation
hledger copied to clipboard

Partial record accessors in hledger-ui

Open mitchellwrosen opened this issue 9 years ago • 5 comments

Screen is defined as a sum type with record accessors that differ in each data constructor. So, e.g.

tsTransaction :: Screen -> NumberedTransaction

is a partial function that only works for TransactionScreen.

It would probably be smart to refactor this data type.

mitchellwrosen avatar Jun 18 '16 16:06 mitchellwrosen

I'm not happy with it either, but I haven't found a better way to structure the types yet..

simonmichael avatar Jun 18 '16 16:06 simonmichael

How about this?

class Screen s where
  ... some stuff, probably sInit, sDraw, sHandle ...

data AccountsScreen = ...
instance Screen AccountsScreen

data TransactionsScreen = ...
instance Screen TransactionsScreen

data SomeScreen = forall s. Screen s => SomeScreen s

data UIState = ...
  { screens = [SomeScreen], ... }

mitchellwrosen avatar Jun 18 '16 16:06 mitchellwrosen

I like the sound of it! If you get some time, please give it a try and see how it works out.

simonmichael avatar Jun 18 '16 16:06 simonmichael

Hm, I looked into this a bit. I think because of the tangling between Screen and UIState it won't be possible to do what I outlined above - specifically, you need to be able to peek inside a UIState and know what Screen is active, because sInit, sDraw, and sHandle all take a UIState as input, where the active screen is assumed to be the type that sInit is called on (errors otherwise).

So, I first tried adding a type parameter to UIState to track the current screen, then ended up tracking the entire stack of previous screens, and ended up bringing in way too much type-level programming, hitting weird hard-to-type functions like regenerateScreens and resetScreens.

Anyways, back to the original problem of partial record selectors, I think it could be solved rather simply. Instead of

data Screen
  = AccountScreen { sInit :: ... }
  | TransactionScreen { sInit :: ... }

have:

data Screen 
  = AScreen AccountScreen
  | TScreen TransactionScreen

data AccountScreen = AccountScreen { asInit :: ... }

data TransactionScreen = TransactionScreen { tsInit :: ... }

Then, if you want one vocabulary for init, handle, and draw, just use

class ScreenClass s where
  sInit :: s -> ...
  sDraw :: s -> ...
  sHandle :: s -> ...

instance ScreenClass AccountScreen
instance ScreenClass TransactionScreen

Basically, it's the same solution as above, but without the existential SomeScreen that UIState carries around, because it needs to know what screen it actually is.

mitchellwrosen avatar Jun 18 '16 18:06 mitchellwrosen

It's a bit tricky isn't it. Almost makes you wish for plain old object-oriented code. As you rightly say, we don't want to make this complex and harder to understand. Let me know how the above works out.

simonmichael avatar Jun 18 '16 19:06 simonmichael