Menu
▾
▴
#3134 Patch: Removes the translate_axis call from axis-group-interface outside-staff positioning.
Accepted
nobody
None
needs_work
Enhancement
2015-10-22
2013-01-22
Anonymous
No
Originally created by: *anonymous
Originally created by: mts...@gmail.com
Originally owned by: mts...@gmail.com
Caches the interior skylines of vertical axis groups and systems.
This will allow for a more modular approach to outside-staff-spacing,
eventually eliminating the call to translate_axis in axis-group-interface.cc.
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes make, make test and a full make doc.
Labels: -Patch-new Patch-review
Originally posted by: mts...@gmail.com
Caches the interior skylines of vertical axis groups and systems.
This will allow for a more modular approach to outside-staff-spacing,
eventually eliminating the call to translate_axis in axis-group-interface.cc.
http://codereview.appspot.com/7185044
Labels: -Patch-review Patch-new
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: Fails Make
Labels: -Patch-new Patch-needs_work
Originally posted by: mts...@gmail.com
Caches the interior skylines of vertical axis groups and systems.
This will allow for a more modular approach to outside-staff-spacing,
eventually eliminating the call to translate_axis in axis-group-interface.cc.
http://codereview.appspot.com/7185044
Labels: -Patch-needs_work Patch-new
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes make, make test and a full make doc.
Labels: -Patch-new Patch-review
Originally posted by: mts...@gmail.com
Ditto - are the comments here up to snuff? It'd be nice to be able to push this as well to continue work on some things.
Originally posted by: dak@gnu.org
Mike, could you update your version of git-cl? Your version apparently is rather old and misses out on a lot of bookkeeping, like setting the correct Status and Owner of the issue. And since you don't bother all too much with setting those fields yourself, it would make things easier if you used a version of git-cl that does a better job.
Owner: mts...@gmail.com
Status: Started
Originally posted by: mts...@gmail.com
(No comment was entered for this change.)
Summary: Patch: Removes the translate_axis call from axis-group-interface outside-staff positioning.
Originally posted by: mts...@gmail.com
Eliminates translate_axis call from outside-staff-positioning.
http://codereview.appspot.com/7185044
Labels: -Patch-review Patch-new
Originally posted by: pkx1...@gmail.com
Passes Make and Make test but reg tests show programming errors.
Specifically:
-programming error: Grob `MeasureCounter' has no interface for property `direction'
-continuing, cross fingers
Labels: -Patch-new Patch-needs_work
Originally posted by: mts...@gmail.com
What is odd about this is that I added side-position-interface to MeasureCounter in the patch, so I'm not sure why it's allergic to the direction property (for other properties it works). Are MeasureCounter's interfaces getting wiped at some point?
Originally posted by: mts...@gmail.com
Even with check-internal-types set, I can't reproduce this error outside of the regtests. Can anyone?
Originally posted by: mts...@gmail.com
Setting the patch to review - I'm pretty sure the patch removes these errors (instead of creating them). There should be minus signs next to the errors. I'm getting these errors with current master but not with the patch...
Labels: -Patch-needs_work Patch-review
Originally posted by: dak@gnu.org
Please don't set the status from Patch-needs_work to Patch-review in any case. If you are sure that something is an error, set it to Patch-new again with an explanation why you do, and wait for your judgment to be corroborated in a renewed test.
Labels: -Patch-review Patch-new
Originally posted by: mts...@gmail.com
Fair 'nuf.
I just pushed a fix to staging for the MeasureCounter grob. It was a bug that wasn't caught because the original regtest had those errors and regtest diffs don't show programming errors in new regtests.
Originally posted by: mts...@gmail.com
Compiles with current master
http://codereview.appspot.com/7185044
Originally posted by: pkx1...@gmail.com
Passes Make and make test but two reg tests show programming errors
-programming error: Grob `MeasureCounter' has no interface for property `direction'
input/regression/measure-counter-broken.log
and
input/regression/measure-counter.log
Labels: -Patch-new Patch-needs_work
Originally posted by: mts...@gmail.com
Many thanks for the test!
If there is a minus sign next to a programming error, that means it takes one away, not adds one.
Also, against current master, there should be no subtracting or adding of programming errors (assuming [r069117e9315c9d8e77bf5683e4c49aaf207e6d45] is out of staging and into master, which I believe it is).
So I'm setting it to patch new.
Labels: -Patch-needs_work Patch-new
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes make, make test and a full make doc.
Labels: -Patch-new Patch-review
Originally posted by: dak@gnu.org
Regarding comment #18: given the large amount of work James is investing into reviews, it is quite a waste of important resources that nobody bothered to tell him (or anybody else) how to actually read a unified diff. In Documentation/contributor/regressions.itexi, we have
@item
Log files: show the difference in command-line output.
The main thing to examine are any changes in page counts -- if a
file used to fit on 1 page but now requires 4 or 5 pages,
something is suspicious!
That's all. No mention of how different kinds of changes are flagged. No mention of warnings and programming errors and what their respective coming and going means, and how either is signalled.
Now I am somewhat doubtful that every regtest performer will read the job description totally closely (at least I tend to invent most of my instructions as I go), but that's the natural starting point, and we need to do better than that.
Originally posted by: pkx1...@gmail.com
I am ok with Unified diffs, what has obviously thrown me is that the fact that I am seeing an error message that has the '-' sign next to it.
While that does mean the line has been removed, what I cannot understand is that make check is comparing the reg tests (i.e. a known good with a current patched one), so how can an error message be a '-' line in the diff? As doesn't this imply that the error message was *always* there during any make check.
Therefore my assumption has been that any warning/error should always be 'new' and so relevant to the *new* patch.
How can a new patch remove a line in a log that was never there to start with ( If you get my meaning)?
Originally posted by: mts...@gmail.com
The line (or at least the lack of an interface) was there to start with. When the measure-counter.ly regtest became part of LilyPond, these programming errors were there. So the line was (I think) always in the log.
Originally posted by: dak@gnu.org
Regarding comment #21: at the time a regtest gets added, the diff does _not_ include the regtest (what should it be compared with, anyhow?). So any warning that has been there for the whole lifetime of a regtest will never have appeared in a diff.
So it is more or less the responsibility of the regtest submitter to check _manually_ that the regtest compiles without warning.
This appears like a hole in our testing structure. At one point of time, we used "warning-as-errors", I think. For some reason, that practice had been stopped. And "programming errors" don't count as warning, anyway.
Originally posted by: mts...@gmail.com
Incorporates David's suggestions
http://codereview.appspot.com/7185044
Labels: -Patch-review Patch-new
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes make, make test and a full make doc.
Reg test diffs - these did show up before but I thought I'd include them this time, just subtle differences
Labels: -Patch-new Patch-review