Context Menu

[lsmor]

Create popUp widget: widget state, widget drawing and widget handler. The widget is pretty straigthfoward

I have to implement the instalation logic still. I am gonna need some help with it. I understand how to pass some advance parameters, but not others. The whole list of parameters is:

• url
• set
• isolate
• force
• configure Args

If we translate that into code

--                                           |- Change be IsolateDir ... if parameter isolate is set
--                                           |             |- change to True is param force is set.
installGHCBin (GHCTargetVersion lCross lVer) GHCupInternal False [] $> (vi, dirs, ce)
--                                                               |- Is this list for the rest of params??

[hasufell]

Nice.

Although it seems this popup is for installation? We want it for ghcup compile hls.

[lsmor]

Although it seems this popup is for installation? We want it for ghcup compile hls.

Ouch! I got confuse in the conversation. If the new feature is for ghcup compile, how would the user interface be? Another hot-key?

[hasufell]

Although it seems this popup is for installation? We want it for ghcup compile hls.

Ouch! I got confuse in the conversation. If the new feature is for ghcup compile, how would the user interface be? Another hot-key?

I think we're going to run out of hotkeys.

I suggest something like "context menu", maybe defaulting to Enter. Then it has select menu which we may extend in the future.

[lsmor]

I suggest something like "context menu", maybe defaulting to Enter. Then it has select menu which we may extend in the future.

I think I am missing some ghcup knowledge here. From my understanding ghcup compile is a command like ghcup install, or ghcup set. So, Is the "context menu" in fact a "compile menu"? I mean, instead of adding a hot-key for compile, we have a popup with a big "compile" button and a buch of options (following ghcup compile hls --help docs). Maybe there is something about compile I am missing...

For reference I've tried to implement this comment ideas.

[hasufell]

So, Is the "context menu" in fact a "compile menu"?

Let's say the context menu for now will have only one button that leads you to said compile menu.

We can add more stuff as we go.

[lsmor]

We can add more stuff as we go.

I am playing around with this. Would it be useful to add an small Menu widget utilities? It would be some default widgets for easily creating/composing menus. I am thinking three options data MenuEntry = Button | EditBox | CheckBox. Then you defined your menu as

-- Missing internal state bits for each variant
data MenuEntry = Button | EditBox | CheckBox

-- oversimplification
data CompileMenu =  CompileMenu {gitRefEditBox :: MenuEntry, setCheckBox :: MenuEntry, okButton :: MenuEntry}
newCompileMenu = CompileMenu EditBox CheckBox Button

[hasufell]

Yeah, why not. We can always change things. Code decisions here are low impact, because they can be easily reverted.

It's harder with our decisions on how to organize installed files. They are hard to revert.

[runeksvendsen]

Hi @lsmor

Thank you for your efforts in getting this implemented!

I want to help you get this over the finish line.

