Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5497)

Issue 557670043: Revert "Load only the default font for System_start_delimiter"

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years ago by thomasmorley651
Modified:
4 years ago
Reviewers:
lemzwerg, hahnjo
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Revert "Load only the default font for System_start_delimiter" This reverts commit 430bad24a2d15ec6600e0e780348a0caff29799b. Regtest for setting SystemStartGrob.style to 'brace Checks whether the SystemStartBrace is printed

Patch Set 1 #

Total comments: 8

Patch Set 2 : Adressing Werner and Jonas #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -4 lines) Patch
A input/regression/system-start-brace-style.ly View 1 1 chunk +29 lines, -0 lines 0 comments Download
M lily/system-start-delimiter.cc View 1 1 chunk +6 lines, -4 lines 0 comments Download

Messages

Total messages: 5
thomasmorley651
Contains two commits: [PATCH 2/2] Revert "Load only the default font for System_start_delimiter" [PATCH 1/2] ...
4 years ago (2020-04-13 11:20:33 UTC) #1
hahnjo
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly File input/regression/system-start-brace-style.ly (right): https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly#newcode26 input/regression/system-start-brace-style.ly:26: '(SystemStartBracket SystemStartBrace SystemStartSquare SystemStartBar)) Not being super familiar with ...
4 years ago (2020-04-13 11:38:47 UTC) #2
lemzwerg
some nits https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly File input/regression/system-start-brace-style.ly (right): https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly#newcode4 input/regression/system-start-brace-style.ly:4: SystemStartGrob's style of @code{StaffGroup} to @code{'brace}, always ...
4 years ago (2020-04-13 11:58:53 UTC) #3
thomasmorley651
Adressing Werner and Jonas
4 years ago (2020-04-13 18:54:33 UTC) #4
thomasmorley651
4 years ago (2020-04-13 18:59:30 UTC) #5
https://codereview.appspot.com/557670043/diff/569610043/input/regression/syst...
File input/regression/system-start-brace-style.ly (right):

https://codereview.appspot.com/557670043/diff/569610043/input/regression/syst...
input/regression/system-start-brace-style.ly:4: SystemStartGrob's style of
@code{StaffGroup} to @code{'brace}, always prints a
On 2020/04/13 11:58:53, lemzwerg wrote:
> @code{SystemStart@var{Grob}} – and no comma after 'brace

Done.

https://codereview.appspot.com/557670043/diff/569610043/input/regression/syst...
input/regression/system-start-brace-style.ly:6: Every @code{StaffGroup} should
start with a @code{SystemStartBrace}.
On 2020/04/13 11:58:53, lemzwerg wrote:
> If you want a new paragraph, please add an empty line before the sentence.

Done.

https://codereview.appspot.com/557670043/diff/569610043/input/regression/syst...
input/regression/system-start-brace-style.ly:26: '(SystemStartBracket
SystemStartBrace SystemStartSquare SystemStartBar))
On 2020/04/13 11:38:47, hahnjo wrote:
> Not being super familiar with Scheme, this took me a while to understand. If I
> eventually did (correctly), I think just spelling out the four lines is more
> readable:
> \new StaffGroup \with { systemStartDelimiter = SystemStartBracket } << b1 b1
>>
> \new StaffGroup \with { systemStartDelimiter = SystemStartBrace } << b1 b1 >>
> \new StaffGroup \with { systemStartDelimiter = SystemStartSquare } << b1 b1 >>
> \new StaffGroup \with { systemStartDelimiter = SystemStartBar } << b1 b1 >>

Done.

https://codereview.appspot.com/557670043/diff/569610043/lily/system-start-del...
File lily/system-start-delimiter.cc (right):

https://codereview.appspot.com/557670043/diff/569610043/lily/system-start-del...
lily/system-start-delimiter.cc:151: esp. because that triggers mktextfm for
non-existent
On 2020/04/13 11:58:53, lemzwerg wrote:
> mktextfm is no longer relevant to LilyPond since we don't have a TeX back-end
> anymore.  Maybe the comment should be just
> 
>   We use the style sheet to look up the font file
>   name.  This is better than using 'find_font' directly.

Done with a third patch. Looks cleaner to leave the revert as is.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b