Skip to content

Improve trimming and behaviour of some LenSpec#37

Merged
muesli4 merged 3 commits intomuesli4:masterfrom
Xitian9:trim
Oct 29, 2022
Merged

Improve trimming and behaviour of some LenSpec#37
muesli4 merged 3 commits intomuesli4:masterfrom
Xitian9:trim

Conversation

@Xitian9
Copy link
Collaborator

@Xitian9 Xitian9 commented Apr 8, 2022

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
Copy link
Collaborator Author

Xitian9 commented Apr 8, 2022

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:

  • remSpacesB <- remSpacesB'
  • fillLeft <- fillLeft'
  • fillRight <- fillRight'
  • fillCenter <- fillCenter'

@Xitian9 Xitian9 changed the title Add safe version of trim function Add safe version of trim function and expandBetween LenSpec Apr 13, 2022
@Xitian9
Copy link
Collaborator Author

Xitian9 commented Apr 13, 2022

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 Xitian9 changed the title Add safe version of trim function and expandBetween LenSpec Improve trimming and behaviour of some LenSpec Apr 13, 2022
@Xitian9
Copy link
Collaborator Author

Xitian9 commented Apr 13, 2022

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.

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.
This acts as a combination of fixedUntil and expandUntil: it will be
of fixed length until the first argument, after which it will expand to
match the size of the column up until the second argument.

Also add test coverage for grid and the various column specs.
This ensures that columns really are the advertised width, even with
very small column sizes.
@Xitian9
Copy link
Collaborator Author

Xitian9 commented Oct 26, 2022

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

@Xitian9 Xitian9 force-pushed the trim branch 2 times, most recently from 93125a1 to 45cd592 Compare October 27, 2022 00:11
@Xitian9
Copy link
Collaborator Author

Xitian9 commented Oct 27, 2022

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
Copy link
Collaborator Author

Xitian9 commented Oct 27, 2022

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
Copy link
Collaborator Author

Xitian9 commented Oct 29, 2022

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
Copy link
Owner

muesli4 commented Oct 29, 2022

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:

  • The library is still in the early stages.
  • Custom instances of Cell are probably rare.
  • Adapting to new changes is fairly straightforward.

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:

  • Is the context where drop is used only to feed it into buildCell?
  • Could there be any issues with multi-width characters? Since you implemented the functionality you probably know better.
  • Could this possibly break other use cases of Cell instances? I guess one could simply always use the wrapper as a drop-in replacement of Cell.
  • Would we mirror functions of Cell that work on the type?
  • How do the Eq and Ord instances behave?

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

  • CellView (e.g., see std::string and std::string_view in C++)
  • CellRange

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

@muesli4
Copy link
Owner

muesli4 commented Oct 29, 2022

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.

  • drop is probably not very useful, as it would introduce nesting, so you would always have to join again.
  • Would it make sense to measure the alignment of an already modified Cell? Previously, the former was used as a step for the latter.
  • We could provide a length function for the views. Other than that I don't really see why we would need something else.

@muesli4 muesli4 merged commit a196cfb into muesli4:master Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants