Rework TUI

[hasufell]

We need a more compact view, so we have two panes now.

• I have read https://www.haskell.org/ghcup/dev/
• I did not use an LLM to generate this patch or parts of it

---

[lsmor]

Hi. It's been a while since I saw the code for the last time. I have built it succesfully, but I can't run the tui

❯ cabal run exe:ghcup -- tui
Configuration is affected by the following files:
- cabal.project
[ Info  ] downloading: https://raw.githubusercontent.com/haskell/ghcup-metadata/master/ghcup-0.0.10.yaml as file /home/luis/.ghcup/cache/ghcup-0.0.10.yaml
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0    14    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (22) The requested URL returned error: 404
[ Warn  ] Could not get download info, trying cached version (this may not be recent!)
[ ...   ] If this problem persists, consider switching downloader via: 
[ ...   ]     ghcup config set downloader Wget
[ Error ] [GHCup-00160] JSON decoding failed with: YAML exception:
[ ...   ] Yaml file not found: /home/luis/.ghcup/cache/ghcup-0.0.10.yaml
[ ...   ] Consider removing /home/luis/.ghcup/cache/ghcup-0.0.10.yaml manually.

Following the advise at errors.haskell.org I have tried to download manually:

❯ curl https://raw.githubusercontent.com/haskell/ghcup-metadata/master/ghcup-0.0.10.yaml
404: Not Found

Am I missing something?

[hasufell]

You just have to set the previous url:

ghcup -s https://raw.githubusercontent.com/haskell/ghcup-metadata/refs/heads/develop/ghcup-0.0.9.yaml tui

[hasufell]

TODO:

• fix verbose parsing for backwards compat

[lsmor]

I find a little bit bogus the current behaviour in the tool list. When an action-key is pressed (i(nstall) | u(ninstall) | s(et)| c(hange log)) the tui executes such action with (I guess) the last element of the list. That's very odd, and a source of errors. I would suggest to not execute any action unless the focus is on the version list.

the most straightfoward change would be to eliminate those keybindings from keyHandlersToolList, but I am not sure if that's the most "elegant" solution. Probably it is. The patch below is all you need (Also include normalization of capital letters for Help and Advance options )

