racket / pict

Other
15 stars 21 forks source link

tree-edge: won't take #f as a tree-layout #49

Open RenaissanceBug opened 5 years ago

RenaissanceBug commented 5 years ago

In the pict/tree-layout docs, the tree-edge function is specified as taking a tree-layout?, which in turn is defined to be #f or the output of the tree-layout function. However, the tree-edge contract given in pict/tree-layout.rkt is not sufficiently permissive to allow #f as an input:

> (tree-edge #f)
  tree-edge: contract violation
  expected: tree-layout?
  given: #f
  in: the 1st argument of
      (->* (tree-layout?)
           (#:edge-color (or/c string?
                               (is-a?/c color%)
                               (list/c byte? byte? byte?))
            #:edge-width (or/c 'unspecified real? #f))
           tree-edge?)
  contract from:
      <pkgs>/pict-lib/pict/tree-layout.rkt
  blaming: anonymous-module
   (assuming the contract is correct)
  at: <pkgs>/pict-lib/pict/tree-layout.rkt:20.10

The practical consequence is that this appears to prevent the creation of a binary-tree-layout with one or more individually customized edges.

The immediate cause is that the tree-layout? given as first input in the contract is the struct type predicate from private/layout.rkt, and not the more permissive _tree-layout? function available to client code. Making the input be _tree-layout? does fix the immediate problem and allow tree-edge to take #f; however, naive-layered and binary-tidier fail to render trees created that way:

> (define (complete d)
    (cond
      [(zero? d) #f]
      [else (define s (complete (- d 1)))
            (tree-layout (tree-edge #:edge-color "red" s) s)]))
> (naive-layered (complete 4))
. . ../../Applications/Racket v7.1/collects/racket/private/kw.rkt:1281:57: pin-line: contract violation
  expected: pict-path?
  given: #f
  in: the 4th argument of
      (->* (pict-convertible?
             pict-path?
             (-> pict? pict-path? (values number? number?))
             pict-path?
             (-> pict? pict-path? (values number? number?)))
           ((or/c #f number?)
            (or/c #f string?)
            boolean?
            #:style (or/c #f symbol?))
           pict?)
  contract from:
      <pkgs>/pict-lib/pict/private/utils.rkt
  blaming: <pkgs>/pict-lib/pict/private/main.rkt
   (assuming the contract is correct)
  at: <pkgs>/pict-lib/pict/private/utils.rkt:78.4
> (binary-tidier (complete 4))
. . Repos/pict/pict-lib/pict/private/tidier.rkt:88:9: match: no matching clause for #f

The hv-alternating function does correctly render the resulting trees.

rfindler commented 5 years ago

If I am understanding things correctly, tree-edge makes sense only when its argument is not #f, so the fix we should have is to document it as something like (and/c tree-layout? (not/c #f)), plus adjusting the contract so the right error message prints.

The reason I say that is that there is no edge to a node that isn't drawn, and that's what #f is supposed to be. So probably you want this function:

#lang racket
(require pict/tree-layout)
(define (complete d)
  (cond
    [(zero? d) #f]
    [else (define s (complete (- d 1)))
          (tree-layout (and s (tree-edge #:edge-color "red" s)) s)]))
(naive-layered (complete 4))
(binary-tidier (complete 4))
RenaissanceBug commented 5 years ago

Ah! OK, that makes sense.

At the time I wrote that, I was forgetting that the #f nodes weren't drawn, but remembering the line "The default node-pict (used when it is #f) is..." from tree-layout's docs. If you don't mind, in submitting the doc fix you've suggested for this, I'm going to change that "it" to node-pict, lest any other space cadet think "it" refers to the child rather than the pict.

rfindler commented 5 years ago

Wonderful, thank you!

I also think we need to chagne the contract from (the internal) tree-layout? function to (and/c tree-layout? (not/c #f)) so that the error messages come out right and match the docs.

rfindler commented 5 years ago

(using the external tree-layout? function in the second contract)

RenaissanceBug commented 5 years ago

OK, commit c088bb028865bf824651be5f515a66c0dc758f41 has those changes.

RenaissanceBug commented 5 years ago

And 5c4726bf5854452d5749c636d72c6939772646e1 has the internal fn on the actual (not docs) contract.