|
|
Created:
9 years, 7 months ago by dak Modified:
9 years, 5 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionIssue 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. #
MessagesTotal messages: 18
Fix typo, add regtest
Sign in to reply to this message.
LGTM It worked fine on (adapted) examples from the divisi staff issue and emails. Looking at the code, I was confused how it worked without the \cleansed concept of the LSR entries, until I realized un-grouped tags effectively form their own group, so old uses of \keepWithTag ignore tags in the new groups. https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/in... File Documentation/notation/input.itely (right): https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/in... Documentation/notation/input.itely:2246: @code{\keepWithTag} is not used. The last paragraph is difficult to understand; maybe leave it out. https://codereview.appspot.com/137920043/diff/20001/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/137920043/diff/20001/ly/music-functions-init.l... ly/music-functions-init.ly:562: may be either a single symbol or a list of symbols.") You could make the whole tag-group enhancement parenthetical "...or untagged (ignoring any tags in tag groups different from the tag groups containing @var{tag}; see @code{tagGroup})." It will probably help understanding to say "\keepWithTag A.B performs \removeWithTag C.D.E... where C,D,E,etc., are all the tags other than A.B in the tag group(s) that contain A and B. https://codereview.appspot.com/137920043/diff/20001/scm/music-functions.scm File scm/music-functions.scm (right): https://codereview.appspot.com/137920043/diff/20001/scm/music-functions.scm#n... scm/music-functions.scm:2650: (not (any (lambda (t) (eq? (tag-group-get t) group)) music-tags)))))) This last line has some non-obvious important details. ; If this music has non-matching tags, and if any of them are in the same group as the tag we are keeping, then we do *not* keep this music. The set of tags outside any declared group, for which tag-group-get returns #f, are treated as their own tagGroup https://codereview.appspot.com/137920043/diff/20001/scm/music-functions.scm#n... scm/music-functions.scm:2656: (any (lambda (t) (memq t tags)) music-tags) I didn't understand the comment below until after figuring out the code (and then re-interpreting the comment until I found an interpretation that fit). ; This music has tags that do not match any of our tags to keep. If any of the music's tags are in the same group as any of our tags to keep, then we do *not* keep this music.
Sign in to reply to this message.
On 2014/09/05 06:59:18, Keith wrote: > LGTM > It worked fine on (adapted) examples from the divisi staff issue and emails. > Looking at the code, I was confused how it worked without the \cleansed concept > of the LSR entries, until I realized un-grouped tags effectively form their own > group, so old uses of \keepWithTag ignore tags in the new groups. [some detailed criticism about documentation and code comments] It is probably helpful to realize that both documentation and code have been written with a level of understanding exhibited in <URL:https://code.google.com/p/lilypond/issues/detail?id=4083#c14> that I had to backpaddle on in <URL:https://code.google.com/p/lilypond/issues/detail?id=4083#c20>. So both the user level documentation as well as the code comments describe the *implementation* without a proper understanding of the resulting *semantics*. And the fundamental semantics are that \keepWithTags works independently on different tag groups, whether one collects them in one \keepWithTags command or not. The implementation of the predicate could be done by splitting in separate tag groups and then iterating through them, allowing each of them to vote "no" for keeping music. The current implementation is more efficient I think but that's more an accident than by design. So it would probably be a good idea to rewrite the documentation/codecomments from scratch. It's just sort of drudge work to start over.
Sign in to reply to this message.
https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/in... File Documentation/notation/input.itely (left): https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/in... Documentation/notation/input.itely:2075: @funindex \appendToTag Why are these index lines removed? The functions are still documented in this section.
Sign in to reply to this message.
mark.opus11@googlemail.com writes: > https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/in... > File Documentation/notation/input.itely (left): > > https://codereview.appspot.com/137920043/diff/20001/Documentation/notation/in... > Documentation/notation/input.itely:2075: @funindex \appendToTag > Why are these index lines removed? The functions are still documented in > this section. They are not removed. They are moved down to the part where the corresponding commands are actually explained. Arguably it would make sense to split this section into more parts but in any case it does not make sense to let index entries be off-mark by what amounts to several pages and topics. > https://codereview.appspot.com/137920043/ -- David Kastrup
Sign in to reply to this message.
Reimplement and redocument according to Keith's comments and my own insights
Sign in to reply to this message.
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.l... ly/music-functions-init.ly:562: more than one tag group defined with @code{\\tagGroup}, there must be Tags cannot be members of more than one tag group ??
Sign in to reply to this message.
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.l... ly/music-functions-init.ly:562: more than one tag group defined with @code{\\tagGroup}, there must be On 2014/09/05 19:03:28, Trevor Daniels wrote: > Tags cannot be members of more than one tag group ?? One tag cannot, multiple tags can. How about renaming @var{tag} to @var{tags} and then writing "In each of those tag groups with members in both @var{tags} and the tags on some music, at least one matching tag is needed in order to have the music retained."
Sign in to reply to this message.
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.l... ly/music-functions-init.ly:562: more than one tag group defined with @code{\\tagGroup}, there must be On 2014/09/05 20:59:56, dak wrote: > On 2014/09/05 19:03:28, Trevor Daniels wrote: > > Tags cannot be members of more than one tag group ?? > > One tag cannot, multiple tags can. How about renaming @var{tag} to @var{tags} OK, I see now why I misunderstood. What I'd missed was: @var(tag) can be a list of tags and those tags may separately be members of different tag groups. It's obvious now I see it, of course. I think specifying @var(tags) would help, as would including after "symbols" something like, "Although each individual tag in @var{tags} must be a member of only a single tag group, different tags may be members of different tag groups. If this is the case, there must be at least one matched tag in every matched tag group for a tagged music expression to be retained."
Sign in to reply to this message.
Try incorporating Trevor's suggestions in the most accurate manner
Sign in to reply to this message.
Fix tag/tags confusion in doc strings
Sign in to reply to this message.
LGTM. Thanks David. Trevor
Sign in to reply to this message.
The behavior of the old patch seemed better, in the case where someone does combine a \keepWithTags A.C with A and C from different groups. In that case, the user knows about both tag-groups and might be thinking of them as a unified group, and expect that command to keep music tagged \tag#'A \tag#'Q, regardless of the group membership of Q. The rule "\keepWithTag will keep music if any tag matches" has existed in the past. That is, these two examples, similar to a pair you put in the tracker, \keepWithTag A.B \tag B.C \new Staff c'^\markup"AB_BC" \keepWithTag A.B \tag B { \tag C \new Staff c'^\markup"AB_B_C" } are treated differently in current master. The old patch allowed a description that lets us understand the basics without understanding tagGroups. \keepWithTag will keep music if any tag matches, removing music with unmatching tags, but ignoring tags in a different tagGroup from any of the tags to keep. https://codereview.appspot.com/137920043/diff/80001/Documentation/notation/in... File Documentation/notation/input.itely (right): https://codereview.appspot.com/137920043/diff/80001/Documentation/notation/in... Documentation/notation/input.itely:2230: will then be perfectly equivalent to This is not quite true, and I think you don't want it to be true, if there is music \tag#'violinI \tag#'violinII \clef french
Sign in to reply to this message.
On 2014/09/06 18:55:42, Keith wrote: > The behavior of the old patch seemed better, in the case where someone does > combine a \keepWithTags A.C with A and C from different groups. In that case, > the user knows about both tag-groups and might be thinking of them as a unified > group, and expect that command to keep music tagged \tag#'A \tag#'Q, regardless > of the group membership of Q. > > The rule "\keepWithTag will keep music if any tag matches" has existed in the > past. That is, these two examples, similar to a pair you put in the tracker, > \keepWithTag A.B \tag B.C \new Staff c'^\markup"AB_BC" > \keepWithTag A.B \tag B { \tag C \new Staff c'^\markup"AB_B_C" } > are treated differently in current master. I disagree here. Point #1 to note is that symmetric behavior makes stuff easier to explain and understand. With the currently proposed implementation, like with the previous implementation which you argue for, exchanging the tags on the \keepWithTag command with those on the music itself does not change whether a match occurs. Now an essential feature of tag groups is that uses of different tag groups is supposed to be orthogonal. That means that operations based on tags will not be affected in any manner when adding tags from a different tag group. That is, \tag A { \tag B { ... } } should not in any manner behave differently from \tag A \tag B { ... } whenever A and B are from different tag groups with regard to whether or not ... will get *removed*, because removing one will effectively remove the other and vice versa either way. But _if_ we stipulate a symmetric relation between tags in \keepWithTags and on some music, that means that \keepWithTag '(A B) (with A and B in different tag groups) should also keep music only if either A or B matches. Now as opposed to the tags on music (which can be delivered by separate commands), tags from different tag groups will not creep into the same \keepWithTag command accidentally. So there is no similarly strong argument against implementing a different behavior here. However, it would come at the cost of sacrificing symmetry in the relation between \keepWithTags command and multiple tags on music. Since with the new behavior, \keepWithTag A \keepWithTag B is identical to \keepWithTag A.B when tags A and B are in different tag groups, the previous behavior offered _more_ functionality. I just don't see that it offered _relevant_ additional functionality that would offset the loss of symmetry and/or the independency of tags from different tag groups. Feel free to convince me otherwise with an appropriate use case. > The old patch allowed a description that lets us understand the basics without > understanding tagGroups. > \keepWithTag will keep music if any tag matches, > removing music with unmatching tags, > but ignoring tags in a different tagGroup from any of the tags to keep. But "if any tag matches" becomes very tricky to explain when one needs to implement a behavior where \tag A { \tag B ... is supposed to be equivalent to \tag A \tag B ... while \keepWithTag A \keepWithTag B is different from \keepWithTag A.B even when A and B are in different tag groups. What do you want to sacrifice? The equivalence of \tag A \tag B ... with \tag A { \tag B ... when A and B are from different tag groups, or the equivalence of "\keepWithTag tag list matches music tag list" with "music tag list matches \keepWithTag tag list"? You don't get to keep both.
Sign in to reply to this message.
https://codereview.appspot.com/137920043/diff/80001/Documentation/notation/in... File Documentation/notation/input.itely (right): https://codereview.appspot.com/137920043/diff/80001/Documentation/notation/in... Documentation/notation/input.itely:2230: will then be perfectly equivalent to On 2014/09/06 18:55:42, Keith wrote: > This is not quite true, and I think you don't want it to be true, if there is > music > \tag#'violinI \tag#'violinII \clef french You are absolutely correct. I need to rewrite this passage.
Sign in to reply to this message.
Patch set 5 will probably be fine. My concern was that someone would be confused that adding to the list of tags to keep \keepWithTag editorial { ... \tag#'editorial \tag#'score \bar'||' } \keepWithTag part.editorial { ... \tag#'editorial \tag#'score \bar'||' } can reduce the things that are kept. But that will not likely come up, and you do explain it in the docstring for \keepWithTag Don't put too much value on symmetry between how the tags in \keepWithTag, and the tags on each given piece of music enter, into the keep-or-remove decision. One set defines a single filter, the other describes each item that is going through the filter. The keep-or-remove decision is part of an asymmetric operation. On Sat, 06 Sep 2014 12:45:40 -0700, <dak@gnu.org> wrote: > On 2014/09/06 18:55:42, Keith wrote: >> The old patch : >> \keepWithTag will keep music if any tag matches, >> removing music with unmatching tags, >> but ignoring tags in a different tagGroupfrom any of the tags to keep. > > But "if any tag matches" becomes very tricky to explain when one needs > to implement a behavior where \tag A { \tag B ... is supposed to be > equivalent to \tag A \tag B ... while \keepWithTag A \keepWithTag B is > different from \keepWithTag A.B even when A and B are in different tag > groups. The explanation above does cover the points you think might be tricky. \tag A { \tag B ... has not to date been equivalent to \tag A \tag B ... The explanation above says when B would be "ignored" by keepWithTag. If A is ignored, or if B is ignored, the expressions just above are equivalent. \keepWithTag A.B might keep more music than \keepWithTag A \keepWithTag B. The explanation above seems to make that clear, because it is easier to match "any tag" in A and B than to match each of A and B.
Sign in to reply to this message.
On 2014/09/07 05:22:02, Keith wrote: > Patch set 5 will probably be fine. > > My concern was that someone would be confused that adding to the list of tags to > keep > \keepWithTag editorial { ... \tag#'editorial \tag#'score \bar'||' } > \keepWithTag part.editorial { ... \tag#'editorial \tag#'score \bar'||' } > can reduce the things that are kept. > But that will not likely come up, and you > do explain it in the docstring for \keepWithTag After sleeping over it I have to admit that my rationale does have a hole. > \tag A { \tag B ... has not to date been equivalent to \tag A \tag B ... That's perfectly correct but to date there has not been the ability to use tag groups for independent applications. Now the salient point I have been missing in my argument is that when using \keepWithTag on tags A and B from different tag groups, \tag A { \tag B *is* equivalent to \tag A \tag B as long as \keepWithTag is never called mentioning more than one tag group in its argument. > Don't put too much value on symmetry between how the tags in > \keepWithTag, and the tags on each given piece of music enter, into > the keep-or-remove decision. One set defines a single filter, the > other describes each item that is going through the filter. The > keep-or-remove decision is part of an asymmetric operation. The operation so far has been symmetric and I consider that a good idea. Tags from different tag groups used in different applications can combine by coincidence and I don't want that to lead to interactions. But tags in different \keepWithTag commands cannot combine by coincidence, only intentionally. And it turns out that without \keepWithTag commands combining by coincidence, the coincidal joinder of unreleated \tag commands from unrelated tag groups is not enough to cause surprises. So my argument does not carry weight. The semantics we are talking about are for the case where a user explicitly juggles multiple tag groups in a single application: this cannot happen coincidentally. In a nutshell, what we are talking about is the behavior of \tagGroup vI.vII \tagGroup obI.obII \keepWithTag vI.obI \tag vI.obII ... If we don't have a strong reason to prefer one behavior over the other here, then it is noteworthy that "remove" can be achieved as end result by \keepWithTag vI \keepWithTag obI in this case. Also the explanation might be a bit more straightforward. So I am leaning towards flipping back again. Sigh.
Sign in to reply to this message.
|