|
|
DescriptionAllow a markup to replace the default LyricHyphen
Solves issue #1255.
Signed-off-by: Knut Petersen <knut_petersen@t-online.de>
Patch Set 1 #
Total comments: 2
Patch Set 2 : Version 2 of issue #1255 patch #
Total comments: 7
MessagesTotal messages: 9
https://codereview.appspot.com/325470043/diff/1/lily/lyric-hyphen.cc File lily/lyric-hyphen.cc (right): https://codereview.appspot.com/325470043/diff/1/lily/lyric-hyphen.cc#newcode55 lily/lyric-hyphen.cc:55: bool use_markup = to_boolean (me->get_property ("use-markup")); That property seems spurious. Why not just use the "text" property when it is a markup and create a graphic hyphen when it isn't?
Sign in to reply to this message.
Version 2 of issue #1255 patch
Sign in to reply to this message.
https://codereview.appspot.com/325470043/diff/1/lily/lyric-hyphen.cc File lily/lyric-hyphen.cc (right): https://codereview.appspot.com/325470043/diff/1/lily/lyric-hyphen.cc#newcode55 lily/lyric-hyphen.cc:55: bool use_markup = to_boolean (me->get_property ("use-markup")); On 2017/09/14 09:34:03, dak wrote: > That property seems spurious. Why not just use the "text" property when it is a > markup and create a graphic hyphen when it isn't? Ok. I uploaded a revised version.
Sign in to reply to this message.
Rats, forgot to publish my code comments. Indepent of those: I don't actually see this addressing issue 1255, so maybe create a new issue in the Sourceforge issue tracker for it and attach this Rietveld issue? It seems at best loosely related to issue #1255 but seems to be desirable nevertheless. https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc File lily/lyric-hyphen.cc (right): https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc#newc... lily/lyric-hyphen.cc:125: Box b (Interval (0, dash_length), Interval (h, h + th)); It seems wasteful to put these in the loop, particularly since dash_mol is then additionally copied before shifting. Maybe pull if (Text_interface::is_markup ...) outside of the loop and have a separate loop for each branch? There is no shared loop code outside of if/else in the current implementation anyway. https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm#newc... scm/define-grobs.scm:1403: (text . '()) That's the default when unset. For properties that may optionally be set, in particular when this default formally does not match the predicate, I think we tend not to explicitly specify it.
Sign in to reply to this message.
On 2017/09/16 19:53:57, dak wrote: > Indepent of those: I don't actually see this addressing issue 1255, so maybe > create a new issue in the Sourceforge issue tracker for it and attach this > Rietveld issue? It seems at best loosely related to issue #1255 but seems to be > desirable nevertheless. Well, issue 1255 addresses the problem that the hyphen glyph of a lyric font and the LyricHyphen grob are not identical. It cites a source code comment: "TODO: should extract hyphen dimensions or hyphen glyph from the font". It cites Alexander Kobel: "Once I also tried to exchange the LyricHyphen stencil to mimic LyricText, but to no avail - AFAICS, the alignment of the hyphens is more or less hard-coded in the C++ part." There is more than one possible solutions if you have to mix LyricHyphens and hyphen glyphs of the lyric font and if those should be (almost) identical: 1) Always use the hyphen glyph. Easy with this patch: set 'text to "-". 2) Always use a rounded box, never use a hyphen glyph. It would be possible to replace all hyphen glyphs in LyricText with a rounded box. A good solution only for a subset of the available fonts. 3) Try to minimize the difference between the hyphen glyph of the font and the LyricHyphen grob. Automatic extraction of the dimensions of the hyphen glyph would give a reasonable solution only for the subset of available fonts with hyphens glyphs that have a form close to a rounded box. A manually constructed hyphen is possible with this patch. I think there are good reasons to think that the patch is a reasonable solution to the problem described in issue 1255.
Sign in to reply to this message.
https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc File lily/lyric-hyphen.cc (right): https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc#newc... lily/lyric-hyphen.cc:125: Box b (Interval (0, dash_length), Interval (h, h + th)); On 2017/09/16 19:53:56, dak wrote: > It seems wasteful to put these in the loop, particularly since dash_mol is then > additionally copied before shifting. Maybe pull if (Text_interface::is_markup > ...) outside of the loop and have a separate loop for each branch? There is no > shared loop code outside of if/else in the current implementation anyway. Well, you are right. https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc#newc... lily/lyric-hyphen.cc:126: Stencil dash_mol (Lookup::round_filled_box (b, 0.8 * lt)); Shouldn't the hardcoded 0.8 be replaced by a property? https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm#newc... scm/define-grobs.scm:1403: (text . '()) On 2017/09/16 19:53:56, dak wrote: > That's the default when unset. For properties that may optionally be set, in > particular when this default formally does not match the predicate, I think we > tend not to explicitly specify it. Well, omitting it here means that there is no indication in the internals reference that LyricHyphen uses that property.
Sign in to reply to this message.
On 2017/09/17 10:02:41, knupero wrote: > On 2017/09/16 19:53:57, dak wrote: > > > Indepent of those: I don't actually see this addressing issue 1255, so maybe > > create a new issue in the Sourceforge issue tracker for it and attach this > > Rietveld issue? It seems at best loosely related to issue #1255 but seems to > be > > desirable nevertheless. > > Well, issue 1255 addresses the problem that the hyphen glyph of a lyric font and > the LyricHyphen grob are not identical. Issue 1255 is: "#1255 Extract hyphen dimensions and/or hyphen glyph from the font " Your patch does not extract hyphen dimensions or the hyphen glyph from the font. It might get used for using the hyphen glyph (provided that you know its font-dependent character code) as a hyphen, so it might contribute to deciding to retire issue #1255 "Wontfix". > It cites a source code comment: "TODO: should extract hyphen dimensions or > hyphen glyph from the font". > > It cites Alexander Kobel: "Once I also tried to exchange the LyricHyphen stencil > to mimic LyricText, but to no avail - AFAICS, the alignment of the hyphens is > more or less hard-coded in the C++ part." "Once I also tried" points to a related problem. > I think there are good reasons to think that the patch is a reasonable solution > to the problem described in issue 1255. Issue 1255 does not describe a _problem_ but a particular desired functionality. This functionality is not the one you implement but you state that the underlying problem is the same. People refer to issue numbers, and search the issues with search terms. When implementing different functionality, it thus makes sense to create a separate issue and cross-reference the issues.
Sign in to reply to this message.
https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc File lily/lyric-hyphen.cc (right): https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc#newc... lily/lyric-hyphen.cc:126: Stencil dash_mol (Lookup::round_filled_box (b, 0.8 * lt)); On 2017/09/17 10:16:01, knupero wrote: > Shouldn't the hardcoded 0.8 be replaced by a property? We have a lot of hardcoded typesetting. Unless a particular need for more flexibility or variation is reported, we tend to keep things to perceived best practice. That also gives us the flexibility to change such values based on the actual situation if that seems warranted, rather than leaving it to the user to optimize each case. Do you see a particular reason why making this configurable might be needed? https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm#newc... scm/define-grobs.scm:1403: (text . '()) On 2017/09/17 10:16:01, knupero wrote: > On 2017/09/16 19:53:56, dak wrote: > > That's the default when unset. For properties that may optionally be set, in > > particular when this default formally does not match the predicate, I think we > > tend not to explicitly specify it. > > Well, omitting it here means that there is no indication in the internals > reference that LyricHyphen uses that property. Which is the same with every overridable grob having a text-interface, like, say, ChordName or InstrumentName . If you want to have this policy changed, the way to do that is a separate patch just doing that everywhere, not changing stuff piecemeal. Though since the Internals Reference is already rather large, the enthusiasm for explicitly documenting Grob properties that are left at their interface default will likely not be all that large, particularly since the actual documentation is _not_ specific to the grob but recruited from the respective interface definition.
Sign in to reply to this message.
> Issue 1255 is: "#1255 Extract hyphen dimensions and/or hyphen glyph from the > font " > > Your patch does not extract hyphen dimensions or the hyphen glyph from the font. > It might get used for using the hyphen glyph (provided that you know its > font-dependent character code) as a hyphen, so it might contribute to deciding > to retire issue #1255 "Wontfix". > I opened a new issue 5210 and uploaded extended code for review. So this issue here is dead.
Sign in to reply to this message.
|