Improvements to scrolling #1481
Conversation
The spacing + highlighting on the inventory panel is not quite right, see the discussion at jtdaugherty/brick#484 . Also fix the size of the key shortcut hint lines to 2 rows, so the world doesn't keep resizing as we move the panel focus around when there are or are not panel-specific key shortcuts.
Still needs a mechanism to automatically scroll down to new prompt when hitting 'enter'.
- when hitting a key other than PgUp/PgDown - when a computation completes and a result is printed
byorgey
added
the
CHANGELOG
| drawREPL s = | ||
| vBox | ||
| [ withVScrollBars OnRight . viewport REPLViewport Vertical . vBox $ history <> [currentPrompt] | ||
| , vBox mayDebug |
There was a problem hiding this comment.
This vBox of empty list is a neat trick, I am surprised it does not produce an empty line. 🤔
@byorgey I noticed I can no longer click in the middle of the REPL to focus on it, only the edges work.
There was a problem hiding this comment.
I noticed I can no longer click in the middle of the REPL to focus on it, only the edges work.
Good catch. I looked into this for a while but can't figure it out. It seems like after making the REPL into a scrollable viewport, click events are no longer generated for any clicks in the interior of the panel. I'm wondering if it's a brick bug. I might just split this out into another issue and investigate it later.
src/Swarm/TUI/Model/Repl.hs
Outdated
| -- | Get only the items from the REPL history that were entered during | ||
| -- the current session. | ||
| getSessionREPLHistoryItems :: REPLHistory -> [REPLHistItem] | ||
| getSessionREPLHistoryItems h = toList $ Seq.drop (h ^. replStart) (h ^. replSeq) |
There was a problem hiding this comment.
Would it be better to use generic list rendering of history as Seq instead of plain vBox of history as list?
There was a problem hiding this comment.
@byorgey honestly I might be asking you for a solution to my 10000 messages issue, so feel free to leave it as is. 😅
There was a problem hiding this comment.
I think there's definitely some things we can do to make it more efficient, converting the entire history to a list and having brick render the whole thing is probably not a good idea. I will look into it.
There was a problem hiding this comment.
Ah, unfortunately using brick's GenericList rendering machinery will not work, since it assumes that every list item has the same height. Cutting the Seq down to a smaller size before handing it off to brick to render will definitely save time, but to make that work I guess we would have to manually tell brick how big to make the scrollbar. I don't know if that is possible.
There was a problem hiding this comment.
I think I found a good solution re: efficiency, which is to cache the REPL history so it is only re-rendered every time a new input is entered or a new output becomes available.
Indeed, clearly trackpad scrolling is a thing in some terminal applications, but sadly |
Depends on unreleased `brick` feature; see jtdaugherty/brick#484 . We should wait until it is released before merging this.
I think this is the proper way to do things: we give the entire REPL history to brick so that it can render the entire thing and thus draw the scroll bar correctly, scroll through the history with PgUp/PgDown etc., but we cache it so that it is only re-rendered when the history changes (i.e. when a new input is entered, or a new output becomes available).
byorgey
added
merge me
This fixes an issue introduced in #1481, where the old REPL history would still be visible when starting a new scenario until entering a command for the first time. Incidentally, this also fixes the issue where the previous world would be sometimes briefly visible before showing the new world upon starting a new scenario.
Closes #1461. Errors which used to be displayed in a pop-up window (parse errors, type errors) are now displayed inline in the REPL window instead. - Get rid of the pop-up error dialog - Also fix a bug introduced in #1481 where the REPL history would not be properly cleared when first starting a new scenario, because the old cache was still being used
. . .at top and bottom of info panel, since we now have scrollbar as a visual indicator when there is more contentThanks to @jtdaugherty for releasing
brick-1.10with a new ability to specify blank space to the side of scrollbars; see jtdaugherty/brick#484 .Also towards #1461 .