Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

Thomas Morley-2
Reviewers: ,

Message:
Please review.
There are some TODOs in the code where I'd appreciate some feedback.
Thanks-

Description:
Improve markup->string

'all-relevant-markup-commands' is now a toplevel-defined procedure.
So it is not longer a part of the rekursive 'markup->string'.
It needs to be a procedure, because not all bindings of the
lily-module are already done in markup.scm, so it should be
evaluated at the time 'markup->string' is called.
Additionally we gain the chance to have user-defined
markup-commands from '(current-module)' been processed as well.

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

Affected files (+31, -23 lines):
   M scm/markup.scm


Index: scm/markup.scm
diff --git a/scm/markup.scm b/scm/markup.scm
index  
14a007a95bb27f8a89da2aeb51a607695af40068..169e52c78e3b8b15c560ff7efffce14f95aca279  
100644
--- a/scm/markup.scm
+++ b/scm/markup.scm
@@ -79,29 +79,38 @@ following stencil.  Stencils with empty Y extent are  
not given
  (define markup-commands-to-ignore
    '(page-ref-markup))

+(define (all-relevant-markup-commands)
+  ;; Returns a list containing the names of all markup-commands and
+  ;; markup-list-commands with predicate @code{cheap-markup?} or
+  ;; @code{markup-list?} in their @code{markup-command-signature}.
+  ;; It needs to be a procedure, because before it is called in
+  ;; @code{markup->string}, not all bindings in the lily-module are done  
and we
+  ;; want to catch user-defined markup-commands from @code{current-module}
+  ;; as well.
+  ;; @code{table-of-contents} is not caught, though.
+  ;; Markup-commands from @code{markup-commands-to-ignore} are removed.
+  (lset-difference eq?
+    (map car
+      (filter
+        (lambda (x)
+          (let* ((predicates (markup-command-signature (cdr x))))
+            (and predicates
+                 (not
+                   (null?
+                     (lset-intersection eq?
+                       '(cheap-markup? markup-list?)
+                       (map procedure-name predicates)))))))
+        ;; TODO
+        ;;  - other modules to look at?
+        ;;  - need to care about conflicts/duplicates?
+        (append
+          (ly:module->alist (current-module))
+          (ly:module->alist (resolve-module '(lily))))))
+    markup-commands-to-ignore))
+
  (define-public (markup->string m . argscopes)
    (let* ((scopes (if (pair? argscopes) (car argscopes) '())))

-    (define all-relevant-markup-commands
-      ;; Returns a list containing the names of all markup-commands and
-      ;; markup-list-commands with predicate @code{cheap-markup?} or
-      ;; @code{markup-list?} in their @code{markup-command-signature}.
-      ;; @code{table-of-contents} is not caught, same for user-defined  
commands.
-      ;; markup-commands from @code{markup-commands-to-ignore} are removed.
-      (lset-difference eq?
-        (map car
-          (filter
-            (lambda (x)
-              (let* ((predicates (markup-command-signature (cdr x))))
-                (and predicates
-                     (not
-                       (null?
-                         (lset-intersection eq?
-                           '(cheap-markup? markup-list?)
-                           (map procedure-name predicates)))))))
-            (ly:module->alist (resolve-module '(lily)))))
-        markup-commands-to-ignore))
-
      ;; helper functions to handle string cons like string lists
      (define (markup-cons->string-cons c scopes)
        (if (not (pair? c)) (markup->string c scopes)
@@ -113,8 +122,7 @@ following stencil.  Stencils with empty Y extent are  
not given
            (string-join (list (car c) (string-cons-join (cdr c))) "")))

      ;; We let the following line in for future debugging
-    ;; (display-scheme-music (sort all-relevant-markup-commands symbol<?))
-
+    ;; (display-scheme-music (sort (all-relevant-markup-commands)  
symbol<?))

      ;;;; Remark: below only works, if markup?- or markup-list? arguments  
are the
      ;;;;         last listed arguments in the commands definition
@@ -158,7 +166,7 @@ following stencil.  Stencils with empty Y extent are  
not given
          (markup->string (ly:modules-lookup scopes var) (cons mod scopes))))

       ((member (car m)
-              (primitive-eval (cons 'list all-relevant-markup-commands)))
+              (primitive-eval (cons 'list (all-relevant-markup-commands))))
        (markup->string
          (if (> (length (last-pair m)) 1)
              (last-pair m)



_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

David Kastrup
I wonder whether it might be reasonable to just have all markup commands
with a last argument of markup? produce their last (recursively treated)
argument as default, and possibly all markup list commands with a last
argument of markup-list? similarly produce their last argument?  That
leaves fewer commands to cater for and would also make some user-defined
commands work reasonably well.

https://codereview.appspot.com/347000043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

Thomas Morley-2
In reply to this post by Thomas Morley-2
On 2018/11/10 12:53:18, dak wrote:
> I wonder whether it might be reasonable to just have all markup
commands with a
> last argument of markup? produce their last (recursively treated)
argument as
> default, and possibly all markup list commands with a last argument of
> markup-list? similarly produce their last argument?  That leaves fewer
commands
> to cater for and would also make some user-defined commands work
reasonably
> well.

I think I do not fully understand what you mean.

'all-relevant-markup-commands' tries to get all markup-(list-)commands
where 'markup->string' may return reasonable output.
Currently by looking whether 'cheap-markup?' or 'markup-list?' are
somewhere in their predicates.
This could be changed to look whether those are last in predicates.
Would simplify the coding and 'markup->string' only works in this case
anyway. (At least following a comment there.)

So far I understood, at least I hope so.
I have some other simplifications in mind, will upload with next patch.
Though, the amount of caught markup-(list-)commands is the same for both
codings.

But what do you mean with "produce their last (recursively treated)
argument as default"
Could you elaborate a bit?



https://codereview.appspot.com/347000043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

David Kastrup
In reply to this post by Thomas Morley-2
On 2018/11/11 10:57:31, thomasmorley651 wrote:
> On 2018/11/10 12:53:18, dak wrote:
> > I wonder whether it might be reasonable to just have all markup
commands with
> a
> > last argument of markup? produce their last (recursively treated)
argument as
> > default, and possibly all markup list commands with a last argument
of
> > markup-list? similarly produce their last argument?  That leaves
fewer
> commands
> > to cater for and would also make some user-defined commands work
reasonably
> > well.

> I think I do not fully understand what you mean.

More a case of me talking about stuff I know nothing about.

> 'all-relevant-markup-commands' tries to get all markup-(list-)commands
where
> 'markup->string' may return reasonable output.

I have to get used to the thought of not having the monopoly on good
ideas.  That's all.  I had assumed that you work with large lists of
markup commands and rules rather than this kind of deduction and didn't
even bother to check.

Sorry for the noise.

https://codereview.appspot.com/347000043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

Thomas Morley-2
In reply to this post by Thomas Morley-2
On 2018/11/11 11:07:35, dak wrote:
> On 2018/11/11 10:57:31, thomasmorley651 wrote:

> > 'all-relevant-markup-commands' tries to get all
markup-(list-)commands where
> > 'markup->string' may return reasonable output.

> I have to get used to the thought of not having the monopoly on good
ideas.
> That's all.

Well, I'm pretty sure while working on
https://sourceforge.net/p/testlilyissues/issues/4685/
you pointed me in the direction how to deduce things like that. ;)

Though, I just noticed I introduced a so far unnoticed glitch with 4685
markup->string doesn't work on \simple
I looked which other markup commands are unnoticed thrown away. Below
they are listed, devided into worth-to-keep and to-ignore, as far as I
would think:

worth-to-keep:
   wordwrap-string-markup
   justify-string-markup
   simple-markup
   wordwrap-string-internal-markup-list

to-ignore:
   lookup-markup
   postscript-markup
   epsfile-markup
   fret-diagram-markup
   tied-lyric-markup
   musicglyph-markup
   fret-diagram-terse-markup
   rest-markup
   verbatim-file-markup
   harp-pedal-markup

For wordwrap-string-markup and justify-string-markup I think they should
be special-cased, meaning deleting tab and newline-characters. Maybe
simple-markup can be put there as well. Not sure about
wordwrap-string-internal-markup-list, keep or ignore?

The to-ignore-markups could be appended to the list already defined with
'markup-commands-to-ignore'.
Though, this would introduce a manually to maintain instance which I'd
like to avoid, but I'm not aware of a condition to exlude them. Otoh
'markup-commands-to-ignore' has already an entry, so it needs to be
mantained anyway.

Hmm.

Hints? Opinions?



https://codereview.appspot.com/347000043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

paulwmorris
In reply to this post by Thomas Morley-2
Hi Harm, I took a quick look and it LGTM at a quick read. I have a
couple nit-level suggestions.
-Paul


https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm
File scm/markup.scm (right):

https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm#newcode141
scm/markup.scm:141: ;; The string is split at line-breaks, emty strings
removed and finally
typo: empty

https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm#newcode148
scm/markup.scm:148: simple-markup)))
Indent 'list' expression so it is clearer that it is the second argument
to 'member' and not another 'and' expression.

https://codereview.appspot.com/347000043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

Thomas Morley-2
In reply to this post by Thomas Morley-2
On 2018/11/11 15:08:57, thomasmorley651 wrote:
> simplify, catch string-markups

patch-set 3 reflects my musing in comment #5

https://codereview.appspot.com/347000043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Improve markup->string (issue 347000043 by thomasmorley65@gmail.com)

Thomas Morley-2
In reply to this post by Thomas Morley-2
Hi Paul,

thanks for review


https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm
File scm/markup.scm (right):

https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm#newcode141
scm/markup.scm:141: ;; The string is split at line-breaks, emty strings
removed and finally
On 2018/11/11 15:43:37, pwm wrote:
> typo: empty

Done.

https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm#newcode148
scm/markup.scm:148: simple-markup)))
On 2018/11/11 15:43:37, pwm wrote:
> Indent 'list' expression so it is clearer that it is the second
argument to
> 'member' and not another 'and' expression.

Done.

https://codereview.appspot.com/347000043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel