Issue 289980043: Introduce markup-list-command table (Closed)
|
Created:
8 years, 2 months ago by thomasmorley651 Modified:
8 years, 2 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionIntroduce markup-list-command table
Patch Set 1 #
Total comments: 8
Patch Set 2 : Carl's and David's comments #Patch Set 3 : correct oversight #
Total comments: 3
Patch Set 4 : (partly) adressing David's comment #Patch Set 5 : fully adressing David's comments #Patch Set 6 : further improvements #MessagesTotal messages: 14
Sign in to reply to this message.
LGTM -- just a couple of minor changes in documentation/comments https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4724: The amount of columns is specifies by @var{columns}. Each column may be aligned Should read "The number of columns is specified" https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4814: ;; aligned, (-1, 0, 1) means (LEFT, RIGHT, CENTER) Surely you mean (LEFT, CENTER, RIGHT), don't you?
Sign in to reply to this message.
https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4767: ;; -> (list 0 1 3 6 10) It's not clear why rl is even a list if it always has to have exactly one member. And the (reverse rl) indicates that this function does something else. Also the comment does not mention the function of "padding". So the function description tries to be sort-of abstract by avoiding to mention what the function is actually being used for, but there are lots of missing pieces which can only be guessed if you actually know what is being done here. Bad combination. Try being more complete. I suspect that this will yield to a call of fold, but it's hard to say with the documentation not really matching the code. https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4783: (split-lst lst columns '()))) We've had this a few times. A markup-list? argument is not necessarily a list of markups. It can also be a markup list command call. The only thing you can reliably do before starting list processing is to call interpret-markup-list on the _whole_ markup-list argument. Only _then_ can you start splitting the resulting list of stencils.
Sign in to reply to this message.
Carl's and David's comments
Sign in to reply to this message.
https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4724: The amount of columns is specifies by @var{columns}. Each column may be aligned On 2016/01/30 23:02:10, Carl wrote: > Should read "The number of columns is specified" Done. https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4767: ;; -> (list 0 1 3 6 10) On 2016/01/30 23:32:17, dak wrote: > It's not clear why rl is even a list if it always has to have exactly one > member. And the (reverse rl) indicates that this function does something else. > > Also the comment does not mention the function of "padding". So the function > description tries to be sort-of abstract by avoiding to mention what the > function is actually being used for, but there are lots of missing pieces which > can only be guessed if you actually know what is being done here. > > Bad combination. Try being more complete. I suspect that this will yield to a > call of fold, but it's hard to say with the documentation not really matching > the code. Changed it using fold. Better description (hopefully) https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4783: (split-lst lst columns '()))) On 2016/01/30 23:32:17, dak wrote: > We've had this a few times. A markup-list? argument is not necessarily a list > of markups. It can also be a markup list command call. Aargh, I keep forgetting about it. > The only thing you can > reliably do before starting list processing is to call interpret-markup-list on > the _whole_ markup-list argument. Only _then_ can you start splitting the > resulting list of stencils. Done. https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:4814: ;; aligned, (-1, 0, 1) means (LEFT, RIGHT, CENTER) On 2016/01/30 23:02:10, Carl wrote: > Surely you mean (LEFT, CENTER, RIGHT), don't you? Done.
Sign in to reply to this message.
correct oversight
Sign in to reply to this message.
https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... File scm/define-markup-commands.scm (right): https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... scm/define-markup-commands.scm:4724: The number of columns is specifies by @var{columns}. Each column may be aligned Where is the point in specifying the number of columns when it is (length column-align)? Or am I overlooking something? Is xoff needed or can one just write \put-adjacent #X #RIGHT \hspace #3 \table ... or use \pad-x or similar (strangely, we don't seem to have horizontal equivalents to \raise and \lower). A number of other commands does not offer an explicit indent, so I don't quite see that this command would be special.
Sign in to reply to this message.
(partly) adressing David's comment
Sign in to reply to this message.
https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... File scm/define-markup-commands.scm (right): https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... scm/define-markup-commands.scm:4724: The number of columns is specifies by @var{columns}. Each column may be aligned On 2016/01/31 13:43:30, dak wrote: > Where is the point in specifying the number of columns when it is (length > column-align)? Or am I overlooking something? Although you're correct: while testing, I found it comfortable to write a longer list for column-align than the number of columns, because I could easily change between different numbers of columns. I've let it unchanged for now, though I'll not insist on it. Should I change this as well? > Is xoff needed or can one just write > > \put-adjacent #X #RIGHT \hspace #3 \table ... > > or use \pad-x or similar (strangely, we don't seem to have horizontal > equivalents to \raise and \lower). > > A number of other commands does not offer an explicit indent, so I don't quite > see that this command would be special. Good point. x-off removed.
Sign in to reply to this message.
https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... File scm/define-markup-commands.scm (right): https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... scm/define-markup-commands.scm:4724: The number of columns is specifies by @var{columns}. Each column may be aligned On 2016/01/31 14:34:22, thomasmorley651 wrote: > On 2016/01/31 13:43:30, dak wrote: > > Where is the point in specifying the number of columns when it is (length > > column-align)? Or am I overlooking something? > > Although you're correct: > while testing, I found it comfortable to write a longer list for column-align > than the number of columns, because I could easily change between different > numbers of columns. Anything wrong with -- Scheme Procedure: list-head lst k -- C Function: scm_list_head (lst, k) Copy the first K elements from LST into a new list, and return it. ? It would seem easy enough to drop this into your argument for debugging purposes and then just jiggle k.
Sign in to reply to this message.
fully adressing David's comments
Sign in to reply to this message.
On 2016/01/31 14:56:37, dak wrote: > https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... > File scm/define-markup-commands.scm (right): > > https://codereview.appspot.com/289980043/diff/40001/scm/define-markup-command... > scm/define-markup-commands.scm:4724: The number of columns is specifies by > @var{columns}. Each column may be aligned > On 2016/01/31 14:34:22, thomasmorley651 wrote: > > On 2016/01/31 13:43:30, dak wrote: > > > Where is the point in specifying the number of columns when it is (length > > > column-align)? Or am I overlooking something? > > > > Although you're correct: > > while testing, I found it comfortable to write a longer list for column-align > > than the number of columns, because I could easily change between different > > numbers of columns. > > Anything wrong with > > -- Scheme Procedure: list-head lst k > -- C Function: scm_list_head (lst, k) > Copy the first K elements from LST into a new list, and return it. > > ? It would seem easy enough to drop this into your argument for debugging > purposes and then just jiggle k. Convinced. Changed accordingly.
Sign in to reply to this message.
further improvements
Sign in to reply to this message.
On 2016/01/30 23:32:17, dak wrote: > https://codereview.appspot.com/289980043/diff/1/scm/define-markup-commands.sc... > scm/define-markup-commands.scm:4783: (split-lst lst columns '()))) > We've had this a few times. A markup-list? argument is not necessarily a list > of markups. It can also be a markup list command call. The only thing you can > reliably do before starting list processing is to call interpret-markup-list on > the _whole_ markup-list argument. Only _then_ can you start splitting the > resulting list of stencils. Thinking more about: it's not easily predictable how many stencils a markup list command call will return. Thus I extended the code, adding point-stencils to last row of the table, if needed, i.e. if (pseudo-code) (/ (length stencils) number-of-columns) is not integer.
Sign in to reply to this message.
|
