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

Issue 137920043: Issue 4083: Implement \tagGroup command (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by dak
Modified:
9 years, 5 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Issue 4083: Implement \tagGroup command After mulling this over and figuring out that declaring a \tagGroup will not just keep \keepWithTag of some package unaffected by any tags otherwise in use but will _also_ hide the use of tags internal to the package from any outside use of \keepWithTag, I decided to go forward on this approach. The given implementation does "nothing special" for \keepWithTag and \removeWithTag when given tags from different tag groups, or when defining the same tag group several times (possibly by loading some code twice). It is arguable that either could warrant a warning. However, the functionality of \keepWithTag #'(fromgroupI fromgroupII) cannot easily be provided by anything else. While I currently cannot imagine a useful application for it myself, the implemented behavior is logically consistent. Also contains: Basic documentation for \tagGroup command

Patch Set 1 #

Patch Set 2 : Fix typo, add regtest #

Total comments: 5

Patch Set 3 : Reimplement and redocument according to Keith's comments and my own insights #

Total comments: 3

Patch Set 4 : Try incorporating Trevor's suggestions in the most accurate manner #

Patch Set 5 : Fix tag/tags confusion in doc strings #

Total comments: 2

Patch Set 6 : Aaand back again. Documentation appears deteriorating but I'm getting tired. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -36 lines) Patch
M Documentation/notation/input.itely View 1 2 3 4 5 4 chunks +36 lines, -9 lines 0 comments Download
A input/regression/tag-group.ly View 1 1 chunk +78 lines, -0 lines 0 comments Download
M ly/music-functions-init.ly View 1 2 3 4 5 3 chunks +25 lines, -27 lines 0 comments Download
M scm/music-functions.scm View 1 2 3 4 5 1 chunk +60 lines, -0 lines 0 comments Download

Messages

Total messages: 18
dak
Fix typo, add regtest
9 years, 7 months ago (2014-09-02 13:15:05 UTC) #1
Keith
LGTM It worked fine on (adapted) examples from the divisi staff issue and emails. Looking ...
9 years, 6 months ago (2014-09-05 06:59:18 UTC) #2
dak
On 2014/09/05 06:59:18, Keith wrote: > LGTM > It worked fine on (adapted) examples from ...
9 years, 6 months ago (2014-09-05 07:29:15 UTC) #3
mark_opus11.net
https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/input.itely File Documentation/notation/input.itely (left): https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/input.itely#oldcode2075 Documentation/notation/input.itely:2075: @funindex \appendToTag Why are these index lines removed? The ...
9 years, 6 months ago (2014-09-05 08:13:38 UTC) #4
dak
mark.opus11@googlemail.com writes: > https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/input.itely > File Documentation/notation/input.itely (left): > > https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/input.itely#oldcode2075 > Documentation/notation/input.itely:2075: @funindex \appendToTag ...
9 years, 6 months ago (2014-09-05 08:36:18 UTC) #5
dak
Reimplement and redocument according to Keith's comments and my own insights
9 years, 6 months ago (2014-09-05 15:41:48 UTC) #6
Trevor Daniels
Apart from an inconsistency, marked below, LGTM. https://codereview.appspot.com/137920043/diff/40001/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/137920043/diff/40001/ly/music-functions-init.ly#newcode562 ly/music-functions-init.ly:562: more than ...
9 years, 6 months ago (2014-09-05 19:03:28 UTC) #7
dak
https://codereview.appspot.com/137920043/diff/40001/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/137920043/diff/40001/ly/music-functions-init.ly#newcode562 ly/music-functions-init.ly:562: more than one tag group defined with @code{\\tagGroup}, there ...
9 years, 6 months ago (2014-09-05 20:59:56 UTC) #8
Trevor Daniels
https://codereview.appspot.com/137920043/diff/40001/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/137920043/diff/40001/ly/music-functions-init.ly#newcode562 ly/music-functions-init.ly:562: more than one tag group defined with @code{\\tagGroup}, there ...
9 years, 6 months ago (2014-09-05 22:14:26 UTC) #9
dak
Try incorporating Trevor's suggestions in the most accurate manner
9 years, 6 months ago (2014-09-05 23:07:03 UTC) #10
dak
Fix tag/tags confusion in doc strings
9 years, 6 months ago (2014-09-06 08:07:00 UTC) #11
Trevor Daniels
LGTM. Thanks David. Trevor
9 years, 6 months ago (2014-09-06 09:24:07 UTC) #12
Keith
The behavior of the old patch seemed better, in the case where someone does combine ...
9 years, 6 months ago (2014-09-06 18:55:42 UTC) #13
dak
On 2014/09/06 18:55:42, Keith wrote: > The behavior of the old patch seemed better, in ...
9 years, 6 months ago (2014-09-06 19:45:40 UTC) #14
dak
https://codereview.appspot.com/137920043/diff/80001/Documentation/notation/input.itely File Documentation/notation/input.itely (right): https://codereview.appspot.com/137920043/diff/80001/Documentation/notation/input.itely#newcode2230 Documentation/notation/input.itely:2230: will then be perfectly equivalent to On 2014/09/06 18:55:42, ...
9 years, 6 months ago (2014-09-06 19:45:54 UTC) #15
Keith
Patch set 5 will probably be fine. My concern was that someone would be confused ...
9 years, 6 months ago (2014-09-07 05:22:02 UTC) #16
dak
On 2014/09/07 05:22:02, Keith wrote: > Patch set 5 will probably be fine. > > ...
9 years, 6 months ago (2014-09-07 05:54:53 UTC) #17
dak
9 years, 6 months ago (2014-09-07 14:37:22 UTC) #18
Aaand back again.  Documentation appears deteriorating but I'm getting tired.
Sign in to reply to this message.

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