To achieve this, I think we first need a clear specification of exactly what it is we want to implement. I have read the linked issue (#706) as well as the comments in this PR, and below is my suggestion for a final spec (for this PR):

1. A new "context menu" is added
   1. This is a small popup widget (overlaying part of the main TUI widget) that appears when the user presses ENTER on the currently selected tool (GHCup/Stack/HLS/cabal/GHC) in the TUI
   2. The title of this popup widget is Context menu (<tool name> <tool version>), for example Context menu (GHC 8.10.7)
   3. The widget contains one or more actions relevant to the given tool. Each action is a line that can be navigated to using the up/down keys and entered by pressing ENTER. All configuration for the given action happens in the widget that appears after pressing ENTER on an action (to keep the context menu clean).
   4. Pressing ESCAPE closes the context menu (thus showing the default TUI view)
2. Generally speaking, this context menu will contain additional actions relevant to the selected tool in question. Thus, the content of the context menu may differ depending on which tool is selected. For example:
   1. For GHC only there may be a "compile HLS for this GHC version"-item
   2. For all tools:
      1. An "advanced installation" item, which allows configuring advanced options such as "isolated install"/"force install"/"compile instead of downloading bin-dist" (the options displayed by install --help)
      2. A "compile" item, which acts as a TUI for the ghcup compile command
   3. For HLS only we can imagine being able to deselect plugins

Example context menu for GHC 8.10.7:

┌────────────────────────────────────────GHCup────────────────────────────────────────┐
│    Tool  Version         Tags                          Notes                        │
│─────────────────────────────────────────────────────────────────────────────────────│
│                                                                                     │
│─────────────────────────────────────────────────────────────────────────────────────│
│                                                                                     │
│          ┌───────────────────Context menu (GHC 8.10.7)───────────────────┐          │
│          │┌────────┐                                                     │          │
│          ││Compile │                                                     │          │
│          │└────────┘                                                     │          │
│          │┌─────────────────────┐                                        │          │
│──────────││Advanced installation│                                        │──────────│
│          │└─────────────────────┘                                        │          │
│          │┌────────────────────────────────────────┐                     │          │
│          ││Compile HLS for this GHC version        │                     │          │
│          │└────────────────────────────────────────┘                     │          │
│          └───────────────────────────────────────────────────────────────┘          │
│                                                                                     │
│                                                                                     │
│─────────────────────────────────────────────────────────────────────────────────────│
│                                                                                     │
└─────────────────────────────────────────────────────────────────────────────────────┘
q:Quit  i:Install  u:Uninstall  s:Set  c:ChangeLog  a:Show all versions  ↑:Up  ↓:Down
h:help

Note that the above is only about the nature of the "context menu", which allows putting more advanced features into the TUI without cluttering the main view and adding additional hot keys.

Does this sound reasonable? @hasufell @arjunkathuria @david-christiansen @Kleidukos

If we agree on this, then it's up to you @lsmor to decide which of the above context menu features you want to implement. It looks like you've started to implement the "advanced installation"-item feature. The goal of this PR can then be adding (1) the context menu itself; and (2) whatever context menu feature @lsmor wants to implement.

If @lsmor wants to continue with the "advanced installation"-item feature then a subsequent PR can add the "compile HLS for this GHC version"-feature.

[lsmor]

Code decisions here are low impact, because they can be easily reverted.

Well, given the amount of features for this (I wont implement all of them in this PR though), I think It is worth to split the brick related stuff into another component (say lib-tui), instead of having a single file. So I am taking this quote literal, and do It. The only thing I wonder is if this is going to break CI for some reason. In principle I am not planning to break any cabal flag so I guess there should be no problem

I want to help you get this over the finish line.

Thanks, actually I had somethings wrong. This design helped to get things clearer

[runeksvendsen]

Thanks, actually I had somethings wrong. This design helped to get things clearer

Great! I'm happy to hear that.

I have to implement the instalation logic still. I am gonna need some help with it. I understand how to pass some advance parameters, but not others. The whole list of parameters is:

• url
• set
• isolate
• force
• configure Args

If we translate that into code

--                                           |- Change be IsolateDir ... if parameter isolate is set
--                                           |             |- change to True is param force is set.
installGHCBin (GHCTargetVersion lCross lVer) GHCupInternal False [] $> (vi, dirs, ce)
--                                                               |- Is this list for the rest of params??

I'm not that familiar with the GHCup codebase, but after looking around a bit, I can see that all the relevant options are contained in the data type InstallOptions (in the GHCup.OptParse.Install module).

So I would prefer to go a few levels up from installGHCBin, and calling GHCup.OptParse.Install.install. This takes an InstallOptions as argument and ultimately calls installGHCBin. This should also work for all tools instead of just GHC, which we need because the Advanced installation action should be in the Context Menu for all tools.

Generally speaking, I prefer to call some sort of eval function which takes a single data type as argument that specifies everything to do. This avoids having to call specific functions with specific arguments in the right place. In this case the install function seems to behave this way along with the InstallCommand data type.

@hasufell does the above sound reasonable to you? I'm not familiar with how various options are passed around by GHCup, so calling GHCup.OptParse.Install.install from an action within the Context menu might be problematic.

[hasufell]

@hasufell does the above sound reasonable to you? I'm not familiar with how various options are passed around by GHCup, so calling GHCup.OptParse.Install.install from an action within the Context menu might be problematic.

I'm not sure I agree. GHCup.OptParse.Install.install was written for the cli interface and assumes cli when doing error handling, logging, etc.

My approach in such cases, where it's not clear what the design overlap is, usually is to rather duplicate code and let us explore what actually ends up similar. Abstraction is, imo, discovered and not actively sought.

Here I'm not sure what a good design is that abstracts over cli and tui. A pre-existing interface may just skew your creativity when coming up with a good API for the tui to consume.

Also remember that this creates coupling and refactors over the optparse code would also affect the tui.

That said, I'd probably expect a small common denominator between cli and tui in a separate sublibrary or the main library, if one really wants to avoid code/logic duplication. So you might end up with 3 ADTs:

• TUI ADT
• opt-parse ADT
• GHCup library ADT, which the other two are converted to

And now I'm not sure it's worth it. But feel free to give it a try.

[runeksvendsen]

@hasufell thank you for your input! You make several good points.

And now I'm not sure it's worth it.

I agree. Let's go with the simple solution for now.

@lsmor I will try again to answer your question — please ignore my comment above :-)

