Fix determine-frets so that it preserves note order (issue4518045)
Reviewers: MikeSol, Message: In response to Mike's request, I've fixed the problem in determine-frets that sometimes changed the order of the notes in the chord. This keeps the glissando between the same notes in the TabStaff as in the Staff. I've made enough changes that I'd like to get a review of the changes. Thanks, Carl Description: Fix determine-frets so that it preserves note order Please review this at http://codereview.appspot.com/4518045/ Affected files: M input/regression/tablature-harmonic.ly M scm/translation-functions.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix determine-frets so that it preserves note order (issue4518045)
Hi Carl, LGTM, apart from some indentation issues. One question for you though: do you think the refactoring is an improvement on the current code? You could for example achieve the same result with the following: (sort string-fret-fingering-tuples (lambda (a b) ( (car a) (car b) ;; end of determine-frets-and-strings Cheers, Neil http://codereview.appspot.com/4518045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix determine-frets so that it preserves note order (issue4518045)
On 5/9/11 12:34 PM, n.putt...@gmail.com n.putt...@gmail.com wrote: Hi Carl, LGTM, apart from some indentation issues. One question for you though: do you think the refactoring is an improvement on the current code? You could for example achieve the same result with the following: (sort string-fret-fingering-tuples (lambda (a b) ( (car a) (car b) ;; end of determine-frets-and-strings There's no guarantee that the notes will be in decreasing string number order. That's the general case, but it's not required. In absolute mode I can write a C major chord as c' e' g' or g' e' c' and as far as I know, we have no requirement that we write chords with the lowest note first. Originally, (before the harmonic became part of the tab note head) we could be order-agnostic, because each head had its own information necessary to place the glyph. But now we aren't order agnostic because of glissandos on each note, and because the harmonic needs to be associated with the proper note. Additionally, there are instruments (at least one, the ukulele) where strings do not increase in pitch, so pitch order (as chords are generally entered in relative mode) doesn't necessarily match string order (which is what the sort call you proposed creates). For these reasons, I think something like what I've done is better than just the sort code you proposed. But I'm open to having the discussion. Thanks for the feedback! Carl Cheers, Neil http://codereview.appspot.com/4518045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: branching
On 04/29/2011 07:15 PM, Carl Sorensen wrote: This might work, but fails to meet the major criterion of the proposed branching scheme. The proposal is to make 2.14 stable. Yes, that's why my proposal was to apply every BUGFIX to 2.14 first, not every patch. :-) (Of course, by bugfix, I mean fix for a bug in 2.14. Bugs in dev only should get fixed in dev only.) Actually, I think your final comment is backwards. Every patch is applicable to dev, but only some are applicable to 2.14. Backwards for patches in general; not for bugfixes. The idea is that bugfixes get applied to 2.14 first and then merged into dev (minimal cherrypicking here); while other patches (new features etc.) get applied to dev and never go near 2.14. dev still gets everything, 2.14 gets just what it needs, and in the best case scenario, you should _never have to cherrypick_, just merge from 2.14 to dev every time there is a bugfix. Is the idea clearer now? By the way, sorry for not following up on this sooner. Busy time in the office. :-( Thanks best wishes, -- Joe ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix determine-frets so that it preserves note order (issue4518045)
On 9 May 2011 19:44, Carl Sorensen c_soren...@byu.edu wrote: There's no guarantee that the notes will be in decreasing string number order. That's the general case, but it's not required. In absolute mode I can write a C major chord as c' e' g' or g' e' c' and as far as I know, we have no requirement that we write chords with the lowest note first. Ah, good point. I hadn't thought of that. :) In that case, it definitely LGTM. Cheers, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix determine-frets so that it preserves note order (issue4518045)
http://codereview.appspot.com/4518045/diff/1/scm/translation-functions.scm File scm/translation-functions.scm (right): http://codereview.appspot.com/4518045/diff/1/scm/translation-functions.scm#newcode280 scm/translation-functions.scm:280: (define free-strings (map 1+ (iota (length tuning could use srfi-1 iota here: (iota (length tuning) 1) http://codereview.appspot.com/4518045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204 lily/completion-note-heads-engraver.cc:204: event-set_property(autosplit-remainder, Moment(left_to_do_ - note_dur.get_length ()).smobbed_copy ()); Wouldn't this work equally well as a boolean? As far as I can see, the Tie_engraver only reads this property, checking whether main_part_ is 0, so this test could be done here instead, e.g., if (Moment (left_to_do_ - note_dur.get_length)).main_part_) event-set_property (autosplit-remainder, SCM_BOOL_T); (obviously this would mean renaming this property) http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc File lily/tie-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode121 lily/tie-engraver.cc:121: Determines whether the end of an event was created by indent http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode168 lily/tie-engraver.cc:168: Make a tie only if pitches are equal or if event end was not generated by indent http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode172 lily/tie-engraver.cc:172: (!Tie_engraver::has_autosplit_end(left_ev))) indent http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode261 lily/tie-engraver.cc:261: */ indent http://codereview.appspot.com/4490045/diff/1/lily/tie-engraver.cc#newcode294 lily/tie-engraver.cc:294: (!Tie_engraver::has_autosplit_end(left_ev))) indent space before (left_ev) http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): http://codereview.appspot.com/4490045/diff/1/lily/completion-note-heads-engraver.cc#newcode204 lily/completion-note-heads-engraver.cc:204: event-set_property(autosplit-remainder, Moment(left_to_do_ - note_dur.get_length ()).smobbed_copy ()); Indeed, using a boolean would be the minimal solution. However, the remaining duration is related to the current time and the overall note duration etc. If for example tuplet splitting was to be implemented for the Completions_heads_engraver, the remainder could be useful in a splitting strategy. It depends on whether minimal data structures or minimal renaming of variables is preferred in lilypond development. I don't know about that ... http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix calculation of vertical offset when 'staff-padding is set (issue4489042)
2011/5/8 Trevor Daniels t.dani...@treda.co.uk lemniskata.bernoull...@gmail.com wrote Sunday, May 08, 2011 7:17 AM Style nitpick. http://codereview.appspot.com/4489042/diff/1/lily/side-position-interface.cc#newcode65 lily/side-position-interface.cc:65: bool include_staff = I see quite a lot of whitespace diffs here and there. IIRC we care about them, so please fix :) If you look more carefully you'll see these changes are actually correcting whitespace errors in the original code. Ooops! Sorry :) I remember that someone once told me that such canges should be done separately from the actual coding stuff - that's why i thought your changes were accidental (i didn't examine them closely indeed). Honestly i'm not sure if this policy of separating style fixes and coding is good. Surely it's better to have these separated, but this requires additional time and work. I found this a little discouraging - style fixes like that are best done 'by the way'. But thanks for taking the trouble to review my fix :) I thought it's worth a try since we have too few reviewers. Too bad that i didn't say anything useful :) cheers, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
