Skip to content

Make Cell lazier to allow more instances#44

Closed
Xitian9 wants to merge 1 commit intomuesli4:masterfrom
Xitian9:lazycell
Closed

Make Cell lazier to allow more instances#44
Xitian9 wants to merge 1 commit intomuesli4:masterfrom
Xitian9:lazycell

Conversation

@Xitian9
Copy link
Collaborator

@Xitian9 Xitian9 commented Oct 29, 2022

Previously Cell required being able to drop from the left or right without changing the type of the object. This made it awkward to define instances for which this was not natural.

Cell will no longer drop from the left or right immediately. Instead, drop(Left|Right|Both) will record the amount that has been requested to be dropped, and drop it when buildCell is called.

Changes to the API require that instance declarations for Cell be changed as follows:

instance Cell a where
dropLeft = f
dropRight = g
...

can be changed to

instance Cell a where
buildCellView = buildCellViewLRHelper buildCell f g
...

instance Cell a where
dropBoth = f
...

can be changed to

instance Cell a where
buildCellView = buildCellViewBothHelper buildCell f
...

instance Cell a where
dropLeft = f
dropRight = g
dropBoth = h
...

can be changed to

instance Cell a where
buildCellView = buildCellViewHelper buildCell buildCell buildCell f g h
...

Since dropLeft, dropRight, and dropBoth are no longer class methods of Cell, they may need to be imported explicitly. Code which relies on dropLeft, dropRight, and dropBoth not changing the type of the output may need to be rewritten, possibly by calling buildCell on the result.

@Xitian9
Copy link
Collaborator Author

Xitian9 commented Oct 29, 2022

Answering your questions from #37 here.

Some questions that should be answered first:

  • Is the context where drop is used only to feed it into buildCell?

Within this library, that is indeed the case. You can see from the fact that, aside from in the test suite, no code needed to be changed and it still passes all tests.

  • Could there be any issues with multi-width characters? Since you implemented the functionality you probably know better.

I think this actually makes multi-width characters easier, since we don't have to add the padding at each step. The proof will be in the pudding.

  • 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.

It might, but I think we are reasonably safe.

  • Would we mirror functions of Cell that work on the type?

I think they all work as is. Thoughts?

  • How do the Eq and Ord instances behave?

I was thinking that they would be the auto-derived instances: compare the base items, and only if they're equal move on to the left adjustment, and then the right adjustment. That said, this functionality is not used anywhere, so if you object we could leave it out.

  • drop is probably not very useful, as it would introduce nesting, so you would always have to join again.

I think it's not terrible. The following will basically eliminate the need to consider CellView at all:

instance Cell a => Cell (CellView a) where
  buildCellView = buildCellView . join
  buildCell = buildCellView
  ...

The two methods means that, whenever you call buildCell on an arbitrarily nested stack of CellView, it will successively join them all and build, so the stacking will be collapsed automatically.

  • Would it make sense to measure the alignment of an already modified Cell? Previously, the former was used as a step for the latter.

Yes, I agree this step would probably not be used in practice. But it can be implemented sensibly, and buildCell greatly simplifies working with this.

  • We could provide a length function for the views. Other than that I don't really see why we would need something else.

Are you proposing partially implementing the Cell typeclass for CellView, or removing some more functionality from Cell?

@Xitian9
Copy link
Collaborator Author

Xitian9 commented Oct 29, 2022

The Monad instance of CellView is isomorphic to `Writer (Sum Int, Sum Int), so you can also see that it is lawful that way.

Copy link
Owner

@muesli4 muesli4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result is much better than what I expected. The actual rendering code does not have to be changed and porting old code is easy.

I'm fine with providing an instance of Cell for CellView.

@muesli4
Copy link
Owner

muesli4 commented Oct 29, 2022

Would you mind discussing the issue about emptyCell in #46? Then we may remove the last commit again.

@Xitian9
Copy link
Collaborator Author

Xitian9 commented Oct 30, 2022

I've removed the last commit: it looks quite redundant considering #46.

Previously Cell required being able to drop from the left or right
without changing the type of the object. This made it awkward to define
instances for which this was not natural.

Cell will no longer drop from the left or right immediately. Instead,
drop(Left|Right|Both) will record the amount that has been requested to
be dropped, and drop it when buildCell is called.

Changes to the API require that instance declarations for Cell be
changed as follows:

1.
instance Cell a where
  dropLeft = f
  dropRight = g
  ...

can be changed to

instance Cell a where
  buildCellView = buildCellViewLRHelper buildCell f g
  ...

2.
instance Cell a where
  dropBoth = f
  ...

can be changed to

instance Cell a where
  buildCellView = buildCellViewBothHelper buildCell f
  ...

3.
instance Cell a where
  dropLeft = f
  dropRight = g
  dropBoth = h
  ...

can be changed to

instance Cell a where
  buildCellView = buildCellViewHelper buildCell buildCell buildCell f g h
  ...

Since dropLeft, dropRight, and dropBoth are no longer class methods of
Cell, they may need to be imported explicitly. Code which relies on
dropLeft, dropRight, and dropBoth not changing the type of the output
may need to be rewritten, possibly by calling buildCell on the result.
@muesli4
Copy link
Owner

muesli4 commented Oct 30, 2022

The pull request has been merged manually.

@muesli4 muesli4 closed this Oct 30, 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