• url
• set
• isolate
• force
• configure Args

If we translate that into code

--                                           |- Change be IsolateDir ... if parameter isolate is set
--                                           |             |- change to True is param force is set.
installGHCBin (GHCTargetVersion lCross lVer) GHCupInternal False [] $> (vi, dirs, ce)
--                                                               |- Is this list for the rest of params??

As you've figured out, the parameters isolate, force and configure args are all arguments to installGHCBin.

If the url option is set (the CLI option called instBindist), then we call installGHCBindist instead of installGHCBin as is done here:

ghcup-hs/lib-opt/GHCup/OptParse/Install.hs

Lines 342 to 348 in df192ee

	liftE $ runBothE' (installGHCBindist
	(DownloadInfo uri (Just $ RegexDir "ghc-.*") "" Nothing Nothing)
	v
	(maybe GHCupInternal IsolateDir isolateDir)
	forceInstall
	addConfArgs
	)

and we also run the Excepts actions with the noVerify setting set to True:

ghcup-hs/lib-opt/GHCup/OptParse/Install.hs

Line 340 in df192ee

runInstGHC s'{ settings = settings {noVerify = True}} $ do

For the set option, the following line takes care of this:

ghcup-hs/lib-opt/GHCup/OptParse/Install.hs

Line 349 in df192ee

$ when instSet $ when (isNothing isolateDir) $ liftE $ void $ setGHC v SetGHCOnly Nothing

Let me know whether this clears things up.

[lsmor]

I am force pushing a commit with a big (but easy) change. I have moved the BrickMain.hs into its own library and split it in many modules. The original BrickMain.hs remains as in master branch. The reasons for this are:

• If we are willing to implement a few menus with advance options, one module becomes too much of a mess to handle
• If we gonna need more than one module, a library becomes a better approach (IMO) than many modules in an executable component
• (selfish reason) This wont be an easy PR, hence a lot of rebase / conflicts are expected. I am willing to re-implement changes in BrickMain.hs into the new library, but rebasing 10 commits each with many conflicts is just painfull (not a commit expert myself)

If any of you have concerns about the commit please ask. In general what I've done is to move "sections" from BrickMain.hs into modules in lib-tui. I have changed some types in order to break cyclic dependencies (but all changes are minor)

[runeksvendsen]

@lsmor I think this kind of change makes a lot of sense. Separating into smaller modules increases readability and the sub-library increases testability.

I don't have enough knowledge of the codebase to judge whether this particular organization of modules is the "right" one, but IMO if it works then it's good enough.

[runeksvendsen]

@hasufell what do you think about merging this as-is (without the "context menu"-changes)? It works for me, and I think it's a useful change.

[lsmor]

Hi there!

I'll be pushing some funcitonality soon. For now I've implemented only the visual part, without logic. Before continuing I'd like to have some feedback. Each tools has its own "Context Menu" (I'd prefer advance options, honestly), from there you can go to other settings like "compile", "install", etc...

Code is very messi, I am cleaning it a liitle bit before pushing.

[runeksvendsen]

I'll be pushing some funcitonality soon. For now I've implemented only the visual part, without logic. Before continuing I'd like to have some feedback. Each tools has its own "Context Menu" (I'd prefer advance options, honestly), from there you can go to other settings like "compile", "install", etc...

@lsmor awesome! Looks great.

I would make some small changes — provided it's not too difficult:

1. For the "advanced installation", have a help text (ie. description) of each argument. For example:
   1. what does "force" mean; and
   2. what am I supposed to put in the "isolated" text field?
   3. (the help texts are available here:

      ghcup-hs/lib-opt/GHCup/OptParse/Install.hs

      Lines 175 to 210 in 55030d8

      	installOpts :: Maybe Tool -> Parser InstallOptions
      	installOpts tool =
      	(\(u, v) b is f -> InstallOptions v u b is f)
      	<$> ( ( (,)
      	<$> optional
      	(option
      	(eitherReader uriParser)
      	(short 'u' <> long "url" <> metavar "BINDIST_URL" <> help
      	"Install the specified version from this bindist"
      	<> completer (toolDlCompleter (fromMaybe GHC tool))
      	)
      	)
      	<*> (Just <$> toolVersionTagArgument [] tool)
      	)
      	<|> pure (Nothing, Nothing)
      	)
      	<*> fmap (fromMaybe setDefault) (invertableSwitch "set" Nothing setDefault
      	(help $ if not setDefault then "Set as active version after install" else "Don't set as active version after install"))
      	<*> optional
      	(option
      	(eitherReader isolateParser)
      	( short 'i'
      	<> long "isolate"
      	<> metavar "DIR"
      	<> help "install in an isolated absolute directory instead of the default one"
      	<> completer (bashCompleter "directory")
      	)
      	)
      	<*> switch
      	(short 'f' <> long "force" <> help "Force install (THIS IS UNSAFE, only use it in Dockerfiles or CI)")
      	<*> many (argument str (metavar "CONFIGURE_ARGS" <> help "Additional arguments to bindist configure, prefix with '-- ' (longopts)"))
      	where
      	setDefault = case tool of
      	Nothing -> False
      	Just GHC -> False
      	Just _ -> True

      )
2. Include tool version in Context Menu title
3. Press ESC to go back to the main screen instead of having a "Cancel" button
   1. Show this in the Context Menu window in the same way keyboard shortcuts are shown in the main window, e.g. ESC:Back

[hasufell]

For the "advanced installation", have a help text (ie. description) of each argument. For example:

I think it should be possible to put that help text inside the input fields (if said input field is not focused). I think that's similar to how many web input fields work and saves space.

Press ESC to go back to the main screen instead of having a "Cancel" button

Maybe the same as in the main window, which by default is q. I think I avoided Esc, because it's more likely to be defined for the terminal window or something. It can still be set by the user to this key.

---

Otherwise I think this goes into the right direction.

[hasufell]

@lsmor if you rebase this branch against master, then CI should succeed.

[hasufell]

@lsmor let us know if you need any further feedback for the design. I believe this is a major improvement to the TUI.

[lsmor]

I am pushing (only) the visuals after rebasing. TODOS:

• migrate the fix Restore TUI scrolling to old behavior #987 to the new library
• add option's help message in grey font
• implement the visual for CompileMenu
• implement the logic for AdvanceInstallMenu
• implement the logic for CompileMenu
• Solve bug about q used for exit (see below)

[hasufell]

@lsmor what's the timeline you think you will have this finished? I'm evaluating whether to wait for it before making a release or doing a release sooner than later.

[lsmor]

@lsmor what's the timeline you think you will have this finished? I'm evaluating whether to wait for it before making a release or doing a release sooner than later.

um... I don't expect it to have it soon. the first two points are easy, and I can have them in the next week, but I still don't know how difficult will be to have the last two point working. + There should be some extensive testing which I don't know how to approach actually. So I would expect to have something testable in about 1,5 month. From there, fixing all possible bugs, etc... let say 2 or 3 months to merge.

