fix: Advanced Search results came back in a different order every search (#2626)
## Summary Advanced Search results came back in a different order every search, whether or not a sort was chosen. Follow-up to #2625, where it was noticed while testing the property test. ## Why Results arrive from the search workers in whatever order they finished. Each sort the gump offers compares a single key and `Array.Sort` is not stable, so equal keys, which is most rows under type or map, landed in a different order every run; with no sort chosen the list was arrival order outright. The range sort also answered 0 for any two results off the viewer's map, so those shuffled as well. ## What changed - The five comparers share one base class whose `Compare` applies the chosen key and then a fixed tie-break, serial ascending regardless of direction. The direction is about the key; a fixed order among equal keys is what keeps two searches identical. - The collected result list is put in serial order before the gump sees it, so an unsorted search is deterministic too. That sort runs on the thread-pool work item that already assembles the list and reads only the result records. - No change to which key each sort uses or to what ascending and descending mean. ## Test plan - [x] New `AdvancedSearchResultOrderTests`: six equal-keyed results offered in two arrival orders to every comparer in both directions, asserting the same sequence and serial order; the range comparer on off-map results; reverse flips the key but not the tie-break. - [x] `UOContent.Tests`: 873 passed, 0 failed - [x] `Server.Tests`: 869 passed, 0 failed - [ ] In game: run the same search twice with each sort and with none; the lists match.
This commit is contained in:
Kamron Batman
GitHub
parent
535a098996
commit
f7caff8ad3
3 changed files with 240 additions and 79 deletions
|
|
@ -0,0 +1,132 @@
|
|||
using System;
|
||||
using System.Collections.Generic;
|
||||
using Server;
|
||||
using Server.Engines.AdvancedSearch;
|
||||
using Server.Items;
|
||||
using Xunit;
|
||||
|
||||
namespace UOContent.Tests;
|
||||
|
||||
// Every sort must give the same sequence from any arrival order: key first, then serial.
|
||||
[Collection("Sequential UOContent Tests")]
|
||||
public class AdvancedSearchResultOrderTests : IDisposable
|
||||
{
|
||||
private readonly List<Item> _items = [];
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
for (var i = 0; i < _items.Count; i++)
|
||||
{
|
||||
_items[i].Delete();
|
||||
}
|
||||
|
||||
_items.Clear();
|
||||
}
|
||||
|
||||
private AdvancedSearchResult Result(Item item, string name = "same", Map map = null)
|
||||
{
|
||||
_items.Add(item);
|
||||
return new AdvancedSearchResult(name, item.GetType(), item.Location, map ?? Map.Felucca, null) { Entity = item };
|
||||
}
|
||||
|
||||
private AdvancedSearchResult[] EqualKeyed()
|
||||
{
|
||||
var results = new AdvancedSearchResult[6];
|
||||
|
||||
for (var i = 0; i < results.Length; i++)
|
||||
{
|
||||
results[i] = Result(new Item(0x1));
|
||||
}
|
||||
|
||||
return results;
|
||||
}
|
||||
|
||||
private static AdvancedSearchResult[] Shuffled(AdvancedSearchResult[] inOrder, params int[] permutation)
|
||||
{
|
||||
var shuffled = new AdvancedSearchResult[inOrder.Length];
|
||||
|
||||
for (var i = 0; i < permutation.Length; i++)
|
||||
{
|
||||
shuffled[i] = inOrder[permutation[i]];
|
||||
}
|
||||
|
||||
return shuffled;
|
||||
}
|
||||
|
||||
private static void AssertSameSequenceFromAnyArrival(
|
||||
AdvancedSearchResult[] inOrder,
|
||||
IComparer<AdvancedSearchResult> comparer
|
||||
)
|
||||
{
|
||||
var first = Shuffled(inOrder, 3, 0, 5, 1, 4, 2);
|
||||
var second = Shuffled(inOrder, 5, 4, 3, 2, 1, 0);
|
||||
|
||||
Array.Sort(first, comparer);
|
||||
Array.Sort(second, comparer);
|
||||
|
||||
Assert.Equal(first, second);
|
||||
}
|
||||
|
||||
public static IEnumerable<object[]> Comparers()
|
||||
{
|
||||
yield return [AdvancedSearchResultSerialComparer.Instance];
|
||||
yield return [AdvancedSearchResultTypeComparer.Instance];
|
||||
yield return [AdvancedSearchResultTypeComparer.InstanceReverse];
|
||||
yield return [AdvancedSearchResultNameComparer.Instance];
|
||||
yield return [AdvancedSearchResultNameComparer.InstanceReverse];
|
||||
yield return [AdvancedSearchResultMapComparer.Instance];
|
||||
yield return [AdvancedSearchResultMapComparer.InstanceReverse];
|
||||
yield return [AdvancedSearchResultSelectedComparer.Instance];
|
||||
yield return [AdvancedSearchResultSelectedComparer.InstanceReverse];
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[MemberData(nameof(Comparers))]
|
||||
public void EqualKeysOrderBySerialFromAnyArrivalOrder(IComparer<AdvancedSearchResult> comparer)
|
||||
{
|
||||
var inOrder = EqualKeyed();
|
||||
|
||||
AssertSameSequenceFromAnyArrival(inOrder, comparer);
|
||||
|
||||
var sorted = Shuffled(inOrder, 3, 0, 5, 1, 4, 2);
|
||||
Array.Sort(sorted, comparer);
|
||||
|
||||
Assert.Equal(inOrder, sorted);
|
||||
}
|
||||
|
||||
// Off-map results compare equal on range.
|
||||
[Fact]
|
||||
public void RangeComparerOrdersOffMapResultsBySerial()
|
||||
{
|
||||
var from = new Mobile();
|
||||
from.MoveToWorld(new Point3D(1000, 1000, 0), Map.Trammel);
|
||||
|
||||
try
|
||||
{
|
||||
var inOrder = EqualKeyed();
|
||||
|
||||
AssertSameSequenceFromAnyArrival(inOrder, new AdvancedSearchRangeComparer(from));
|
||||
AssertSameSequenceFromAnyArrival(inOrder, new AdvancedSearchRangeComparer(from, true));
|
||||
}
|
||||
finally
|
||||
{
|
||||
from.Delete();
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ReverseFlipsTheKeyNotTheTieBreak()
|
||||
{
|
||||
var a1 = Result(new Item(0x1), "alpha");
|
||||
var a2 = Result(new Item(0x1), "alpha");
|
||||
var b1 = Result(new Item(0x1), "beta");
|
||||
|
||||
var results = new[] { b1, a2, a1 };
|
||||
|
||||
Array.Sort(results, AdvancedSearchResultNameComparer.Instance);
|
||||
Assert.Equal([a1, a2, b1], results);
|
||||
|
||||
Array.Sort(results, AdvancedSearchResultNameComparer.InstanceReverse);
|
||||
Assert.Equal([b1, a1, a2], results);
|
||||
}
|
||||
}
|
||||
|
|
@ -819,6 +819,8 @@ public class AdvancedSearchGump : Gump
|
|||
}
|
||||
}
|
||||
|
||||
resultsList.Sort(AdvancedSearchResultSerialComparer.Instance);
|
||||
|
||||
SearchResults = resultsList.ToArray();
|
||||
|
||||
// Force the GC to collect the results
|
||||
|
|
|
|||
|
|
@ -2,86 +2,118 @@ using System.Collections.Generic;
|
|||
|
||||
namespace Server.Engines.AdvancedSearch;
|
||||
|
||||
public class AdvancedSearchResultTypeComparer : IComparer<AdvancedSearchResult>
|
||||
// Results arrive in worker-finish order, so every sort ends in the same serial tie-break.
|
||||
public abstract class AdvancedSearchResultComparer : IComparer<AdvancedSearchResult>
|
||||
{
|
||||
public static readonly AdvancedSearchResultTypeComparer Instance = new();
|
||||
public static readonly AdvancedSearchResultTypeComparer InstanceReverse = new(true);
|
||||
protected readonly bool Reverse;
|
||||
|
||||
private readonly bool _reverse;
|
||||
|
||||
public AdvancedSearchResultTypeComparer(bool reverse = false) => _reverse = reverse;
|
||||
protected AdvancedSearchResultComparer(bool reverse) => Reverse = reverse;
|
||||
|
||||
public int Compare(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
var a = x?.Entity?.GetType().Name;
|
||||
var b = y?.Entity?.GetType().Name;
|
||||
|
||||
return _reverse ? b.InsensitiveCompare(a) : a.InsensitiveCompare(b);
|
||||
}
|
||||
}
|
||||
|
||||
public class AdvancedSearchResultNameComparer : IComparer<AdvancedSearchResult>
|
||||
{
|
||||
public static readonly AdvancedSearchResultNameComparer Instance = new();
|
||||
public static readonly AdvancedSearchResultNameComparer InstanceReverse = new(true);
|
||||
|
||||
private readonly bool _reverse;
|
||||
|
||||
public AdvancedSearchResultNameComparer(bool reverse = false) => _reverse = reverse;
|
||||
|
||||
public int Compare(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
var a = x?.Name;
|
||||
var b = y?.Name;
|
||||
|
||||
return _reverse ? b.InsensitiveCompare(a) : a.InsensitiveCompare(b);
|
||||
}
|
||||
}
|
||||
|
||||
public class AdvancedSearchResultMapComparer : IComparer<AdvancedSearchResult>
|
||||
{
|
||||
public static readonly AdvancedSearchResultMapComparer Instance = new();
|
||||
public static readonly AdvancedSearchResultMapComparer InstanceReverse = new(true);
|
||||
|
||||
private readonly bool _reverse;
|
||||
|
||||
public AdvancedSearchResultMapComparer(bool reverse = false) => _reverse = reverse;
|
||||
|
||||
public int Compare(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
var a = x?.Map?.MapID ?? -1;
|
||||
var b = y?.Map?.MapID ?? -1;
|
||||
|
||||
return _reverse ? b.CompareTo(a) : a.CompareTo(b);
|
||||
}
|
||||
}
|
||||
|
||||
public class AdvancedSearchRangeComparer : IComparer<AdvancedSearchResult>
|
||||
{
|
||||
private readonly bool _reverse;
|
||||
private readonly Mobile _from;
|
||||
|
||||
public AdvancedSearchRangeComparer(Mobile from, bool reverse = false)
|
||||
{
|
||||
_from = from;
|
||||
_reverse = reverse;
|
||||
}
|
||||
|
||||
public int Compare(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
if (_from == null || x == null && y == null)
|
||||
if (ReferenceEquals(x, y))
|
||||
{
|
||||
return 0;
|
||||
}
|
||||
|
||||
if (x == null)
|
||||
{
|
||||
return _reverse ? 1 : -1;
|
||||
return -1;
|
||||
}
|
||||
|
||||
if (y == null)
|
||||
{
|
||||
return _reverse ? -1 : 1;
|
||||
return 1;
|
||||
}
|
||||
|
||||
var c = CompareKey(x, y);
|
||||
|
||||
return c != 0 ? c : CompareSerial(x, y);
|
||||
}
|
||||
|
||||
protected abstract int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y);
|
||||
|
||||
// Always ascending; the direction applies to the key only.
|
||||
public static int CompareSerial(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
var a = x.Entity?.Serial ?? Serial.Zero;
|
||||
var b = y.Entity?.Serial ?? Serial.Zero;
|
||||
|
||||
return a.CompareTo(b);
|
||||
}
|
||||
}
|
||||
|
||||
public sealed class AdvancedSearchResultSerialComparer : AdvancedSearchResultComparer
|
||||
{
|
||||
public static readonly AdvancedSearchResultSerialComparer Instance = new();
|
||||
|
||||
private AdvancedSearchResultSerialComparer() : base(false)
|
||||
{
|
||||
}
|
||||
|
||||
protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) => 0;
|
||||
}
|
||||
|
||||
public sealed class AdvancedSearchResultTypeComparer : AdvancedSearchResultComparer
|
||||
{
|
||||
public static readonly AdvancedSearchResultTypeComparer Instance = new();
|
||||
public static readonly AdvancedSearchResultTypeComparer InstanceReverse = new(true);
|
||||
|
||||
public AdvancedSearchResultTypeComparer(bool reverse = false) : base(reverse)
|
||||
{
|
||||
}
|
||||
|
||||
protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
var a = x.Entity?.GetType().Name;
|
||||
var b = y.Entity?.GetType().Name;
|
||||
|
||||
return Reverse ? b.InsensitiveCompare(a) : a.InsensitiveCompare(b);
|
||||
}
|
||||
}
|
||||
|
||||
public sealed class AdvancedSearchResultNameComparer : AdvancedSearchResultComparer
|
||||
{
|
||||
public static readonly AdvancedSearchResultNameComparer Instance = new();
|
||||
public static readonly AdvancedSearchResultNameComparer InstanceReverse = new(true);
|
||||
|
||||
public AdvancedSearchResultNameComparer(bool reverse = false) : base(reverse)
|
||||
{
|
||||
}
|
||||
|
||||
protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) =>
|
||||
Reverse ? y.Name.InsensitiveCompare(x.Name) : x.Name.InsensitiveCompare(y.Name);
|
||||
}
|
||||
|
||||
public sealed class AdvancedSearchResultMapComparer : AdvancedSearchResultComparer
|
||||
{
|
||||
public static readonly AdvancedSearchResultMapComparer Instance = new();
|
||||
public static readonly AdvancedSearchResultMapComparer InstanceReverse = new(true);
|
||||
|
||||
public AdvancedSearchResultMapComparer(bool reverse = false) : base(reverse)
|
||||
{
|
||||
}
|
||||
|
||||
protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
var a = x.Map?.MapID ?? -1;
|
||||
var b = y.Map?.MapID ?? -1;
|
||||
|
||||
return Reverse ? b.CompareTo(a) : a.CompareTo(b);
|
||||
}
|
||||
}
|
||||
|
||||
public sealed class AdvancedSearchRangeComparer : AdvancedSearchResultComparer
|
||||
{
|
||||
private readonly Mobile _from;
|
||||
|
||||
public AdvancedSearchRangeComparer(Mobile from, bool reverse = false) : base(reverse) => _from = from;
|
||||
|
||||
protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
{
|
||||
if (_from == null)
|
||||
{
|
||||
return 0;
|
||||
}
|
||||
|
||||
var fromMap = _from.Map;
|
||||
|
|
@ -93,36 +125,31 @@ public class AdvancedSearchRangeComparer : IComparer<AdvancedSearchResult>
|
|||
|
||||
if (x.Map == fromMap && y.Map != fromMap)
|
||||
{
|
||||
return _reverse ? 1 : -1;
|
||||
return Reverse ? 1 : -1;
|
||||
}
|
||||
|
||||
if (x.Map != fromMap && y.Map == fromMap)
|
||||
{
|
||||
return _reverse ? -1 : 1;
|
||||
return Reverse ? -1 : 1;
|
||||
}
|
||||
|
||||
var xDist = _from.GetDistanceToSqrt(x.Location);
|
||||
var yDist = _from.GetDistanceToSqrt(y.Location);
|
||||
|
||||
return _reverse ? yDist.CompareTo(xDist) : xDist.CompareTo(yDist);
|
||||
return Reverse ? yDist.CompareTo(xDist) : xDist.CompareTo(yDist);
|
||||
}
|
||||
}
|
||||
|
||||
public class AdvancedSearchResultSelectedComparer : IComparer<AdvancedSearchResult>
|
||||
public sealed class AdvancedSearchResultSelectedComparer : AdvancedSearchResultComparer
|
||||
{
|
||||
public static readonly AdvancedSearchResultSelectedComparer Instance = new();
|
||||
public static readonly AdvancedSearchResultSelectedComparer InstanceReverse = new(true);
|
||||
|
||||
private readonly bool _reverse;
|
||||
|
||||
public AdvancedSearchResultSelectedComparer(bool reverse = false) => _reverse = reverse;
|
||||
|
||||
public int Compare(AdvancedSearchResult x, AdvancedSearchResult y)
|
||||
public AdvancedSearchResultSelectedComparer(bool reverse = false) : base(reverse)
|
||||
{
|
||||
var a = x?.Selected ?? false;
|
||||
var b = y?.Selected ?? false;
|
||||
|
||||
// True then false, which is 1 then 0, so the comparison is reverse of integers
|
||||
return _reverse ? a.CompareTo(b) : b.CompareTo(a);
|
||||
}
|
||||
|
||||
// True then false, which is 1 then 0, so the comparison is reverse of integers
|
||||
protected override int CompareKey(AdvancedSearchResult x, AdvancedSearchResult y) =>
|
||||
Reverse ? x.Selected.CompareTo(y.Selected) : y.Selected.CompareTo(x.Selected);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue