@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Disperse task subpriorities in blocks
Summary:
Ref T7664. The current algorithm for moving task subpriorities can end up stuck in a real sticky swamp in some unusual situations.
Instead, use an algorithm which works like this:
- When we notice two tasks are too close together, look at the area around those tasks (just a few paces).
- If things look pretty empty, we can just spread the tasks out a little bit.
- But, if things are still real crowded, take another look further.
- Keep doing that until we're looking at a real nice big spot which doesn't have too many tasks in it in total, even if they're all in one place right now.
- Then, move 'em out!
Also:
- Just swallow our pride and do the gross `INSERT INTO ... "", "", "", "", "", "", ... ON DUPLICATE KEY UPDATE` to bulk update.
- Fix an issue where a single move could cause two different subpriority recalculations.
Test Plan:
- Changed `ManiphesTaskTestCase->testTaskAdjacentBlocks()` to insert 1,000 tasks with identical subpriorities, saw them spread out in 11 queries instead of >1,000.
- Dragged tons of tasks around on workboards.
- Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7664
Differential Revision: https://secure.phabricator.com/D17959
+193
-86
+28
src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
+28
src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
···
125
125
9,
126
126
count($subpri),
127
127
pht('Expected subpriorities to be distributed.'));
128
+
129
+
// Move task 9 to the end.
130
+
$this->moveTask($viewer, $t[9], $t[1], true);
131
+
$tasks = $this->loadTasks($viewer, $auto_base);
132
+
$this->assertEqual(
133
+
array(8, 7, 6, 5, 4, 3, 2, 1, 9),
134
+
array_keys($tasks));
135
+
136
+
// Move task 3 to the beginning.
137
+
$this->moveTask($viewer, $t[3], $t[8], false);
138
+
$tasks = $this->loadTasks($viewer, $auto_base);
139
+
$this->assertEqual(
140
+
array(3, 8, 7, 6, 5, 4, 2, 1, 9),
141
+
array_keys($tasks));
142
+
143
+
// Move task 3 to the end.
144
+
$this->moveTask($viewer, $t[3], $t[9], true);
145
+
$tasks = $this->loadTasks($viewer, $auto_base);
146
+
$this->assertEqual(
147
+
array(8, 7, 6, 5, 4, 2, 1, 9, 3),
148
+
array_keys($tasks));
149
+
150
+
// Move task 5 to before task 4 (this is its current position).
151
+
$this->moveTask($viewer, $t[5], $t[4], false);
152
+
$tasks = $this->loadTasks($viewer, $auto_base);
153
+
$this->assertEqual(
154
+
array(8, 7, 6, 5, 4, 2, 1, 9, 3),
155
+
array_keys($tasks));
128
156
}
129
157
130
158
private function newTask(PhabricatorUser $viewer, $title) {
+162
-64
src/applications/maniphest/editor/ManiphestTransactionEditor.php
+162
-64
src/applications/maniphest/editor/ManiphestTransactionEditor.php
···
384
384
*/
385
385
public static function getAdjacentSubpriority(
386
386
ManiphestTask $dst,
387
-
$is_after,
388
-
$allow_recursion = true) {
387
+
$is_after) {
389
388
390
389
$query = id(new ManiphestTaskQuery())
391
390
->setViewer(PhabricatorUser::getOmnipotentUser())
···
407
406
// If we find an adjacent task, we average the two subpriorities and
408
407
// return the result.
409
408
if ($adjacent) {
410
-
$epsilon = 0.01;
409
+
$epsilon = 1.0;
411
410
412
411
// If the adjacent task has a subpriority that is identical or very
413
-
// close to the task we're looking at, we're going to move it and all
414
-
// tasks with the same subpriority a little farther down the subpriority
415
-
// scale.
416
-
if ($allow_recursion &&
417
-
(abs($adjacent->getSubpriority() - $base) < $epsilon)) {
418
-
$conn_w = $adjacent->establishConnection('w');
412
+
// close to the task we're looking at, we're going to spread out all
413
+
// the nearby tasks.
419
414
420
-
$min = ($adjacent->getSubpriority() - ($epsilon));
421
-
$max = ($adjacent->getSubpriority() + ($epsilon));
415
+
$adjacent_sub = $adjacent->getSubpriority();
416
+
if ((abs($adjacent_sub - $base) < $epsilon)) {
417
+
$base = self::disperseBlock(
418
+
$dst,
419
+
$epsilon * 2);
420
+
if ($is_after) {
421
+
$sub = $base - $epsilon;
422
+
} else {
423
+
$sub = $base + $epsilon;
424
+
}
425
+
} else {
426
+
$sub = ($adjacent_sub + $base) / 2;
427
+
}
428
+
} else {
429
+
// Otherwise, we take a step away from the target's subpriority and
430
+
// use that.
431
+
if ($is_after) {
432
+
$sub = ($base - $step);
433
+
} else {
434
+
$sub = ($base + $step);
435
+
}
436
+
}
422
437
423
-
// Get all of the tasks with the similar subpriorities to the adjacent
424
-
// task, including the adjacent task itself.
425
-
$query = id(new ManiphestTaskQuery())
426
-
->setViewer(PhabricatorUser::getOmnipotentUser())
427
-
->withPriorities(array($adjacent->getPriority()))
428
-
->withSubpriorityBetween($min, $max);
438
+
return array($dst->getPriority(), $sub);
439
+
}
429
440
430
-
if (!$is_after) {
431
-
$query->setOrderVector(array('-priority', '-subpriority', '-id'));
432
-
} else {
433
-
$query->setOrderVector(array('priority', 'subpriority', 'id'));
434
-
}
441
+
/**
442
+
* Distribute a cluster of tasks with similar subpriorities.
443
+
*/
444
+
private static function disperseBlock(
445
+
ManiphestTask $task,
446
+
$spacing) {
435
447
436
-
$shift_all = $query->execute();
437
-
$shift_last = last($shift_all);
448
+
$conn = $task->establishConnection('w');
438
449
439
-
// Select the most extreme subpriority in the result set as the
440
-
// base value.
441
-
$shift_base = head($shift_all)->getSubpriority();
450
+
// Find a block of subpriority space which is, on average, sparse enough
451
+
// to hold all the tasks that are inside it with a reasonable level of
452
+
// separation between them.
442
453
443
-
// Find the subpriority before or after the task at the end of the
444
-
// block.
445
-
list($shift_pri, $shift_sub) = self::getAdjacentSubpriority(
446
-
$shift_last,
447
-
$is_after,
448
-
$allow_recursion = false);
454
+
// We'll start by looking near the target task for a range of numbers
455
+
// which has more space available than tasks. For example, if the target
456
+
// task has subpriority 33 and we want to separate each task by at least 1,
457
+
// we might start by looking in the range [23, 43].
449
458
450
-
$delta = ($shift_sub - $shift_base);
451
-
$count = count($shift_all);
459
+
// If we find fewer than 20 tasks there, we have room to reassign them
460
+
// with the desired level of separation. We space them out, then we're
461
+
// done.
452
462
453
-
$shift = array();
454
-
$cursor = 1;
455
-
foreach ($shift_all as $shift_task) {
456
-
$shift_target = $shift_base + (($cursor / $count) * $delta);
457
-
$cursor++;
463
+
// However: if we find more than 20 tasks, we don't have enough room to
464
+
// distribute them. We'll widen our search and look in a bigger range,
465
+
// maybe [13, 53]. This range has more space, so if we find fewer than
466
+
// 40 tasks in this range we can spread them out. If we still find too
467
+
// many tasks, we keep widening the search.
458
468
459
-
queryfx(
460
-
$conn_w,
461
-
'UPDATE %T SET subpriority = %f WHERE id = %d',
462
-
$adjacent->getTableName(),
463
-
$shift_target,
464
-
$shift_task->getID());
469
+
$base = $task->getSubpriority();
465
470
466
-
// If we're shifting the adjacent task, update it.
467
-
if ($shift_task->getID() == $adjacent->getID()) {
468
-
$adjacent->setSubpriority($shift_target);
469
-
}
471
+
$scale = 4.0;
472
+
while (true) {
473
+
$range = ($spacing * $scale) / 2.0;
474
+
$min = ($base - $range);
475
+
$max = ($base + $range);
470
476
471
-
// If we're shifting the original target task, update the base
472
-
// subpriority.
473
-
if ($shift_task->getID() == $dst->getID()) {
474
-
$base = $shift_target;
475
-
}
476
-
}
477
+
$result = queryfx_one(
478
+
$conn,
479
+
'SELECT COUNT(*) N FROM %T WHERE priority = %d AND
480
+
subpriority BETWEEN %f AND %f',
481
+
$task->getTableName(),
482
+
$task->getPriority(),
483
+
$min,
484
+
$max);
485
+
486
+
$count = $result['N'];
487
+
if ($count < $scale) {
488
+
// We have found a block which we can make sparse enough, so bail and
489
+
// continue below with our selection.
490
+
break;
477
491
}
478
492
479
-
$sub = ($adjacent->getSubpriority() + $base) / 2;
493
+
// This block had too many tasks for its size, so try again with a
494
+
// bigger block.
495
+
$scale *= 2.0;
496
+
}
497
+
498
+
$rows = queryfx_all(
499
+
$conn,
500
+
'SELECT id FROM %T WHERE priority = %d AND
501
+
subpriority BETWEEN %f AND %f
502
+
ORDER BY priority, subpriority, id',
503
+
$task->getTableName(),
504
+
$task->getPriority(),
505
+
$min,
506
+
$max);
507
+
508
+
$task_id = $task->getID();
509
+
$result = null;
510
+
511
+
// NOTE: In strict mode (which we encourage enabling) we can't structure
512
+
// this bulk update as an "INSERT ... ON DUPLICATE KEY UPDATE" unless we
513
+
// provide default values for ALL of the columns that don't have defaults.
514
+
515
+
// This is gross, but we may be moving enough rows that individual
516
+
// queries are unreasonably slow. An alternate construction which might
517
+
// be worth evaluating is to use "CASE". Another approach is to disable
518
+
// strict mode for this query.
519
+
520
+
$extra_columns = array(
521
+
'phid' => '""',
522
+
'authorPHID' => '""',
523
+
'status' => '""',
524
+
'priority' => 0,
525
+
'title' => '""',
526
+
'originalTitle' => '""',
527
+
'description' => '""',
528
+
'dateCreated' => 0,
529
+
'dateModified' => 0,
530
+
'mailKey' => '""',
531
+
'viewPolicy' => '""',
532
+
'editPolicy' => '""',
533
+
'ownerOrdering' => '""',
534
+
'spacePHID' => '""',
535
+
'bridgedObjectPHID' => '""',
536
+
'properties' => '""',
537
+
'points' => 0,
538
+
'subtype' => '""',
539
+
);
540
+
541
+
$defaults = implode(', ', $extra_columns);
542
+
543
+
$sql = array();
544
+
$offset = 0;
545
+
546
+
// Often, we'll have more room than we need in the range. Distribute the
547
+
// tasks evenly over the whole range so that we're less likely to end up
548
+
// with tasks spaced exactly the minimum distance apart, which may
549
+
// get shifted again later. We have one fewer space to distribute than we
550
+
// have tasks.
551
+
$divisor = (double)(count($rows) - 1.0);
552
+
if ($divisor > 0) {
553
+
$available_distance = (($max - $min) / $divisor);
480
554
} else {
481
-
// Otherwise, we take a step away from the target's subpriority and
482
-
// use that.
483
-
if ($is_after) {
484
-
$sub = ($base - $step);
485
-
} else {
486
-
$sub = ($base + $step);
555
+
$available_distance = 0.0;
556
+
}
557
+
558
+
foreach ($rows as $row) {
559
+
$subpriority = $min + ($offset * $available_distance);
560
+
561
+
// If this is the task that we're spreading out relative to, keep track
562
+
// of where it is ending up so we can return the new subpriority.
563
+
$id = $row['id'];
564
+
if ($id == $task_id) {
565
+
$result = $subpriority;
487
566
}
567
+
568
+
$sql[] = qsprintf(
569
+
$conn,
570
+
'(%d, %Q, %f)',
571
+
$id,
572
+
$defaults,
573
+
$subpriority);
574
+
575
+
$offset++;
488
576
}
489
577
490
-
return array($dst->getPriority(), $sub);
578
+
foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
579
+
queryfx(
580
+
$conn,
581
+
'INSERT INTO %T (id, %Q, subpriority) VALUES %Q
582
+
ON DUPLICATE KEY UPDATE subpriority = VALUES(subpriority)',
583
+
$task->getTableName(),
584
+
implode(', ', array_keys($extra_columns)),
585
+
$chunk);
586
+
}
587
+
588
+
return $result;
491
589
}
492
590
493
591
protected function validateAllTransactions(
-22
src/applications/maniphest/query/ManiphestTaskQuery.php
-22
src/applications/maniphest/query/ManiphestTaskQuery.php
···
17
17
private $dateCreatedBefore;
18
18
private $dateModifiedAfter;
19
19
private $dateModifiedBefore;
20
-
private $subpriorityMin;
21
-
private $subpriorityMax;
22
20
private $bridgedObjectPHIDs;
23
21
private $hasOpenParents;
24
22
private $hasOpenSubtasks;
···
109
107
110
108
public function withSubpriorities(array $subpriorities) {
111
109
$this->subpriorities = $subpriorities;
112
-
return $this;
113
-
}
114
-
115
-
public function withSubpriorityBetween($min, $max) {
116
-
$this->subpriorityMin = $min;
117
-
$this->subpriorityMax = $max;
118
110
return $this;
119
111
}
120
112
···
406
398
$conn,
407
399
'task.subpriority IN (%Lf)',
408
400
$this->subpriorities);
409
-
}
410
-
411
-
if ($this->subpriorityMin !== null) {
412
-
$where[] = qsprintf(
413
-
$conn,
414
-
'task.subpriority >= %f',
415
-
$this->subpriorityMin);
416
-
}
417
-
418
-
if ($this->subpriorityMax !== null) {
419
-
$where[] = qsprintf(
420
-
$conn,
421
-
'task.subpriority <= %f',
422
-
$this->subpriorityMax);
423
401
}
424
402
425
403
if ($this->bridgedObjectPHIDs !== null) {
+3
src/applications/project/controller/PhabricatorProjectMoveController.php
+3
src/applications/project/controller/PhabricatorProjectMoveController.php