Btw, I am terrible at ETA. Notice #850 , took me 5+ months (summer in the middle) and I estimated 3 months (and it was merge with a bug on my side!!!)... Also I am more familiar with the code base now.

[hasufell]

I am terrible at ETA

Well, I'm not trying to be a manager :D

Just trying to understand when to schedule releases. I think I'll start setting up a milestone then and only merge smaller stuff.

[lsmor]

Maybe the same as in the main window, which by default is q

I just recall, why this isn't a good Idea. You can either type q on a textbox, or q to exit. You could be intelligent enough and dispatch the q depending on whether or not a text box is focused, but thats brittle as it literally depends on pattern matching order... Also, it feels a little bit clumsy.

3. Press ESC to go back to the main screen instead of having a "Cancel" button

AS @hasufell said, adding a different key for exit, is risky as something like ESC might be dispatch to the terminal itself instead of the content within the terminal (example: Ctrl + Enter doesn't work on my gnome's terminal, probably because some colission with terminal's shortcuts)

¿is it ok if we keep the cancell button?

I think it should be possible to put that help text inside the input fields

I am adding this to the check list

[lsmor]

Hi there:

I have rebased master and think about @dfordivam feedback. In particular, these two suggestions:

• Allow specifying comma or space separated list of multiple target GHC
• a new field for "-g" option would be very useful.

The current design is that the target to compile is the version the cursor is over. So if you open the context menu of ghc-9.8.2 you'd compile that precise version. IMO, It would be a little bit weird if you open the menu but you can compile whatever you want. The same argument applies to cli's -g option, which is disable in both hls and ghc context menus.

I think is ok if this PR includes most of the functionality of the cli provides, sacrificing it a little bit of it for a better UX.

I don't know if you will be at Zurihac

I wasn't... sorry for the late response. 😅

[dfordivam]

Hi @lsmor

The current design is that the target to compile is the version the cursor is over. So if you open the context menu of ghc-9.8.2 you'd compile that precise version.

I think there is a slight confusion here, if you put the cursor on GHC, the advanced compile menu will open the options for compiling the GHC.

The user needs to put the cursor over HLS to open the options for compiling HLS.
In this case the target GHC will not be chosen, and the user will have to manually fill in the target GHC version(s).

"Allow specifying comma or space separated list of multiple target GHC" option is only valid in the case of HLS compilation, as HLS can be compiled for multiple GHC targets. For GHC this does not make sense.

Nevertheless, this is a usability related issue, and both of these can be tackled later, after merging this PR.
It may be nice to even create another advanced option menu, which allows the user to compile HLS when the cursor is over GHC, but that can also be done later.

The remaining fixes necessary for this PR are:

• There are a few hlint warnings, should be straightforward to fix.

• Tests are failing to compile, this should also be straightforward to fix

 test/optparse-test/SetTest.hs:27:13: error:
    • Data constructor not in scope:
        SetRecommended :: GHCup.Utils.Parsers.SetToolVersion
    • Perhaps you meant ‘Recommended’ (imported from GHCup.Types)
   |
27 |   [ ("set", SetRecommended)
   |             ^^^^^^^^^^^^^^

[lsmor]

The remaining fixes necessary for this PR are:

• There are a few hlint warnings, should be straightforward to fix.
• Tests are failing to compile, this should also be straightforward to fix

Done. There is a check failing but is I think is a problem with the host machine:

[ 1 of 10] Compiling Data.Hashable.FFI ( src/Data/Hashable/FFI.hs, dist/build/Data/Hashable/FFI.o, dist/build/Data/Hashable/FFI.dyn_o )
clang-13: error: the clang compiler does not support '-march=native'

<no location info>: error:
    `clang' failed in phase `C Compiler'. (Exit code: 1)

Up to internet, is because of Apple's M1 chip

[dfordivam]

There are a couple of minor fixes necessary.

Other than that looks good to me!

[hasufell]

Thanks @lsmor. Great work.