From 3fbcb945241a906c6af5d20cd23e678b95556581 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:37:59 +0000 Subject: [PATCH 1/2] fix: refuse 0,0,0 and duplicate colours for realms The web map reads land without an owner as 0,0,0, so a realm coloured black claimed all of it on hover and could not be picked itself. Realm, guild and title colours now refuse 0,0,0, and /faction setcolour refuses a colour another realm or guild already uses, as /guild setcolour does. Co-Authored-By: Claude Opus 5.5 --- .../simplefactions/managers/CommandManager.java | 13 ++++++++++--- .../simplefactions/managers/FactionManager.java | 4 ++++ .../tiers/admin/TitleAdminService.java | 2 ++ .../managers/CommandManagerCoverageTest.java | 8 +++++++- .../managers/FactionManagerCoverageTest.java | 2 +- .../tiers/admin/TitleAdminServiceTest.java | 6 ++++++ 6 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java index da157527..9e03b387 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java @@ -496,6 +496,8 @@ private boolean dispatchCommand(CommandSender sender, Command cmd, String label, p.sendMessage("§cRGB values must be numbers (e.g. 128,128,128)"); } else if(result == 3) { p.sendMessage("§cEach RGB value must be between 0 and 255"); + } else if(result == 4) { + p.sendMessage("§c0,0,0 is reserved for unclaimed land; pick another colour"); } else if(!rgb.equalsIgnoreCase(g.getRGB()) && !RandomRGB.isFree(rgb)) { p.sendMessage("§cThat colour is already used"); } else { @@ -837,9 +839,12 @@ private boolean dispatchCommand(CommandSender sender, Command cmd, String label, String rgb = args[1]; int result = FactionManager.validateRGB(rgb); - if(result == 0) { - Faction f = FactionManager.getByLeader(Members.key(p)); - FactionManager.getMap().enqueue("nation", f.getRGB()); + Faction f = FactionManager.getByLeader(Members.key(p)); + String old = f.getRGB(); + if(result == 0 && !rgb.equalsIgnoreCase(old) && !RandomRGB.isFree(rgb)) { + p.sendMessage("§cThat colour is already used"); + } else if(result == 0) { + FactionManager.getMap().enqueue("nation", old); f.setRGB(rgb); FactionManager.getMap().enqueue("nation", f.getRGB()); p.sendMessage("§aFaction colour updated to §f" + rgb); @@ -849,6 +854,8 @@ private boolean dispatchCommand(CommandSender sender, Command cmd, String label, p.sendMessage("§cRGB values must be numbers (e.g. 128,128,128)"); } else if(result == 3) { p.sendMessage("§cEach RGB value must be between 0 and 255"); + } else if(result == 4) { + p.sendMessage("§c0,0,0 is reserved for unclaimed land; pick another colour"); } } else { diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java index e35fa35f..e543e915 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/FactionManager.java @@ -904,10 +904,14 @@ public static int validateRGB(String rgb) { if (parts.length != 3) return 1; try { + int total = 0; for (String part : parts) { int value = Integer.parseInt(part.trim()); if (value < 0 || value > 255) return 3; + total += value; } + // The web map reads land without an owner as 0,0,0. + if (total == 0) return 4; return 0; // All good } catch (NumberFormatException e) { return 2; diff --git a/src/main/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminService.java b/src/main/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminService.java index 32bb9606..9898ce35 100644 --- a/src/main/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminService.java +++ b/src/main/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminService.java @@ -74,6 +74,8 @@ public static Result setColour(String id, String rawRgb) { if (title == null) return Result.error("No title with the id " + id); String rgb = parseRgb(rawRgb); if (rgb == null) return Result.error("Colour must be R,G,B with each value 0-255, e.g. 200,168,80"); + // The web map reads land without an owner as 0,0,0. + if (rgb.equals("0,0,0")) return Result.error("0,0,0 is reserved for unclaimed land; pick another colour"); String old = title.getRgb(); if (rgb.equals(old)) return Result.error(title.getId() + " already uses " + rgb); for (Title other : TitleLoader.getByTier(title.getTier())) { diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/CommandManagerCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/CommandManagerCoverageTest.java index 63a6c35b..cae14317 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/CommandManagerCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/CommandManagerCoverageTest.java @@ -835,7 +835,9 @@ void baseGuildAppearanceIsChangedThroughTheFactionCommands(String action, String "faction,2,must be numbers", "guild,2,must be numbers", "faction,3,between 0 and 255", - "guild,3,between 0 and 255" + "guild,3,between 0 and 255", + "faction,4,reserved for unclaimed land", + "guild,4,reserved for unclaimed land" }) void invalidColoursHaveSpecificValidationMessages(String name, int code, String message) { guildLeader(); @@ -855,6 +857,10 @@ void validColoursQueueBothFactionMapStatesAndRespectGuildColourUniqueness() { verify(map).enqueue("nation", "1,1,1"); verify(map).enqueue("nation", "2,2,2"); try (MockedStatic colours = mockStatic(RandomRGB.class)) { + assertTrue(run("faction", "setcolour", "3,3,3")); + verify(player).sendMessage(contains("already used")); + verify(faction, never()).setRGB("3,3,3"); + clearInvocations(player); assertTrue(run("guild", "setcolour", "2,2,2")); verify(player).sendMessage(contains("already used")); colours.when(() -> RandomRGB.isFree("2,2,2")).thenReturn(true); diff --git a/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java index 0d547efe..ad24adbf 100644 --- a/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/managers/FactionManagerCoverageTest.java @@ -307,7 +307,7 @@ void joiningGuildIsAllowedForUnaffiliatedAndRealmMembersButNotGuildLeaders() { } @ParameterizedTest - @CsvSource(value = {"0,0,0|0", "255, 12, 3|0", "1,2|1", "1,2,3,4|1", "x,2,3|2", "-1,2,3|3", "256,2,3|3"}, delimiter = '|') + @CsvSource(value = {"0,0,0|4", "0, 0, 00|4", "0,0,1|0", "255, 12, 3|0", "1,2|1", "1,2,3,4|1", "x,2,3|2", "-1,2,3|3", "256,2,3|3"}, delimiter = '|') void rgbValidationProvidesSpecificErrors(String input, int expected) { assertEquals(expected, FactionManager.validateRGB(input)); assertEquals(1, FactionManager.validateRGB(null)); diff --git a/src/test/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminServiceTest.java b/src/test/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminServiceTest.java index 1ff61d25..817f8032 100644 --- a/src/test/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminServiceTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/tiers/admin/TitleAdminServiceTest.java @@ -87,6 +87,12 @@ void setColourMustBeUniqueWithinATierOnly() { assertFalse(TitleAdminService.setColour("COUNTY_1", "1,2").ok()); } + @Test + void setColourRefusesTheUnclaimedLandColour() { + assertFalse(TitleAdminService.setColour("COUNTY_1", "0, 0, 00").ok()); + assertEquals("1,1,1", county1.getRgb()); + } + @Test void addProvinceMovesItFromItsOldTitle() { Result result = TitleAdminService.addProvinces("COUNTY_3", List.of("1"), ANY_PROVINCE); From f1e36ba2f6230ac315d4ca16a47a65a4b584ae7e Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:55:20 +0000 Subject: [PATCH 2/2] fix: compare colours by value when checking they are free 255,0,00 and 255,0,0 are the same colour on the map, so the uniqueness checks now compare the numbers rather than the text. Co-Authored-By: Claude Opus 5.5 --- .../managers/CommandManager.java | 4 ++-- .../simplefactions/utils/RandomRGB.java | 20 +++++++++++++++++-- .../SharedDomainUtilitiesCoverageTest.java | 11 ++++++++++ 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java index 9e03b387..6aa065dc 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java @@ -498,7 +498,7 @@ private boolean dispatchCommand(CommandSender sender, Command cmd, String label, p.sendMessage("§cEach RGB value must be between 0 and 255"); } else if(result == 4) { p.sendMessage("§c0,0,0 is reserved for unclaimed land; pick another colour"); - } else if(!rgb.equalsIgnoreCase(g.getRGB()) && !RandomRGB.isFree(rgb)) { + } else if(!RandomRGB.sameRGB(rgb, g.getRGB()) && !RandomRGB.isFree(rgb)) { p.sendMessage("§cThat colour is already used"); } else { g.setRGB(rgb); @@ -841,7 +841,7 @@ private boolean dispatchCommand(CommandSender sender, Command cmd, String label, Faction f = FactionManager.getByLeader(Members.key(p)); String old = f.getRGB(); - if(result == 0 && !rgb.equalsIgnoreCase(old) && !RandomRGB.isFree(rgb)) { + if(result == 0 && !RandomRGB.sameRGB(rgb, old) && !RandomRGB.isFree(rgb)) { p.sendMessage("§cThat colour is already used"); } else if(result == 0) { FactionManager.getMap().enqueue("nation", old); diff --git a/src/main/java/net/tfminecraft/simplefactions/utils/RandomRGB.java b/src/main/java/net/tfminecraft/simplefactions/utils/RandomRGB.java index 60946f67..c5afc4c3 100644 --- a/src/main/java/net/tfminecraft/simplefactions/utils/RandomRGB.java +++ b/src/main/java/net/tfminecraft/simplefactions/utils/RandomRGB.java @@ -107,13 +107,29 @@ private static float hueToRGB(float p, float q, float t) { public static boolean isFree(String rgb) { for(Faction f : FactionManager.factions) { - if(f.getRGB().equalsIgnoreCase(rgb)) return false; + if(sameRGB(f.getRGB(), rgb)) return false; for(Guild g : f.getGuildHandler().getGuilds()) { - if(g.getRGB().equalsIgnoreCase(rgb)) return false; + if(sameRGB(g.getRGB(), rgb)) return false; } } return true; } + /** Same colour by value, so 255,0,00 matches 255,0,0. */ + public static boolean sameRGB(String left, String right) { + if (left == null || right == null) return false; + String[] a = left.trim().split(",", -1); + String[] b = right.trim().split(",", -1); + if (a.length != 3 || b.length != 3) return left.trim().equalsIgnoreCase(right.trim()); + try { + for (int i = 0; i < 3; i++) { + if (Integer.parseInt(a[i].trim()) != Integer.parseInt(b[i].trim())) return false; + } + return true; + } catch (NumberFormatException e) { + return left.trim().equalsIgnoreCase(right.trim()); + } + } + } diff --git a/src/test/java/net/tfminecraft/simplefactions/utils/SharedDomainUtilitiesCoverageTest.java b/src/test/java/net/tfminecraft/simplefactions/utils/SharedDomainUtilitiesCoverageTest.java index 67fe9ed9..430c58e9 100644 --- a/src/test/java/net/tfminecraft/simplefactions/utils/SharedDomainUtilitiesCoverageTest.java +++ b/src/test/java/net/tfminecraft/simplefactions/utils/SharedDomainUtilitiesCoverageTest.java @@ -87,6 +87,17 @@ private void fund(String key, long pouch, long bank) { assertFalse(RandomRGB.isFree("21,42,63")); assertFalse(RandomRGB.isFree("84,105,126")); assertTrue(RandomRGB.isFree("0,0,0")); + assertFalse(RandomRGB.isFree("21, 042,63"), "same colour spelt differently"); + } + + @Test void coloursMatchByValue() { + assertTrue(RandomRGB.sameRGB("255,0,00", "255,0,0")); + assertFalse(RandomRGB.sameRGB("255,0,1", "255,0,0")); + assertFalse(RandomRGB.sameRGB(null, "255,0,0")); + assertTrue(RandomRGB.sameRGB("odd", "ODD"), "unparsable colours compare as text"); + assertFalse(RandomRGB.sameRGB("1,2", "1,2,3")); + assertTrue(RandomRGB.sameRGB("a,b,c", "A,B,C")); + assertFalse(RandomRGB.sameRGB("a,b,c", "1,2,3")); } @Test void randomFactionColorsRetryCollisionsAndRemainValidRgbValues() {