diff --git a/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java b/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java index da157527..6aa065dc 100644 --- a/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java +++ b/src/main/java/net/tfminecraft/simplefactions/managers/CommandManager.java @@ -496,7 +496,9 @@ 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(!rgb.equalsIgnoreCase(g.getRGB()) && !RandomRGB.isFree(rgb)) { + } else if(result == 4) { + p.sendMessage("§c0,0,0 is reserved for unclaimed land; pick another colour"); + } else if(!RandomRGB.sameRGB(rgb, g.getRGB()) && !RandomRGB.isFree(rgb)) { p.sendMessage("§cThat colour is already used"); } else { g.setRGB(rgb); @@ -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 && !RandomRGB.sameRGB(rgb, 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/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/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); 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() {