Problem

An audit of Together against what it and docs/seating.md say it does. Three promises do not hold. Each was reproduced against 1.x with a throwaway kernel test, and the output is quoted; none is visible in the arrangements the current tests use.

1. A reduction can give up the place the booker chose (major)

"A place the visitor chose on the map is never moved" is the one rule the documentation puts under its own heading, and the reducing section spells it out: "A place the visitor chose goes last of all, after every seat that can be handed back without touching their choice."

giveUpFirst() builds its answer fragment by fragment, and peelOrder() puts a chosen place last within its own fragment. The fragments are then concatenated, so a chosen place in an earlier fragment lands ahead of every seat of every later one.

On a row of twelve: the booker picks position 1 on the map, the stepper gives them 6, 7, 8 and 9, then 2 and 3 beside their pick. Two fragments, 1-2-3 holding their choice and 6-7-8-9. Stepping down by three produces this order:

3, 2, 1 (their own), 9, 8, 7, 6

The three that go back include the seat they chose, while four seats they never asked for stay held.

Two things hide it. The first pass clears whole fragments and refuses one holding a chosen place, so the fault needs a reduction too small to clear either fragment; it is the second pass, over the survivors, that concatenates. And testReductionGivesUpTheChosenPlaceLast holds a single fragment, where within-fragment order is the whole answer.

2. Row distance is ignored exactly when it is needed

nearTheParty() promises "a seat one row back is taken before one twenty rows back". It ranks candidates through rowOrder(), which numbers rows from the available candidates alone. A party whose own row has no free seat therefore has no rank for its own row, every row distance collapses to PHP_INT_MAX, and the tie-break falls through to the seat's position along its row: how far away a row is stops counting at all.

The party's row being full is a common reason for reaching this rung. With a party holding all of row A, one free seat far along row B (the very next row) and one free seat in row L (ten rows back) at the position the party sits at, adding one seat takes row L, and the booker is told "Your places are in the same section, but not all in the same row."

3. A gap inside the party loses to an end below it

beside() says it "takes a gap inside the party before either end of it, which is the one available seat that makes the party more together rather than merely bigger". It walks the row by ascending position and takes the first seat touching the party, so an end at a lower position beats a gap above it. With a party on 2, 3, 5 and 6 of a six-seat row, a hole at 4 and a free end at 1, adding one seat gives:

1, 2, 3, 5, 6

The party ends in two pieces around the hole at 4, where taking 4 would have made 2-3-4-5-6 one run and left the spare seat outside the party. The existing test puts the hole at 3 with room at 6, so the walk meets the hole first and the promise appears to hold.

Proposed resolution

Three ordering changes, each inside the rung that owns it. No rung answers where it did not before, and nothing new is released or moved.

  • Chosen last, over the party rather than the fragment. Hold the chosen anchors back while giveUpFirst() walks the fragments and append them after every unchosen line, so "last of all" is true of the answer instead of each piece of it. A chosen place is still in the answer, because a reduction with nothing else left must still reduce.
  • Rank rows from the places, not the vacancies. rowOrder() should number the rows of the candidates and of the party's own places, so a full row keeps an ordinal and distance from it is measurable.
  • Prefer a gap to an end. beside() should take the seats closing a gap inside the party before those extending it, rather than relying on the order positions happen to arrive in. A gap is a seat with a held place on both sides, which is how the row is already being read.

Remaining tasks

  • The three fixes above, each with kernel coverage seen to fail against the unfixed code first: a chosen place in one fragment with given places in another and a reduction that clears neither; a party whose row is full choosing between two rows at different distances; a gap sitting above a free end.
  • Every existing test must keep passing untouched. Each fault has a sibling test that covers the favorable arrangement, and those are what prove the fixes did not trade one order for another.
  • Whether the unchosen seats of two survivor fragments should interleave rather than concatenate is a separate question, out of scope here, and its own issue if it turns out to be wrong.
  • The class docblock says "Five attempts, in this order" and lists five, but propose() tries six: growFromTheParty() sits between the re-place and near-the-party rungs and is missing. docs/seating.md has it correctly as rung 3, so only the docblock is stale.
  • docs/seating.md where it describes the two ladder promises, and a French translation of anything new, in the same commit.

Performance, audited with nothing to fix here

Read for cost against a hall of 1422 seats. No path scales worse than linearly in the size of the hall, so the documented claim that everything after the one read is arithmetic on that list holds. Three notes, none worth changing on today's numbers:

  • rows() is rebuilt from the whole candidate list up to five times in one propose(). It belongs to the geometry collaborator of #3615214: Split the seating strategy: the row geometry and the rate matching are not one plugin's business and should be folded in there rather than patched here.
  • consolidate() materializes every window in the house before ranking them; ranking by start index and slicing only the winner would avoid the allocations.
  • shareOutByMatching() is bounded by places times slots squared, reachable only for a large group at differently-priced rates on a nearly full hall, and it exits as soon as every slot is filled.

User interface changes

A booker who picked a seat keeps it through any reduction that has another seat to give. A party growing beside itself closes its own gap first. A party whose row is full is offered the nearest row rather than one that happens to line up with it.

Issue fork yoyaku-3615216

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mably created an issue. See original summary.

  • mably committed 1517ff07 on 1.x
    fix: #3615216 The seating strategy breaks three of its own stated...
mably’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.