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

Issue 554960043: Issue 5217: Fix sorting order without outside-staff-priority (Closed)

Can't Edit
Can't Publish+Mail
Start Review
7 months ago by hahnjo
5 months ago
lilypond-pkx, Dan Eble, Carl, carl.d.sorensen


Issue 5217: Fix sorting order without outside-staff-priority If the two Grobs have no outside-staff-priority, the compare function staff_priority_less() would relate the two pointers. This may lead to changing sorting orders in subsequent runs, apparently resulting in "random" positions in the regression tests rest-dot-position.ly and sometimes merge-rests-engraver.ly. Solve this by keeping the original order in the vector: * Mark two Grobs without outside-staff-priority as being equal by always returning false (none is less than the other), and * use vector_stable_sort() to keep equal items in their relation.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -5 lines) Patch
M lily/axis-group-interface.cc View 2 chunks +5 lines, -5 lines 0 comments Download


Total messages: 4
Dan Eble
THANK YOU I applied this on top of master, ran `make test-baseline` and then `make ...
7 months ago (2019-11-05 19:23:40 UTC) #1
LGTM. Nice work! Carl
7 months ago (2019-11-05 19:33:21 UTC) #2
Interestingly (or not) I did 3 patch runs on this patch and 2 out of ...
7 months ago (2019-11-06 09:33:45 UTC) #3
7 months ago (2019-11-06 09:36:39 UTC) #4
On 2019/11/06 09:33:45, lilypond-pkx wrote:
> Interestingly (or not) I did 3 patch runs on this patch and 2 out of 3 times I
> still got the reg test showing.
> I have some time today so I'll see if I can spot anything and  update
> accordingly.
> Jams

Compared to which baseline, run with master or with this patch already applied?
If with master, you'll have the random positions in the baseline...
If with this patch applied, I'll try later if I can reproduce on my system.
Sign in to reply to this message.

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