Re: Automatically sort alist grob-properties in IR. (issue 102760044)

[markpolesky]

Reviewers: dak,


https://codereview.appspot.com/102760044/diff/1/scm/document-backend.scm
File scm/document-backend.scm (right):

https://codereview.appspot.com/102760044/diff/1/scm/document-backend.scm#newcode22
scm/document-backend.scm:22: (apply eq? #t (map pair? x))))
On 2014/05/28 00:00:58, dak wrote:
> (apply eq? #t (map pair? x)) is horribly contorted and subject
> to limitations in argument list length.  Use (every pair? x)
> which has the advantage of short-circuit evaluation.

Ah, that's the procedure I was looking for, thanks!  Sorry, I didn't
know about that one.

> Also, it seems like a stretch to assume that every list with
> pair members will tolerate sorting.

Do you mean that
1) the order of keys in some alists may be significant?
or
2) my code might trigger a scheme error during sorting?

Regarding #1, I *am* assuming that key-order is irrelevant, but if
that's misguided, let me know.  Regarding #2, I thought my definition of
`ly:alist<?' covered that with the `else' clause:

(else
  (ly:string<? (object->string (car a))
               (object->string (car b))))

> You do not even restrict this to lists with symbols in the car
> and explicitly sort lists with non-symbols.
> That seems like a bad idea.

Not trying to be annoying (really), but why?

https://codereview.appspot.com/102760044/diff/1/scm/lily-sort.scm
File scm/lily-sort.scm (right):

https://codereview.appspot.com/102760044/diff/1/scm/lily-sort.scm#newcode112
scm/lily-sort.scm:112: ((and (number? (car a)) (number? (car b)))
On 2014/05/28 00:00:58, dak wrote:
> Can you point to any "alists" where the keys are numbers?  This seems
rather
> fishy to me.

Accidental.glyph-name-alist:
'((0 . accidentals.natural)
  (-1/2 . accidentals.flat)
  (1/2 . accidentals.sharp)
  (1 . accidentals.doublesharp)
  (-1 . accidentals.flatflat)
  (3/4 . accidentals.sharp.slashslash.stemstemstem)
  (1/4 . accidentals.sharp.slashslash.stem)
  (-1/4 . accidentals.mirroredflat)
  (-3/4 . accidentals.mirroredflat.flat))

Finally, for what it's worth, here's the list of all alist
grob-properties:
http://www.markpolesky.com/norobots/alist-grob-properties.txt

Thanks.
- Mark

Description:
Automatically sort alist grob-properties in IR.
Also, rewrite sorting code to be easier to understand.

