-
Notifications
You must be signed in to change notification settings - Fork 73
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
Context Menu #949
base: master
Are you sure you want to change the base?
Context Menu #949
Conversation
Nice. Although it seems this popup is for installation? We want it for |
Ouch! I got confuse in the conversation. If the new feature is for |
I think we're going to run out of hotkeys. I suggest something like "context menu", maybe defaulting to |
I think I am missing some For reference I've tried to implement this comment ideas. |
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. |
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 -- 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 |
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. |
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):
Example context menu for GHC 8.10.7:
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. |
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
Thanks, actually I had somethings wrong. This design helped to get things clearer |
Great! I'm happy to hear that.
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 So I would prefer to go a few levels up from Generally speaking, I prefer to call some sort of @hasufell does the above sound reasonable to you? I'm not familiar with how various options are passed around by GHCup, so calling |
I'm not sure I agree. 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:
And now I'm not sure it's worth it. But feel free to give it a try. |
@hasufell thank you for your input! You make several good points.
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 :-)
As you've figured out, the parameters If the ghcup-hs/lib-opt/GHCup/OptParse/Install.hs Lines 342 to 348 in df192ee
Excepts actions with the noVerify setting set to True : ghcup-hs/lib-opt/GHCup/OptParse/Install.hs Line 340 in df192ee
For the ghcup-hs/lib-opt/GHCup/OptParse/Install.hs Line 349 in df192ee
Let me know whether this clears things up. |
d54958f
to
01427b8
Compare
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 any of you have concerns about the commit please ask. In general what I've done is to move "sections" from |
@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. |
@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. |
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. |
@lsmor awesome! Looks great. I would make some small changes — provided it's not too difficult:
|
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.
Maybe the same as in the main window, which by default is Otherwise I think this goes into the right direction. |
@lsmor if you rebase this branch against master, then CI should succeed. |
@lsmor let us know if you need any further feedback for the design. I believe this is a major improvement to the TUI. |
6a19d75
to
231341b
Compare
I am pushing (only) the visuals after rebasing. TODOS:
|
@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. |
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. |
I just recall, why this isn't a good Idea. You can either type
AS @hasufell said, adding a different key for exit, is risky as something like ¿is it ok if we keep the cancell button?
I am adding this to the check list |
395c49b
to
0aa69f6
Compare
Hi again! This is pretty much ready to test. The only problem is that I don't know how to test it. I have being able to set up a docker container with both ghcup-v0.1.22.0 and ghcup-this.branch, which set of parameter do I need to test? specially for the compile menus. For example, |
Lines 988 to 1005 in 7daf199
|
Hi, I am having a totally not fun time compiling I am compiling with the following options (below, the equivalent CLI, but in am doing it via the new menu in the TUI)
I wonder if there is something in |
It should be But I just realize we only let you define the flavor and pass configure args, not hadrian args. The idea was that "flavour transformers" should be enough: https://gitlab.haskell.org/ghc/ghc/-/blob/master/hadrian/doc/flavours.md#flavour-transformers But there appears to be none to disable docs. But you can apply a patch to disable docs in the build flavor:; https://gist.githubusercontent.com/hasufell/8de8890c6ede3ea190a1d177a6cae5ac/raw/e25cf46563bcb37e7b3cb696c25a034d9ebd064a/0001-Disable-docs-for-quickest-build-flavor.patch Then your line becomes: ghcup compile ghc -v 9.8.2 -b 9.4.8 --hadrian --flavour quickest --isolate /root/.cache/host-machine/builds/test -j 4 --patch=https://gist.githubusercontent.com/hasufell/8de8890c6ede3ea190a1d177a6cae5ac/raw/e25cf46563bcb37e7b3cb696c25a034d9ebd064a/0001-Disable-docs-for-quickest-build-flavor.patch |
Related: #846 |
Hi there, Good news!. I've been able to compile both
The combinations of different settings is large. Is there any particular setting I should test?. I have created a precarious ubuntu-based dockerfile in this branch in my fork. The dockerfile should be enough to compile all three of What else is needed to move this PR forward? |
Not much. I just need to find the time to go through it again. I'm still recovering from health issues. I definitely want to merge this before Zurihac, since I will be giving a talk in the ecosystem workshop preceding Zurihac. The new TUI design should be merged by then, so I don't give outdated information. |
Let me know if I can facilitate something: documentation, testing workflows, etc... |
I have tried this PR and made some minor fixes on this branch. There are a couple of issues I noticed
This is an initial feedback, more testing is necessary. |
@lsmor if you happen to be on Matrix, consider joining #ghcup:matrix.org |
Thanks for the feedback and for the contibutions!
Indeed. I notice the cabal file is too optimistic with respect the tui. Here is the diff you need to make it ❯ git diff ghcup.cabal
diff --git a/ghcup.cabal b/ghcup.cabal
index 0a9545f..0ed5275 100644
--- a/ghcup.cabal
+++ b/ghcup.cabal
@@ -402,19 +402,13 @@ executable ghcup
build-depends:
, ghcup
, ghcup-optparse
- , ghcup-tui
if flag(internal-downloader)
cpp-options: -DINTERNAL_DOWNLOADER
if flag(tui)
cpp-options: -DBRICK
- other-modules: BrickMain
- build-depends:
- , brick ^>=2.1
- , transformers ^>=0.5
- , vty ^>=6.0 || ^>=6.1 || ^>=6.2
- , optics ^>=0.4
+ build-depends: ghcup-tui
if os(windows)
cpp-options: -DIS_WINDOWS
Which configuration doesn't work for you?
I am not in matrix nor any messaging platform. But I could join if there is a discussion about this PR there. |
Up to you. I'm just trying to gather contributors and establish a less formal place to get pinged, do discussions and ask questions. E.g. I don't know if you will be at Zurihac. |
@lsmor I did more testing and had the following feedback, plus responses to your comments.
Both of the APIs return If this API returns a
Whereas if a
The most likely use cases for the tui based advanced installations would be "simple" scenarios like compiling HLS (when a new minor release of GHC is released), and the power users would prefer the CLI. If the user is invoking the advanced options menu for compile/install of only a single version of a tool, then keeping the state is certainly a better choice, since the user might want to tweak the values in case the installation or compilation did not happen successfully. (Note: User should be able to run the compilation of HLS for multiple GHC versions in a single step, as I mentioned above in this comment) When the user want to use the advanced options submenu multiple times in a Therefore let's consider both of these design choices for a couple of scenarios I think would be common.
I think other than these two scenarios there aren't many use cases for using the advanced options menu. Anything more complicated might be better done via CLI. Overall I think maintaining the state is better. |
There is something intriguing about the TUI, which is actually much harder to do via the CLI: #846 There are 3 classes of "pass through flags".
At the moment, the |
At any rate... I believe the TUI can very well be improved gradually. Unlike the CLI, it doesn't really impose a static interface, because it cannot be used in scripts. We can "break" TUI just fine and provide alternative ways to e.g. navigate it. It is interactive, after all. So I don't mind a bit of experimentation wrt the interface. |
Oh!! sure, I'll do it
I did not commit those...
I remmember to implement this, but there was a problem with it... not very sure now, I'll take a look a come back with feedback later. I will merge your commits and push them (read below). By the way, thanks a lot for the help and testing!!! probably keeping the state is better than not keeping it.
Absolutely, I was thinking about adding a section to Now a git question... how do I proceed? can @dfordivam merge to this PR directly? o should I add locally his fork to my remotes, then pull, then push to my fork? (the last is moreless what I do at job, but the usage of git is much more amateur) To recap:
|
To have frequently used fields shown first.
INTERNAL_DOWNLOADER, and BRICK are not used in ghcup-tui
I was only referring the TUI interface. The TUI could provide a different set of options than CLI.
This is how it's usually accomplished, but I opened a PR on your fork to make it easy for you to merge the branch via GitHub. I have also pushed a few more commits on my branch, including the cabal file fixes. |
Improvements to the PR
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:
If we translate that into code