Skip to content
Snippets Groups Projects

Tui improvements

Merged Julian Ospald requested to merge TUI-improvements into master
3 unresolved threads

Peek_2020-10-09_23-16

Wrt #69 (closed)

Inserting separators isn't possible with how brick processes lists? I think that would require a lot of work to feel natural with navigation. I'm considering to insert an empty row though, but that also needs some work.

Edited by Julian Ospald

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • mentioned in issue #69 (closed)

  • @jtdaugherty do you have an idea on how to insert separators between different tools? Afais this isn't easily possible with how renderList works.

  • Julian Ospald changed the description

    changed the description

  • If you want separators, then a List is no longer the way to go, unfortunately. I'm not sure how much it matters to you to maintain a single list for everything, but I find it awkward, personally, and wouldn't recommend trying to put them all in one list.

    • I believe @glguy also had some ideas about how this could be done a bit differently that wouldn't depend on a monolithic list approach; perhaps he can comment.

    • The way we'd discussed in another forum was that we could have a two-level approach; first you pick a tool and then you get all that tool's versions.

      The first list might look like this sketch

      Tool      Recommended      Latest
      ---------------------------------
      ghc        8.8.4           8.10.2
      hls        0.5.0           0.5.0
      cabal      3.2.0.0         3.2.0.0
      ghcup      0.1.11          0.1.11

      We could decorate that with pretty green checkmarks next to things to indicate which were installed. The leftmost check could mean it was installed at all, and then if a recommended or latest version was installed we could indicate that too.

      Tool      Recommended      Latest
      ---------------------------------
      ✓ ghc        8.8.4         ✓ 8.10.2
        hls        0.5.0           0.5.0
      ✓ cabal    ✓ 3.2.0.0       ✓ 3.2.0.0
      ✓ ghcup      0.1.11          0.1.11

      This view makes it clear which tools are available in ghcup and makes it clear which tools are "outdated" with a glance.

    • Please register or sign in to reply
  • I basically reimplemented GenericList a little more loose and butchered drawListElements from Brick internals. The result is:

    Screenshot_2020-10-11_19-20-42

  • If that screenshot is showing a List where the elements do not all have the same height, then that will result in broken scrolling computations. But I don't know how you implemented it.

  • Julian Ospald added 4 commits

    added 4 commits

    Compare with previous version

  • @jtdaugherty !39 (7afd262b)

    I believe everything is working.

    Edited by Julian Ospald
164 164 then emptyWidget
165 165 else foldr1 (\x y -> x <+> str "," <+> y) $ notes
166 166 )
167 <+> (vLimit 1 $ fill ' ')
  • Where is the ListResult type declaration?

  • data ListResult = ListResult
      { lTool      :: Tool
      , lVer       :: Version
      , lCross     :: Maybe Text -- ^ currently only for GHC
      , lTag       :: [Tag]
      , lInstalled :: Bool
      , lSet       :: Bool -- ^ currently active version
      , fromSrc    :: Bool -- ^ compiled from source
      , lStray     :: Bool -- ^ not in download info
      , lNoBindist :: Bool -- ^ whether the version is available for this platform/arch
      , hlsPowered :: Bool
      }
  • Jonathan Daugherty
    Jonathan Daugherty @jtdaugherty started a thread on an outdated change in commit 7afd262b
  • 212
    213 listItemHeight = 1
    214 listSelected = fmap fst $ listSelectedElement' is
    215
    216 -- The number of items to show is the available height
    217 -- divided by the item height...
    218 initialNumPerHeight = (c ^. availHeightL - seps) `div` listItemHeight
    219 -- ... but if the available height leaves a remainder of
    220 -- an item height then we need to ensure that we render an
    221 -- extra item to show a partial item at the top or bottom to
    222 -- give the expected result when an item is more than one
    223 -- row high. (Example: 5 rows available with item height
    224 -- of 3 yields two items: one fully rendered, the other
    225 -- rendered with only its top 2 or bottom 2 rows visible,
    226 -- depending on how the viewport state changes.)
    227 numPerHeight =
    • This logic about numPerHeight and listItemHeight needs to go away if you need to have list items with different heights. The reason this numPerHeight stuff is in here is as an optimization to avoid drawing too many items that won't be visible anyway, but you will have to abandon that optimization if your items have different heights. This also means the viewport translation using off needs to be removed. The approach you're taking where you add visible and embed the results in a viewport will work okay without this, but just note that it won't scale if you have hundreds of list elements because at some point there is a drawing cost for those that will introduce latency in your rendering loop.

    • changed this line in version 3 of the diff

    • Please register or sign in to reply
  • This logic about numPerHeight and listItemHeight needs to go away if you need to have list items with different heights.

    But they are all the same height, no? I accounted for the additional separators by subtracting them from the available height.

  • If some of the items contain separators, they are not the all the same height. Some are 1 row high, others are 2 high. The difference matters because the number of items to render relies on the connection between the item height and the viewport height. If they aren't the same height, that calculation will get that wrong sometimes, leading to strange scrolling behavior. You might never notice for most UI sizes, but for some viewport heights (i.e. window heights) you may run into problems once there are enough items with and without separators to trigger errors in that logic.

  • If some of the items contain separators

    I have a hard time to follow, what kind of separators? :sweat_smile:

    So I see two ways here:

    1. make sure all items are always one row high (I believe that's currently the case, no?)
    2. fix the logic to account for different heights
  • Yes, the List in the library follows approach (1). The logic cannot be fixed to account for items with different heights unless the optimization to render fewer items is removed (i.e. the logic related to numPerHeight). Introducing hBorders for some items (i.e. making them have different heights) will ultimately break that optimization and lead to broken scrolling behavior, depending on the viewport height.

  • So you are saying this

                  addSeparator w = case es !? (i' - 1) of
                    Just e' | lTool e' /= lTool e ->
                      hBorder <=> w
                    _                             -> w

    breaks the logic and I have to make the separator an actual item for the input list instead of composing it with a row? Something like [Either (Widget String) ListResult]?

  • You could make the separators list items to avoid the issue I mentioned, but I wouldn't recommend it since that will be awkward. You'd end up with separators that could be selected, and that probably isn't what you want. I'm just saying that if you want to put these separators in while also re-using the List implementation, you'll need to remove the code that attempts to render less than the entire list in order to avoid the scrolling issues that will come up. All this amounts to in the end is that you'll have

          render
            $ viewport "GHCup" Vertical
            $ vBox
            $ V.toList drawnElements

    instead of

          render
            $ viewport "GHCup" Vertical
            $ translateBy (Location (0, off))
            $ vBox
            $ V.toList drawnElements

    and then the binding to es will be

    es = clr

    instead of

    es = slice start (numPerHeight * 2) clr
  • As an aside, I also recommend making a resource name type, e.g,

    data Name =
        ProgramList | ...
        deriving (Eq, Show)

    and then using that for your n type (in Widget n). That would mean you'd have viewport ProgramList ... instead of viewport "GHCup" ....

  • You'd end up with separators that could be selected

    I have control over the move-up/move-down logic and can move the index further, when it hits a Left.

  • Yes, that would also work.

  • I think there's no point though... we won't have hundreds of GHC versions anyway. So I'll just ditch the logic.

  • Julian Ospald added 3 commits

    added 3 commits

    Compare with previous version

  • Julian Ospald added 1 commit

    added 1 commit

    • 778ee142 - Upload golden files as artifacts on failure

    Compare with previous version

  • Julian Ospald added 1 commit

    added 1 commit

    Compare with previous version

  • merged

  • Please register or sign in to reply
    Loading