diff --git a/lib-tui/GHCup/Brick/Actions.hs b/lib-tui/GHCup/Brick/Actions.hs
index 1f14aab7..b79dae66 100644
--- a/lib-tui/GHCup/Brick/Actions.hs
+++ b/lib-tui/GHCup/Brick/Actions.hs
@@ -724,16 +724,12 @@ keyHandlersToolList :: KeyBindings
                ]
 keyHandlersToolList KeyBindings {..} =
   [ (bQuit, Just $ const "Quit"     , Brick.halt)
-  , (bInstall, Just $ const "Install"  , withIOAction install')
-  , (bUninstall, Just $ const "Uninstall", withIOAction del')
-  , (bSet, Just $ const "Set"      , withIOAction set')
-  , (bChangelog, Just $ const "ChangeLog", withIOAction changelog')
   , ( bShowAllVersions
     , Just $ \BrickSettings {..} ->
        if _showAllVersions then "Don't show all versions" else "Show all versions"
     , hideShowHandler' (not . _showAllVersions)
     )
-  , (KeyCombination (Vty.KChar 'h') [], Just $ const "help", mode .= KeyInfo)
+  , (KeyCombination (Vty.KChar 'h') [], Just $ const "Help", mode .= KeyInfo)
   , (KeyCombination Vty.KEnter [], Just $ const "Show tool details", mode .= Common.ToolInfo )
   , (KeyCombination KLeft [], Nothing, versionFocus .= False)
   , (KeyCombination KRight [], Nothing, versionFocus .= True)
@@ -757,8 +753,8 @@ keyHandlersVersionList KeyBindings {..} =
        if _showAllVersions then "Don't show all versions" else "Show all versions"
     , hideShowHandler' (not . _showAllVersions)
     )
-  , (KeyCombination (Vty.KChar 'h') [], Just $ const "help", mode .= KeyInfo)
-  , (KeyCombination Vty.KEnter [], Just $ const "advanced options", createMenuforTool )
+  , (KeyCombination (Vty.KChar 'h') [], Just $ const "Help", mode .= KeyInfo)
+  , (KeyCombination Vty.KEnter [], Just $ const "Advanced options", createMenuforTool )
   , (KeyCombination KLeft [], Nothing, versionFocus .= False)
   , (KeyCombination KRight [], Nothing, versionFocus .= True)
   , (KeyCombination (Vty.KChar '\t') [], Nothing, versionFocus %= not)

[lsmor]

The new definition of BrickInternalState makes every pattern match very verbose, in general.

[hasufell]

the tui executes such action with (I guess) the last element of the list

It actually doesn't. It picks the recommended version, which I think makes sense. But we should probably make this more clear.

[lsmor]

I see versionFocus is used to dispatch events either to the tool list or the version list. Usually the field _mode is meant for that.

[hasufell]

data Mode = Navigation
          | KeyInfo
          | Tutorial
          | ContextPanel
          | AdvancedInstallPanel
          | CompileGHCPanel
          | CompileHLSPanel

Both the tool and the version list are part of Navigation though. Are you suggesting to add a Bool to.the Navigation constructor?

[lsmor]

yes, Kind of. I think It is more consistent with the current API

-- Code not tested. Just an Idea

-- ******************
--      Common.hs
-- ******************
data NavigationFocus = ToolList | ToolVersionList  -- morally eq. to Bool
data Mode = Navigation NavigationFocus -- You can also model this as data Mode = NavigationTool | NavigationVersion | KeyInfo ...
          | KeyInfo
          | ToolInfo
          | Tutorial
          | ContextPanel
          | AdvancedInstallPanel
          | CompileGHCPanel
          | CompileHLSPanel
          deriving (Eq, Show, Ord)

-- ******************
--      App.hs
-- ******************
-- Split navigation in two handlers

navigationToolHandler :: BrickEvent Name e -> EventM Name BrickState ()
navigationToolHandler ev = do
  BrickState{..} <- get
  AppState { keyBindings = kb } <- liftIO $ readIORef Actions.settings'
  case ev of
    inner_event@(VtyEvent (Vty.EvKey key mods)) -> 
          case find (\(key', _, _) -> key' == KeyCombination key mods) (Actions.keyHandlersToolList kb) of
            Just (_, _, handler) -> handler
            Nothing -> void $ Common.zoom appState $ Navigation.handler _versionFocus inner_event
    inner_event -> Common.zoom appState $ Navigation.handler _versionFocus inner_event

navigationVersionHandler :: BrickEvent Name e -> EventM Name BrickState ()
navigationVersionHandler ev = do
  BrickState{..} <- get
  AppState { keyBindings = kb } <- liftIO $ readIORef Actions.settings'
  case ev of
    inner_event@(VtyEvent (Vty.EvKey key mods)) -> 
          case find (\(key', _, _) -> key' == KeyCombination key mods) (Actions.keyHandlersVersionList kb) of
            Just (_, _, handler) -> handler
            Nothing -> void $ Common.zoom appState $ Navigation.handler _versionFocus inner_event
    inner_event -> Common.zoom appState $ Navigation.handler _versionFocus inner_event

... 

-- Use the mode to dispatch to each handler
eventHandler :: BrickEvent Name e -> EventM Name BrickState ()
eventHandler ev = do
  m <- use mode
  case m of
    KeyInfo      -> keyInfoHandler ev
    ToolInfo     -> toolInfoHandler ev
    Tutorial     -> tutorialHandler ev  
    Navigation ToolList  -> navigationToolHandler ev             -- <<< ********
    Navigation VersionList  -> navigationVersionHandler ev
    ContextPanel -> contextMenuHandler ev
    AdvancedInstallPanel -> advancedInstallHandler ev
    CompileGHCPanel     -> compileGHCHandler ev
    CompileHLSPanel     -> compileHLSHandler ev

-- ******************
--     Navigation.hs
-- ******************
-- This should change too.  But I am not so sure how
handler :: Bool -> BrickEvent Common.Name e -> EventM Common.Name BrickInternalState ()
handler = ...

I mean, there is nothing deeply wrong about using versionFocus but the TUI already has the Mode data type for this usage. Just to be consistent.

[hasufell]

That doesn't work.

When you spawn a dialog popup, you now lose the information on which panel of the navigation menu was focused. So once you quit the dialog popup, you don't know what part of the navigation to focus.

[lsmor]

Regarding TUI. Looks good to me. I still don't like the big tuple in the internal state haha, but I guess it is reasonable.

[lsmor]

By the way, I'll be in a bussiness travel the next week so I will be unresponsive.