This will ensure that grob-properties whose default values are alists
(like 'details or 'glyph-name-alist) will get sorted by key before being
displayed in the IR section `All layout objects'.

http://code.google.com/p/lilypond/issues/detail?id=3932

Please review this at https://codereview.appspot.com/102760044/

Affected files (+67, -28 lines):
  M scm/document-backend.scm
  M scm/lily-sort.scm


Index: scm/document-backend.scm
diff --git a/scm/document-backend.scm b/scm/document-backend.scm
index  
80ba17a3ba34cd1958f80fa6b72bae234613c008..272b7f36a7ed9a592740841141476dcfcc204404  
100644
--- a/scm/document-backend.scm
+++ b/scm/document-backend.scm
@@ -16,27 +16,50 @@
 ;;;; You should have received a copy of the GNU General Public License
 ;;;; along with LilyPond.  If not, see <http://www.gnu.org/licenses/>.

-(define (sort-grob-properties x)
-  ;; force 'meta to the end of each prop-list
-  (let ((meta (assoc 'meta x)))
-    (append (sort (assoc-remove! x 'meta) ly:alist-ci<?)
-            (list meta))))

-;; properly sort all grobs, properties, and interfaces
+(define (alist? x)
+  (and (list? x)
+       (apply eq? #t (map pair? x))))
+
+(define (alist-prop? x)
+   (alist? (cdr x)))
+
+(define (not-alist-prop? x)
+   (not (alist-prop? x)))
+
+(define (sort-grob-properties x)
+  ;; props that are alists (eg. 'details) get sorted by key.
+  ;; also, force 'meta to the end of each prop-list.
+  (let* ((meta            (assoc 'meta x))
+         (all-but-meta    (assoc-remove! x 'meta))
+         (alist-props     (filter alist-prop? all-but-meta))
+         (non-alist-props (filter not-alist-prop? all-but-meta))
+         (sorted-alist-props
+           (map (lambda (x) (cons (car x)
+                                  (sort (cdr x) ly:alist-ci<?)))
+                alist-props))
+         (all-but-meta-sorted
+           (sort (append sorted-alist-props non-alist-props)  
ly:alist-ci<?)))
+    (append all-but-meta-sorted (list meta))))
+
+;; properly sort all properties, and interfaces
 ;; within the all-grob-descriptions alist
 (for-each
- (lambda (x)
-   (let* ((props      (assoc-ref all-grob-descriptions (car x)))
-          (meta       (assoc-ref props 'meta))
-          (interfaces (assoc-ref meta 'interfaces)))
-     (set! all-grob-descriptions
-           (sort (assoc-set! all-grob-descriptions (car x)
-                             (sort-grob-properties
-                              (assoc-set! props 'meta
-                                          (assoc-set! meta 'interfaces
-                                                      (sort interfaces  
ly:symbol-ci<?)))))
-                 ly:alist-ci<?))))
- all-grob-descriptions)
+   (lambda (x)
+     (let* ((grob-key      (car x))
+            (props         (assoc-ref all-grob-descriptions grob-key))
+            (meta          (assoc-ref props 'meta))
+            (interfaces    (assoc-ref meta 'interfaces))
+            (sorted-ifaces (sort interfaces ly:symbol-ci<?))
+            (new-meta      (assoc-set! meta 'interfaces sorted-ifaces))
+            (new-props     (assoc-set! props 'meta new-meta))
+            (sorted-props  (sort-grob-properties new-props)))
+       (set! all-grob-descriptions
+         (assoc-set! all-grob-descriptions grob-key sorted-props))))
+   all-grob-descriptions)
+
+;; sort all grobs in the all-grob-descriptions alist
+(set! all-grob-descriptions (sort all-grob-descriptions ly:alist-ci<?))

 (define (interface-doc-string interface grob-description)
   (let* ((name (car interface))
Index: scm/lily-sort.scm
diff --git a/scm/lily-sort.scm b/scm/lily-sort.scm
index  
e05c15a61189b18e42417c517e8a7cb11494bc18..f59b8ba500714210a14c3870f854104e744259fb  
100644
--- a/scm/lily-sort.scm
+++ b/scm/lily-sort.scm
@@ -102,15 +102,31 @@
                   (symbol->string b)))

 (define (ly:alist<? a b)
-  "Return #t if the first key of alist A is less than the first key of
-  alist B, using case-sensitive LilyPond sort order.  Keys are assumed to
-  be symbols."
-  (ly:string<? (symbol->string (car a))
-               (symbol->string (car b))))
+  "Return #t if the first key of alist A is less than the first
+  key of alist B, using case-sensitive LilyPond sort order.
+  Keys are assumed to be numbers or symbols."
+   (cond
+     ((and (symbol? (car a)) (symbol? (car b)))
+      (ly:string<? (symbol->string (car a))
+                   (symbol->string (car b))))
+     ((and (number? (car a)) (number? (car b)))
+      (< (car a) (car b)))
+     (else
+      (ly:string<? (object->string (car a))
+                   (object->string (car b))))))

 (define (ly:alist-ci<? a b)
-  "Return #t if the first key of alist A is less than the first key of
-  alist B, using case-insensitive LilyPond sort order.  Keys are assumed
-  to be symbols."
-  (ly:string-ci<? (symbol->string (car a))
-                  (symbol->string (car b))))
+  "Return #t if the first key of alist A is less than the first
+  key of alist B, using case-insensitive LilyPond sort order.
+  Keys are assumed to be numbers or symbols."
+   (cond
+     ((and (symbol? (car a)) (symbol? (car b)))
+      (ly:string-ci<? (symbol->string (car a))
+                      (symbol->string (car b))))
+     ((and (number? (car a)) (number? (car b)))
+      (< (car a) (car b)))
+     (else
+      (ly:string-ci<? (object->string (car a))
+                      (object->string (car b))))))
+
+