Rework View all characters - fix packet and limit max chrs

Thanks to Lehava#4668 for finding the packet structure

src/main/java/net/server/Server.java

@@ -1453,37 +1453,33 @@ public class Server {
     }
     */
 
-    public Pair<Pair<Integer, List<Character>>, List<Pair<Integer, List<Character>>>> loadAccountCharlist(Integer accountId, int visibleWorlds) {
+    public SortedMap<Integer, List<Character>> loadAccountCharlist(int accountId, int visibleWorlds) {
-        List<World> wlist = this.getWorlds();
+        List<World> worlds = this.getWorlds();
-        if (wlist.size() > visibleWorlds) {
+        if (worlds.size() > visibleWorlds) {
-            wlist = wlist.subList(0, visibleWorlds);
+            worlds = worlds.subList(0, visibleWorlds);
         }
 
-        List<Pair<Integer, List<Character>>> accChars = new ArrayList<>(wlist.size() + 1);
+        SortedMap<Integer, List<Character>> worldChrs = new TreeMap<>();
         int chrTotal = 0;
-        List<Character> lastwchars = null;
 
         lgnRLock.lock();
         try {
-            for (World w : wlist) {
+            for (World world : worlds) {
-                List<Character> wchars = w.getAccountCharactersView(accountId);
+                List<Character> chrs = world.getAccountCharactersView(accountId);
-                if (wchars == null) {
+                if (chrs == null) {
                     if (!accountChars.containsKey(accountId)) {
                         accountCharacterCount.put(accountId, (short) 0);
                         accountChars.put(accountId, new HashSet<>());    // not advisable at all to write on the map on a read-protected environment
                     }                                                           // yet it's known there's no problem since no other point in the source does
-                } else if (!wchars.isEmpty()) {                                  // this action.
+                } else if (!chrs.isEmpty()) {                                  // this action.
-                    lastwchars = wchars;
+                    worldChrs.put(world.getId(), chrs);
-
-                    accChars.add(new Pair<>(w.getId(), wchars));
-                    chrTotal += wchars.size();
                 }
             }
         } finally {
             lgnRLock.unlock();
         }
 
-        return new Pair<>(new Pair<>(chrTotal, lastwchars), accChars);
+        return worldChrs;
     }
 
     private static Pair<Short, List<List<Character>>> loadAccountCharactersViewFromDb(int accId, int wlen) {

src/main/java/net/server/handlers/login/ViewAllCharHandler.java

@@ -28,11 +28,12 @@ import net.AbstractPacketHandler;
 import net.packet.InPacket;
 import net.server.Server;
 import tools.PacketCreator;
-import tools.Pair;
 
-import java.util.List;
+import java.util.*;
 
 public final class ViewAllCharHandler extends AbstractPacketHandler {
+    private static final int CHARACTER_LIMIT = 60; // Client will crash if sending 61 or more characters
+
     @Override
     public final void handlePacket(InPacket p, Client c) {
         try {
@@ -41,34 +42,88 @@ public final class ViewAllCharHandler extends AbstractPacketHandler {
                 return;
             }
 
-            int accountId = c.getAccID();
+            SortedMap<Integer, List<Character>> worldChrs = Server.getInstance().loadAccountCharlist(c.getAccID(), c.getVisibleWorlds());
-            Pair<Pair<Integer, List<Character>>, List<Pair<Integer, List<Character>>>> loginBlob = Server.getInstance().loadAccountCharlist(accountId, c.getVisibleWorlds());
+            worldChrs = limitTotalChrs(worldChrs, CHARACTER_LIMIT);
 
-            List<Pair<Integer, List<Character>>> worldChars = loginBlob.getRight();
+            padChrsIfNeeded(worldChrs);
-            int chrTotal = loginBlob.getLeft().getLeft();
-            List<Character> lastwchars = loginBlob.getLeft().getRight();
 
-            if (chrTotal > 9) {
+            int totalWorlds = worldChrs.size();
-                int padRight = chrTotal % 3;
+            int totalChrs = countTotalChrs(worldChrs);
-                if (padRight > 0 && lastwchars != null) {
+            c.sendPacket(PacketCreator.showAllCharacter(totalWorlds, totalChrs));
-                    Character chr = lastwchars.get(lastwchars.size() - 1);
 
-                    for (int i = padRight; i < 3; i++) { // filling the remaining slots with the last character loaded
+            final boolean usePic = YamlConfig.config.server.ENABLE_PIC && !c.canBypassPic();
-                        chrTotal++;
+            worldChrs.forEach((worldId, chrs) ->
-                        lastwchars.add(chr);
+                    c.sendPacket(PacketCreator.showAllCharacterInfo(worldId, chrs, usePic))
-                    }
+            );
-                }
-            }
-
-            int charsSize = chrTotal;
-            int unk = charsSize + (3 - charsSize % 3); //rowSize?
-            c.sendPacket(PacketCreator.showAllCharacter(charsSize, unk));
-
-            for (Pair<Integer, List<Character>> wchars : worldChars) {
-                c.sendPacket(PacketCreator.showAllCharacterInfo(wchars.getLeft(), wchars.getRight(), YamlConfig.config.server.ENABLE_PIC && !c.canBypassPic()));
-            }
         } catch (Exception e) {
             e.printStackTrace();
         }
     }
+    
+    private static SortedMap<Integer, List<Character>> limitTotalChrs(SortedMap<Integer, List<Character>> worldChrs,
+                                                                      int limit) {
+        if (countTotalChrs(worldChrs) <= limit) {
+            return worldChrs;
+        } else {;
+            return cutAfterChrLimit(worldChrs, limit);
+        }
+    }
+
+    private static int countTotalChrs(Map<Integer, List<Character>> worldChrs) {
+        return worldChrs.values().stream()
+                .mapToInt(List::size)
+                .sum();
+    }
+
+    private static SortedMap<Integer, List<Character>> cutAfterChrLimit(SortedMap<Integer, List<Character>> worldChrs,
+                                                                        int limit) {
+        SortedMap<Integer, List<Character>> cappedCopy = new TreeMap<>();
+        int runningChrTotal = 0;
+        for (Map.Entry<Integer, List<Character>> entry : worldChrs.entrySet()) {
+            int worldId = entry.getKey();
+            List<Character> chrs = entry.getValue();
+            if (runningChrTotal + chrs.size() <= limit) { // Limit not reached, move them all
+                runningChrTotal += chrs.size();
+                cappedCopy.put(worldId, chrs);
+            } else { // Limit would be reached if all chrs were moved. Move just enough to fit within limit.
+                int remainingSlots = limit - runningChrTotal;
+                List<Character> lastChrs = chrs.subList(0, remainingSlots);
+                cappedCopy.put(worldId, lastChrs);
+                break;
+            }
+        }
+
+        return cappedCopy;
+    }
+
+    /**
+     * If there are more characters than fits the screen (9), and you start scrolling down,
+     * the characters on the last row will not appear unless the row is completely filled.
+     * Meaning, if there are 1 or 2 characters remaining on the last row, they will not appear.
+     *
+     * @param totalChrs total amount of characters to display on 'View all characters' screen
+     * @return if we need to pad the last row to include the characters that would otherwise not appear
+     */
+    private static void padChrsIfNeeded(SortedMap<Integer, List<Character>> worldChrs) {
+        while (shouldPadLastRow(countTotalChrs(worldChrs))) {
+            final List<Character> lastWorldChrs = getLastWorldChrs(worldChrs);
+            final Character lastChrForPadding = getLastItem(lastWorldChrs);
+            lastWorldChrs.add(lastChrForPadding);
+        }
+    }
+
+    private static boolean shouldPadLastRow(int totalChrs) {
+        boolean shouldScroll = totalChrs > 9;
+        boolean isLastRowFilled = totalChrs % 3 == 0;
+        return shouldScroll && !isLastRowFilled;
+    }
+
+    private static List<Character> getLastWorldChrs(SortedMap<Integer, List<Character>> worldChrs) {
+        return worldChrs.get(worldChrs.lastKey());
+    }
+
+    private static <T> T getLastItem(List<T> list) {
+        Objects.requireNonNull(list);
+        return list.get(list.size() - 1);
+    }
 }

src/main/java/net/server/world/World.java

@@ -515,8 +515,8 @@ public class World {
         return chrList;
     }
 
-    public List<Character> getAccountCharactersView(Integer accountId) {
+    public List<Character> getAccountCharactersView(int accountId) {
-        List<Character> chrList;
+        final List<Character> chrList;
 
         accountCharsLock.lock();
         try {

src/main/java/tools/PacketCreator.java

@@ -2473,11 +2473,11 @@ public class PacketCreator {
         return p;
     }
 
-    public static Packet showAllCharacter(int chars, int unk) {
+    public static Packet showAllCharacter(int totalWorlds, int totalChrs) {
         OutPacket p = OutPacket.create(SendOpcode.VIEW_ALL_CHAR);
-        p.writeByte(chars > 0 ? 1 : 5); // 2: already connected to server, 3 : unk error (view-all-characters), 5 : cannot find any
+        p.writeByte(totalChrs > 0 ? 1 : 5); // 2: already connected to server, 3 : unk error (view-all-characters), 5 : cannot find any
-        p.writeInt(chars);
+        p.writeInt(totalWorlds);
-        p.writeInt(unk);
+        p.writeInt(totalChrs);
         return p;
     }