Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make cat' and friends accumulate seperation between empty diagrams #130

Closed
wants to merge 3 commits into from

Conversation

bergey
Copy link
Member

@bergey bergey commented Oct 17, 2013

closes #37

@byorgey, is this how you want to handle #37?

The test code:

import Diagrams.Prelude
import Diagrams.Backend.SVG

a = let d = hcat' with { sep = 0.2 } [circle 1, mempty, mempty, mempty, circle 1]
        in
     d <> square 1

b = let d = mempty ||| circle 1 in
    d <> square 1

c = let d = hcat' with { sep = 0.2 } [circle 1, circle 1]
        in
     d <> square 1


main :: IO ()
main = renderSVG "bug37.svg" (Dims 400 400) $ (a === b === c) # pad 1.02

produces the following output, as expected:

bug37

@@ -323,7 +325,7 @@ instance Num (Scalar v) => Default (CatOpts v) where
--
-- See also 'cat'', which takes an extra options record allowing
-- certain aspects of the operation to be tweaked.
cat :: ( Juxtaposable a, Monoid' a, HasOrigin a
cat :: ( Enveloped a, Juxtaposable a, Monoid' a, HasOrigin a
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, adding an Enveloped constraint back is super annoying. This was the original reason I split out a separate Juxtaposable class, because some things (e.g. animations) are Juxtaposable but not Enveloped. I think I now remember running into this the last time I thought about #37. Am I correct that all you need is to be able to tell whether the things being concatenated are empty or not? That is a much weaker requirement than having an envelope. Maybe we should make a separate class for that (and add it as a superclass of Juxtaposable, probably).

@bergey
Copy link
Member Author

bergey commented Oct 31, 2013

I took another pass at what this might look like without the Enveloped constraint. I think it will still do the right thing for Enveloped types, but I'm still not sure I understand the full generality of Juxtaposable.

For example, I'd expect a class law like

isEmpty a IFF a == mempty

(Which is still useful for types that are Monoid but not Eq.) But that isn't fulfilled by the current instances, which used Enveloped. At the moment, we still have some non-empty Diagrams with empty Envelopes, at least in 3D. Should all non-empty Diagrams have point envelopes instead, as for text?

I don't think that there is an instance HasEmpty a => HasEmpty (b -> a). At least, not the way I defined HasEmpty. That's one reason I haven't made HasEmpty a superclass of Juxtaposable. (The other being that HasEmpty seems more closely related to Monoid.)

@byorgey
Copy link
Member

byorgey commented Nov 2, 2013

Yes, I would expect that law for HasEmpty too. Which instances are you referring to, which do not satisfy that? (Other than HasEmpty b => HasEmpty [b]... still pondering that one.)

I am not worried about non-empty Diagrams with empty Envelopes. I am not even sure if we should have a HasEmpty Diagram instance; even if we did, it should certainly not just tell you whether the diagram's envelope is empty.

I think you are right not to make HasEmpty a superclass of Juxtaposable.

@bergey
Copy link
Member Author

bergey commented Nov 9, 2013

I don't understand the design here enough to finish this. Since there are plenty of other areas I can keep working on, I'd rather leave this one to you. Sorry for the false starts.

@bergey bergey closed this Nov 9, 2013
@byorgey
Copy link
Member

byorgey commented Nov 9, 2013

No worries, thanks for looking at it. False starts are still better than nothing, and we've certainly learned some things from them! I'll try to take another careful shot at this at some point, though maybe not until after the 1.0 release.

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.

More problems with beside + mempty
2 participants