[Draft] Introductory mode #850
Conversation
|
This is pretty cool!
The TUI code is not very large, so I'm open to a complete rewrite. But the visual design should mainly stay the same, with the finicky separators... those required me to copy-paste internals and adjust them, see:
The other thing I want to get rid of is this: #65 Wrt the actually proposed change... I think we want to avoid popups by default. I could imagine that after ghcup installation, we print a message to start in introductory mode via |
Absolutely. I am not planning to change it.
Well. I can give it a try, but I suspect this is going to be indeed difficult. Any case since the refactoring should include
I think many people would like to have the |
I don't have any evidence for this. It's the first time someone complains about it.
I'm fine with new specific settings, because the average user doesn't need to know about it. However, I'm not fond of popup questions by default. These should only exist in "introductory mode" or something else. Another way is to just introduce another key that denotes "install and set". |
|
Any news? |
|
@hasufell . I've been getting used to the code base a little bit more.
This is doable (but will take time). If we want a decent/maintanable code a full rewrite of the TUI part should be done. The parts we can save from the current code are those which handle the As for the ETA. With very few hours a week, I could have it in october or so. Do you want to continue in this draft or to open a new issue? |
I'm not very familiar with brick. I don't do TUI often. The code was reviewed once in its early stages by the brick author, but that was it.
Yeah maybe.
Yeah, sure. But it's not a multi-threaded environment or anything.
Sure, just keep it here. |
|
A quick update before vacation. I did some deeper research on the code and
|
ghcup.cabal
Outdated
| @@ -352,6 +352,9 @@ executable ghcup | |||
| , transformers ^>=0.5 | |||
| , unix ^>=2.7 | |||
| , vty ^>=5.37 | |||
| , microlens ^>=0.4.13 | |||
There was a problem hiding this comment.
for this particular commit it isn't necessary but notice that brick builds on top of microlens; hence if we want to reuse brick widgets (like Brick.Widget.List or the new widget for logs) we need microlens. It is a transitive dependency already, so I think it doesn't incur in bigger build times.
If this is a stopper for you I could try to not use microlens but keep in mind brick is kind of opinionated about this
|
Hi, I am comming back to this as you notice. I wasn't willing to push to this branch, I am working in a separate branch on my fork. Nevertheless, In order to review this, Do you prefer a rebase from master and then squash all changes? or do you prefer one commit for task? Btw: I've read #848 . Thanks for your work, I'd like to help but my knowledge in distribution channels is very limited 😅 . That being said I am happy to document/answer question about the TUI part once I implement it |
|
I don't believe in squashing. Organize the commits however you want. I'll have capacity to review it. If you have links to the brick documentation about changes done, that might help. |
|
I'll be able to drop dependencies on |
ghcup.cabal
Outdated
| @@ -352,6 +352,9 @@ executable ghcup | |||
| , transformers ^>=0.5 | |||
| , unix ^>=2.7 | |||
| , vty ^>=5.37 | |||
| , microlens ^>=0.4.13 | |||
| , microlens-th ^>=0.4.3 | |||
| , microlens-mtl ^>=0.2.0 | |||
There was a problem hiding this comment.
These deps also need to be added to the ghcup-optparse library component.
There was a problem hiding this comment.
I've dropped dependency in mircrolens in favor of optics. So It is consistent with the rest of the code base. I am still working on this but in somewhat slow path. But I'll finish it
|
I have rebase the branch and finalice the important part of the work (that is the tutorial). I also, change the main data structure for a map-like one which integrates better with
Some changes with respect the scrolling have changed. Also, the tutorial display is a draft. Tomorrow, I'll make comments in the changes, so it is easier to review |
There was a problem hiding this comment.
These commits have some behaviour changes:
- The viewport isn't as smooth as before. It keeps the whole tool section on screen. Not the end of the world
- Tab and Shift + Tab can be use to move from one tool to another. This can be changed with two lines of code, but I find it useful
- pressing x open the tutorial screen.
- pressing enter when the tutorial screen is open, closes it
- when in tutorial, no other key works other than Enter.
ghcup.cabal
Outdated
| @@ -352,6 +352,9 @@ executable ghcup | |||
| , transformers ^>=0.5 | |||
| , unix ^>=2.7 | |||
| , vty ^>=5.37 | |||
| , microlens ^>=0.4.13 | |||
There was a problem hiding this comment.
for this particular commit it isn't necessary but notice that brick builds on top of microlens; hence if we want to reuse brick widgets (like Brick.Widget.List or the new widget for logs) we need microlens. It is a transitive dependency already, so I think it doesn't incur in bigger build times.
If this is a stopper for you I could try to not use microlens but keep in mind brick is kind of opinionated about this
ghcup.cabal
Outdated
| @@ -352,6 +352,9 @@ executable ghcup | |||
| , transformers ^>=0.5 | |||
| , unix ^>=2.7 | |||
| , vty ^>=5.37 | |||
| , microlens ^>=0.4.13 | |||
| , microlens-th ^>=0.4.3 | |||
| , microlens-mtl ^>=0.2.0 | |||
There was a problem hiding this comment.
I've dropped dependency in mircrolens in favor of optics. So It is consistent with the rest of the code base. I am still working on this but in somewhat slow path. But I'll finish it
| $ viewport "GHCup" Vertical | ||
| $ vBox | ||
| $ V.toList drawnElements | ||
|
|
There was a problem hiding this comment.
We can remove this function completely as renderSectionList do its job
| , Brick.txt "Press Enter to exit the tutorial" | ||
| ] | ||
| in [tutorial, ui dimAttrs st] | ||
|
|
There was a problem hiding this comment.
drawUI now splits, depending on which mode we are. This is the reason Mode type is so usefull. We can define handlers, drawings, etc... for each mode with barely extra complexity
| ] | ||
| where | ||
| withBackColor | no_color = \attr _ -> attr `Vty.withStyle` Vty.reverseVideo | ||
| | otherwise = Vty.withBackColor | ||
|
|
||
| eventHandler :: BrickState -> BrickEvent String e -> EventM String BrickState () | ||
| eventHandler st@BrickState{..} ev = do | ||
| tutorialHandler :: BrickEvent Name e -> EventM Name BrickState () |
There was a problem hiding this comment.
We define a handler for each mode
| m <- use mode | ||
| case m of | ||
| Navigation -> navigationHandler ev | ||
| Tutorial -> tutorialHandler ev | ||
|
|
There was a problem hiding this comment.
eventHandler dispaches the event depending on the Mode
| putStrLn "Press enter to continue" | ||
| _ <- getLine | ||
| pure (updateList data' as) | ||
| Left err -> throwIO $ userError err |
There was a problem hiding this comment.
I've tried to make this part of the TUI, instead of SuspendAndResume. You can find a decent attempt here. It isn't difficult, as it is only to extent the Mode type and to add a custom event channel, but there are some difficulties
There was a problem hiding this comment.
Yes, the logging and receiving the process output is the main issue. It's tricky with the current code.
There was a problem hiding this comment.
well the logging isn't a problem. It is relativaly easy to deviate the logging part to a Brick channel so It it can be used as a widget. The main problem is the output of external processes, for example the call to curl prints directly on top of the TUI... I don't think this feature will be implemented in this PR
There was a problem hiding this comment.
The main problem is the output of external processes, for example the call to curl prints directly on top of the TUI... I don't think this feature will be implemented in this PR
That is what I meant with "logging". On the linux side we use low-level handling of stdout/stderr file descriptors... they could be exposed and used as callback style thing:
ghcup-hs/lib/GHCup/Prelude/Process/Posix.hs
Lines 110 to 115 in 9d8d6e3
On windows side we use the process package fake pipes:
ghcup-hs/lib/GHCup/Prelude/Process/Windows.hs
Lines 161 to 184 in 9d8d6e3
So a solution would probably have to write a wrapper around both that exposes the necessary parts and a callback that listens to messages on the pipe/Fd and reacts to it.
|
I tried to build and run this and moving through the list of elements in the list has some odd behavior. I'm not sure how to explain. E.g.:
I also noticed the horizontal separator is missing under the "Tool Version ..." header |
|
I can reproduce with a big enough font (or an small enough terminal). This is happening because the scroll follows the whole section instead of the element in the section. I should change that.
Well, this is happening for the same thing. Essentially you have 9 items out of view...
upps.. already fixed in my local machine. Do you have any preference about "tutorial" part? the current text is rather poor (in essence a Mock), but any change is easy to implement. |
|
What's the status of this PR? |
This PR implements the tutorial / introductory mode. A major effort has gone into refactoring the code so It follows )a more I think the main purpose of the PR is done (maybe the Tutorial widget needs better text). I am willing to merge it soon, but given the current windows (#912) work, maybe you prefer to prioritize that PR over this. |
lsmor
changed the title
|
The scrolling behavior is still quite different, as can be seen here: This PR
Old behavior
I personally prefer the old behavior. It's very simple and also matches how most GUIs behave (e.g. file manager where you press up button on the list of files). I asked one other user and they also prefer the old behavior. |
|
Another possible topic for introductory mode: haskell/ghcup-metadata#143 |
No rush!, I'll continue with the three unchecked items. Take as much time as you need |
How about we use That way we can also design a proper help page. And I think it's natural to find the tutorial under a help page? |
I have added a new info layer with a verbose description of the keybindings. I don't know what With respect to Keybindings.
Remmember this is a draft and I am not a native English speaker, so I might use too much the passive voice 😆 ScreenShots for each new widgets: Key Info
Tutorial
|
|
I guess this is mostly ready to merge then. If you mark the PR as ready, I'll try to go over the code some time. |
|
Notice that there are two items with missing documentation and I don't now exactly what to put there
Best |
We shouldn't go into too much detail here but just say this is the most reasonable choice for people starting out with haskell or companies that are not keen to experiment.
That key is removed in master already (unfortunately, you'll have to rebase due to that... if it's too much trouble, I can revert it temporarily). |
Don't worry, I'll rebase it and include the missing recommended doc |
…g'. Lensify the app
…e tutorial aesthetics and text
|
I don't really understand all of the code yet, but I'm just going to merge and maybe comment later or provide follow-up PRs. |
|
I noticed the TUI still doesn't behave like it used to wrt the viewport. Old:
New:
As you can see. Once you're at an offset higher than the render limit of the terminal... your "cursor" will stay at the bottom of the screen. That's not the behavior of e.g. the old GHCup or other terminal TUIs like tig. I tried to fix it in the code, but don't see an evident way how to do it. |
|
We basically need a way to store the current applied viewport translation and then calculate whether the new offset would already fall into that visible viewport when applying the previous translation. I forgot how the previous implementation did it. |
|
I also don't see a way to query Brick "which list elements are currently visible?". If that was possible, we can infer the current translation that is applied. Edit: well, that won't work, because we have multiple lists. |
|
The old implementation simply relied on visible. I tried the same with the current code (we have to apply visible to both the section list and the selected list element): @@ -212,31 +215,27 @@ renderSectionList render_elem section_focus (GenericSectionList focus elms sl_na
c <- Brick.getContext
let -- A section is focused if the whole thing is focused, and the inner list has focus
section_is_focused l = section_focus && (Just (L.listName l) == F.focusGetCurrent focus)
- -- We need to limit the widget size when the length of the list is higher than the size of the terminal
- limit = min (Brick.windowHeight c) (Brick.availHeight c)
- s_idx = fromMaybe 0 $ V.findIndex section_is_focused elms
- render_inner_list has_focus l = Brick.vLimit (length l) $ L.renderList (\b -> render_elem (b && has_focus)) has_focus l
- (widget, off) =
- V.ifoldl' (\wacc i list ->
+ render_inner_list has_focus l = Brick.vLimit (length l) $ L.renderList (\b -> let v = b && has_focus in makeVisible' v . render_elem $
+ makeVisible' b = if b then Brick.visible else id
+ widget =
+ V.ifoldl' (\(!acc_widget) i list ->
let has_focus_list = section_is_focused list
- (!acc_widget, !acc_off) = wacc
- new_widget = if i == 0 then render_inner_list has_focus_list list else hBorder <=> render_inner_list has_focu$
- new_off
- | i < s_idx = 1 + L.listItemHeight list * length list
- | i == s_idx = 1 + L.listItemHeight list * fromMaybe 0 (L.listSelected list)
- | otherwise = 0
- in (acc_widget <=> new_widget, acc_off + new_off)
+ new_widget = if i == 0
+ then makeVisible' has_focus_list $ render_inner_list has_focus_list list
+ else hBorder <=> (makeVisible' has_focus_list $ render_inner_list has_focus_list list)
+ in acc_widget <=> new_widget
)
- (Brick.emptyWidget, 0)
+ Brick.emptyWidget
elms
- Brick.render $ Brick.viewport sl_name Brick.Vertical $ Brick.translateBy (Brick.Location (0, min 0 (limit-off))) widget
+ Brick.render $ Brick.viewport sl_name Brick.Vertical widgetThis somewhat restores the old behavior, but that has two issues:
|
|
Let me check this. I remmember, reliying on visible did not work, you have no manually calculate the viewport tranlation |
|
We probably need https://hackage.haskell.org/package/brick-2.3.1/docs/Brick-Widgets-Core.html#v:visibleRegion Just I think computing the relative location of the selected list item in the section should be possible. That may fix both issues. |
| limit = min (Brick.windowHeight c) (Brick.availHeight c) | ||
| s_idx = fromMaybe 0 $ V.findIndex section_is_focused elms | ||
| render_inner_list has_focus l = Brick.vLimit (length l) $ L.renderList (\b -> render_elem (b && has_focus)) has_focus l | ||
| (widget, off) = |
There was a problem hiding this comment.
Here is where the viewport translation is calculated. s_idx stands for section index, hence the amount of offset is offset = (each previous section * its length) + (the current section * the current index in such a list) + (0 * next sections)
This is calculated with and indexed left fold. The variable new_off implements the logic above (notice we add 1 in each iteration to count for the hborder.)
Current master is not building in my computer due to missing dependencies (bzlib + file-uri). So I am having troubles testing this. Nevetheless, the idea should be in line 232 substitute
-- AS IS
Brick.translateBy (Brick.Location (0, min 0 (limit-off)))
-- TO BE
-- |- Probably something slightly different
Brick.visibleRegion (Brick.Location (0, min 0 (limit-off))) (region_of_the_current_item)
-- This re-uses Brick.Widget.List.renderList
renderSectionList :: (Traversable t, Ord n, Show n, Eq n, L.Splittable t, Semigroup (t e))
=> (Bool -> e -> Widget n) -- ^ Rendering function of the list element, True for the selected element
-> Bool -- ^ Whether the section list has focus
-> GenericSectionList n t e -- ^ The section list to render
-> Widget n
renderSectionList render_elem section_focus ge@(GenericSectionList focus elms sl_name) =
Brick.Widget Brick.Greedy Brick.Greedy $ do
c <- Brick.getContext
let -- A section is focused if the whole thing is focused, and the inner list has focus
section_is_focused l = section_focus && (Just (L.listName l) == F.focusGetCurrent focus)
render_inner_list has_focus l = Brick.vLimit (length l) $ L.renderList (\b -> let v = b && has_focus in render_elem v) has_focus l
widget =
V.ifoldl' (\(!acc_widget) i list ->
let has_focus_list = section_is_focused list
(c', r') = case sectionListSelectedElement ge of
Nothing -> (2, 0)
Just (selElIx, _) -> (2, selElIx)
new_widget = if i == 0
then makeVisible' has_focus_list (c', r') $ render_inner_list has_focus_list list
else hBorder <=> (makeVisible' has_focus_list (c', r') $ render_inner_list has_focus_list list)
in acc_widget <=> new_widget
)
Brick.emptyWidget
elms
Brick.render $ Brick.viewport sl_name Brick.Vertical widget
where
makeVisible' b (c, r) p
| b = Brick.Widget (hSize p) (vSize p) $ do
result <- render p
let imageSize = ( result ^. lensVL Brick.imageL % to Vty.imageWidth
, result ^. lensVL Brick.imageL % to Vty.imageHeight - r
)
return $ if imageSize^._1 > 0 && imageSize^._2 > 0
then result & (lensVL Brick.visibilityRequestsL) %~ (Brick.VR (Brick.Location (c, r)) imageSize :)
else result
| otherwise = pThis comes very close. But there's still some rough edges. |
|
Ah... got it.... the above is 90% there, just had to change to So it always focuses on a tiny dimension. That restores previous behavior. |
This is a proof of concept for #847 . It creates a PopUp on instalation, asking if you want to set the installed tool as the default.
This isn't an introductory mode, but it reflectes how challenging would it be. This PR contains very very ugly code, which would become unmaintanable very quickly. More over creating a complicated introductory mode would require a huge overhaul of the actual code.
microlensare unavoidable is we want decent codedata Mode = ShowingDialog | Normal | Introduction | ...appDrawto depend on theMode. Adding different drawing functions for eachModeevenHandlerthe same way (one handler for each mode, and a function to dispatch).I am willing to do it but I don't know If It worth a huge rewrite.
(Notice, this poc is because you commented "That requires some design and probably a lot more TUI code" in the discord thread... And that stament is very true, I just wanted to give some perspective)