muesli4 / table-layout

Layout data in grids and pretty tables. Provides a lot of tools to get the cell formatting right (positional alignment, alignment on specific characters and limiting of cell width)..
BSD 3-Clause "New" or "Revised" License
37 stars 11 forks source link

Improve trimming and behaviour of some LenSpec #37

Closed Xitian9 closed 1 year ago

Xitian9 commented 2 years ago

The trim function will no longer produce garbage if given a length that is longer than the Cell to trim. Previously the only way to safely trim was with the trimOrPad function, which would also pad if the Cell was too short. This new function avoids this possibly unwanted effect.

We also provide unsafe helper versions of the pad and fill functions which take the visibleLength of the Cell as an argument, ensuring that visibleLength is never calculated more than once.

Xitian9 commented 2 years ago

Depending on how much of the previous API you want to keep, we could also merge the following pairs of functions, as they are not used internally and not exported in Table.hs:

Xitian9 commented 2 years ago

This PR now also adds a new LenSpec: expandBetween. This operates like a combination of fixedUntil and expandUntil, ensuring the column has both a minimum and maximum size, but with otherwise expand responsively. I've also added tests for the various LenSpec, and fixed some bugs these revealed with very small column widths.

Xitian9 commented 2 years ago

This PR now also ensures that extra padding is not added in columns with WideString when using the expandUntil and expandBetween LenSpec. This fixes #35, and replaces #36.

Xitian9 commented 1 year ago

I've incorporated the relevant of your comments from #34 here. I think it may be ready for merge.

Xitian9 commented 1 year ago

Your suggestion for a new type to keep track of padding helped me to realise that we can simplify Cell significantly by making it lazier and relying on this new type for composition. However, it would be backwards incompatible, so I'd like your thoughts.

One problem with the current instance is that dropLeft and friends have to return something of the same type: dropLeft :: Int -> a -> a. This is fine for things like strings, where you can easily add an extra space to top up the length, but is a problem for things like the proposed ElidableList, which needs to specifically keep track of extra padding it needs to add. What if instead Cell did no dropping whatsover until it came time to render, but merely kept track of how much it's been asked to drop.

Here's a rough prototype, I'm open to suggestions on naming.

-- | An object along with the amount that its length should be adjusted on both the left and right.
-- Positive numbers are padding and negative numbers are trimming.
data PaddedOrTrimmed a =
  PaddedOrTrimmed
  { object :: a
  , leftAdjustment :: Int
  , rightAdjustment :: Int
  } deriving (Eq, Ord, Show, Functor)

instance Applicative PaddedOrTrimmed where
  pure x = PaddedOrTrimmed x 0 0
  (PaddedOrTrimmed f l r) <*> (PaddedOrTrimmed x l' r') = PaddedOrTrimmed (f x) (l + l') (r + r')

instance Monad PaddedOrTrimmed where
  (PaddedOrTrimmed x l r) >>= f = let PaddedOrTrimmed y l' r' = f x in PaddedOrTrimmed y (l + l') (r + r')

class Cell a where
  visibleLength :: a -> Int
  measureAlignment :: (Char -> Bool) -> a -> AlignInfo
  buildCellPaddedOrTrimmed :: StringBuilder b => PaddedOrTrimmed a -> b
  buildCell :: StringBuilder b => a -> b
  buildCell = buildCellPaddedOrTrimmed . pure

dropLeft :: Int -> a -> PaddedOrTrimmed a
dropLeft n = dropBoth n 0

dropRight :: Int -> a -> PaddedOrTrimmed a
dropRight n = dropBoth 0 n

dropBoth :: Int -> Int -> a -> PaddedOrTrimmed a
dropBoth l r a = PaddedOrTrimmed a (-l) (-r)

All the work of the previous class methods drop(Left|Right|Both) would now happen in buildCellPaddedOrTrimmed, where it trims or pads at that point as needed: basically the old code in dropBoth would be in there. However, it would also know how to pad (just put spaces on either side as usual).

We no longer have direct composability of the drop* family, but we do have Kleisi composition: dropLeft m . dropLeft n becomes dropLeft m <=< dropLeft n, or dropLeft m =<< dropLeft n x if you give the argument directly. And the fact that dropping is a monoidal action comes for free, since it inherits it from the fact that (+) is a monoid.

What do you think? This would significantly simplify the typeclass, and would also significantly simplify the definition of ElidableList.

Xitian9 commented 1 year ago

We could reduce friction a bit further with some conveniences:

instance Cell a => Cell (PaddedOrTrimmed a) where
  visibleLength (PaddedOrTrimmed x l r) = visibleLength x + l + r

  -- Incorrect as written, but can be fixed with some case analysis
  measureAlignment f (PaddedOrTrimmed x l r) =
    let AlignInfo n m = measureAlignment f x in AlignInfo (l + n) ((r +) <$> m)

  buildCellPaddedOrTrimmed = buildCellPaddedOrTrimmed . join
Xitian9 commented 1 year ago

Since there are now two possible paths forward, I've dropped the last commit from this PR. It still appears in #34, but what remains is common to whatever we decide to do.

muesli4 commented 1 year ago

Your suggestion for a new type to keep track of padding helped me to realise that we can simplify Cell significantly by making it lazier and relying on this new type for composition. However, it would be backwards incompatible, so I'd like your thoughts.

I do not mind breaking existing code for several reasons:

What do you think? This would significantly simplify the typeclass, and would also significantly simplify the definition of ElidableList.

I very much like the idea. It seems it requires less from Cell instances since drop is implemented in terms of the wrapper and is possibly more efficient. Monad laws seem to hold (almost trivially).

Some questions that should be answered first:

When I initially designed Cell I imagined them as ranges or views. My suggestions for the name would be:

But there may be other names that convey its properties better. But PaddedOrTrimmed feels a little bit too bulky. ;)

muesli4 commented 1 year ago

We could reduce friction a bit further with some conveniences:

instance Cell a => Cell (PaddedOrTrimmed a) where
  visibleLength (PaddedOrTrimmed x l r) = visibleLength x + l + r

  -- Incorrect as written, but can be fixed with some case analysis
  measureAlignment f (PaddedOrTrimmed x l r) =
    let AlignInfo n m = measureAlignment f x in AlignInfo (l + n) ((r +) <$> m)

  buildCellPaddedOrTrimmed = buildCellPaddedOrTrimmed . join

I see. You already thought about this.