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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by hahnjo
Modified:
1 week, 4 days ago
Reviewers:
lilypond-pkx, Dan Eble, Carl, carl.d.sorensen
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

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

Messages

Total messages: 4
Dan Eble
THANK YOU I applied this on top of master, ran `make test-baseline` and then `make ...
1 week, 5 days ago (2019-11-05 19:23:40 UTC) #1
Carl
LGTM. Nice work! Carl
1 week, 5 days ago (2019-11-05 19:33:21 UTC) #2
lilypond-pkx
Interestingly (or not) I did 3 patch runs on this patch and 2 out of ...
1 week, 4 days ago (2019-11-06 09:33:45 UTC) #3
hahnjo
1 week, 4 days 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