Merge pull request #981 from aeros/issue926-paginate-long-lines

Improve LinePaginator to support long lines

5 files changed, 149 insertions, 27 deletions

diff --git a/bot/cogs/help.py b/bot/cogs/help.py
index 542f19139..832f6ea6b 100644
--- a/bot/cogs/help.py
+++ b/bot/cogs/help.py
@@ -299,7 +299,7 @@ class CustomHelpCommand(HelpCommand):
             embed,
             prefix=description,
             max_lines=COMMANDS_PER_PAGE,
-            max_size=2040,
+            max_size=2000,
         )
 
     async def send_bot_help(self, mapping: dict) -> None:
@@ -346,7 +346,7 @@ class CustomHelpCommand(HelpCommand):
             # add any remaining command help that didn't get added in the last iteration above.
             pages.append(page)
 
-        await LinePaginator.paginate(pages, self.context, embed=embed, max_lines=1, max_size=2040)
+        await LinePaginator.paginate(pages, self.context, embed=embed, max_lines=1, max_size=2000)
 
 
 class Help(Cog):
diff --git a/bot/cogs/moderation/management.py b/bot/cogs/moderation/management.py
index c39c7f3bc..617d957ed 100644
--- a/bot/cogs/moderation/management.py
+++ b/bot/cogs/moderation/management.py
@@ -268,12 +268,12 @@ class ModManagement(commands.Cog):
             User: {self.bot.get_user(user_id)} (`{user_id}`)
             Type: **{infraction["type"]}**
             Shadow: {hidden}
-            Reason: {infraction["reason"] or "*None*"}
             Created: {created}
             Expires: {expires}
             Remaining: {remaining}
             Actor: {actor.mention if actor else actor_id}
             ID: `{infraction["id"]}`
+            Reason: {infraction["reason"] or "*None*"}
             {"**===============**" if active else "==============="}
         """)
 
diff --git a/bot/cogs/watchchannels/talentpool.py b/bot/cogs/watchchannels/talentpool.py
index 14547105f..33550f68e 100644
--- a/bot/cogs/watchchannels/talentpool.py
+++ b/bot/cogs/watchchannels/talentpool.py
@@ -224,7 +224,7 @@ class TalentPool(WatchChannel, Cog, name="Talentpool"):
                 Status: **Active**
                 Date: {start_date}
                 Actor: {actor.mention if actor else actor_id}
-                Reason: {textwrap.shorten(nomination_object["reason"], width=200, placeholder="...")}
+                Reason: {nomination_object["reason"]}
                 Nomination ID: `{nomination_object["id"]}`
                 ===============
                 """
@@ -237,10 +237,10 @@ class TalentPool(WatchChannel, Cog, name="Talentpool"):
                 Status: Inactive
                 Date: {start_date}
                 Actor: {actor.mention if actor else actor_id}
-                Reason: {textwrap.shorten(nomination_object["reason"], width=200, placeholder="...")}
+                Reason: {nomination_object["reason"]}
 
                 End date: {end_date}
-                Unwatch reason: {textwrap.shorten(nomination_object["end_reason"], width=200, placeholder="...")}
+                Unwatch reason: {nomination_object["end_reason"]}
                 Nomination ID: `{nomination_object["id"]}`
                 ===============
                 """
diff --git a/bot/pagination.py b/bot/pagination.py
index 2aa3590ba..94c2d7c0c 100644
--- a/bot/pagination.py
+++ b/bot/pagination.py
@@ -37,12 +37,19 @@ class LinePaginator(Paginator):
         The suffix appended at the end of every page. e.g. three backticks.
     * max_size: `int`
         The maximum amount of codepoints allowed in a page.
+    * scale_to_size: `int`
+        The maximum amount of characters a single line can scale up to.
     * max_lines: `int`
         The maximum amount of lines allowed in a page.
     """
 
     def __init__(
-        self, prefix: str = '```', suffix: str = '```', max_size: int = 2000, max_lines: int = None
+        self,
+        prefix: str = '```',
+        suffix: str = '```',
+        max_size: int = 2000,
+        scale_to_size: int = 2000,
+        max_lines: t.Optional[int] = None
     ) -> None:
         """
         This function overrides the Paginator.__init__ from inside discord.ext.commands.
@@ -51,7 +58,21 @@ class LinePaginator(Paginator):
         """
         self.prefix = prefix
         self.suffix = suffix
+
+        # Embeds that exceed 2048 characters will result in an HTTPException
+        # (Discord API limit), so we've set a limit of 2000
+        if max_size > 2000:
+            raise ValueError(f"max_size must be <= 2,000 characters. ({max_size} > 2000)")
+
         self.max_size = max_size - len(suffix)
+
+        if scale_to_size < max_size:
+            raise ValueError(f"scale_to_size must be >= max_size. ({scale_to_size} < {max_size})")
+
+        if scale_to_size > 2000:
+            raise ValueError(f"scale_to_size must be <= 2,000 characters. ({scale_to_size} > 2000)")
+
+        self.scale_to_size = scale_to_size - len(suffix)
         self.max_lines = max_lines
         self._current_page = [prefix]
         self._linecount = 0
@@ -62,23 +83,38 @@ class LinePaginator(Paginator):
         """
         Adds a line to the current page.
 
-        If the line exceeds the `self.max_size` then an exception is raised.
+        If a line on a page exceeds `max_size` characters, then `max_size` will go up to
+        `scale_to_size` for a single line before creating a new page for the overflow words. If it
+        is still exceeded, the excess characters are stored and placed on the next pages unti
+        there are none remaining (by word boundary). The line is truncated if `scale_to_size` is
+        still exceeded after attempting to continue onto the next page.
+
+        In the case that the page already contains one or more lines and the new lines would cause
+        `max_size` to be exceeded, a new page is created. This is done in order to make a best
+        effort to avoid breaking up single lines across pages, while keeping the total length of the
+        page at a reasonable size.
 
         This function overrides the `Paginator.add_line` from inside `discord.ext.commands`.
 
         It overrides in order to allow us to configure the maximum number of lines per page.
         """
-        if len(line) > self.max_size - len(self.prefix) - 2:
-            raise RuntimeError('Line exceeds maximum page size %s' % (self.max_size - len(self.prefix) - 2))
-
-        if self.max_lines is not None:
-            if self._linecount >= self.max_lines:
-                self._linecount = 0
-                self.close_page()
-
-            self._linecount += 1
-        if self._count + len(line) + 1 > self.max_size:
-            self.close_page()
+        remaining_words = None
+        if len(line) > (max_chars := self.max_size - len(self.prefix) - 2):
+            if len(line) > self.scale_to_size:
+                line, remaining_words = self._split_remaining_words(line, max_chars)
+                if len(line) > self.scale_to_size:
+                    log.debug("Could not continue to next page, truncating line.")
+                    line = line[:self.scale_to_size]
+
+        # Check if we should start a new page or continue the line on the current one
+        if self.max_lines is not None and self._linecount >= self.max_lines:
+            log.debug("max_lines exceeded, creating new page.")
+            self._new_page()
+        elif self._count + len(line) + 1 > self.max_size and self._linecount > 0:
+            log.debug("max_size exceeded on page with lines, creating new page.")
+            self._new_page()
+
+        self._linecount += 1
 
         self._count += len(line) + 1
         self._current_page.append(line)
@@ -87,6 +123,65 @@ class LinePaginator(Paginator):
             self._current_page.append('')
             self._count += 1
 
+        # Start a new page if there were any overflow words
+        if remaining_words:
+            self._new_page()
+            self.add_line(remaining_words)
+
+    def _new_page(self) -> None:
+        """
+        Internal: start a new page for the paginator.
+
+        This closes the current page and resets the counters for the new page's line count and
+        character count.
+        """
+        self._linecount = 0
+        self._count = len(self.prefix) + 1
+        self.close_page()
+
+    def _split_remaining_words(self, line: str, max_chars: int) -> t.Tuple[str, t.Optional[str]]:
+        """
+        Internal: split a line into two strings -- reduced_words and remaining_words.
+
+        reduced_words: the remaining words in `line`, after attempting to remove all words that
+            exceed `max_chars` (rounding down to the nearest word boundary).
+
+        remaining_words: the words in `line` which exceed `max_chars`. This value is None if
+            no words could be split from `line`.
+
+        If there are any remaining_words, an ellipses is appended to reduced_words and a
+        continuation header is inserted before remaining_words to visually communicate the line
+        continuation.
+
+        Return a tuple in the format (reduced_words, remaining_words).
+        """
+        reduced_words = []
+        remaining_words = []
+
+        # "(Continued)" is used on a line by itself to indicate the continuation of last page
+        continuation_header = "(Continued)\n-----------\n"
+        reduced_char_count = 0
+        is_full = False
+
+        for word in line.split(" "):
+            if not is_full:
+                if len(word) + reduced_char_count <= max_chars:
+                    reduced_words.append(word)
+                    reduced_char_count += len(word) + 1
+                else:
+                    # If reduced_words is empty, we were unable to split the words across pages
+                    if not reduced_words:
+                        return line, None
+                    is_full = True
+                    remaining_words.append(word)
+            else:
+                remaining_words.append(word)
+
+        return (
+            " ".join(reduced_words) + "..." if remaining_words else "",
+            continuation_header + " ".join(remaining_words) if remaining_words else None
+        )
+
     @classmethod
     async def paginate(
         cls,
@@ -97,6 +192,7 @@ class LinePaginator(Paginator):
         suffix: str = "",
         max_lines: t.Optional[int] = None,
         max_size: int = 500,
+        scale_to_size: int = 2000,
         empty: bool = True,
         restrict_to_user: User = None,
         timeout: int = 300,
@@ -142,7 +238,8 @@ class LinePaginator(Paginator):
                 ))
             )
 
-        paginator = cls(prefix=prefix, suffix=suffix, max_size=max_size, max_lines=max_lines)
+        paginator = cls(prefix=prefix, suffix=suffix, max_size=max_size, max_lines=max_lines,
+                        scale_to_size=scale_to_size)
         current_page = 0
 
         if not lines:
diff --git a/tests/bot/test_pagination.py b/tests/bot/test_pagination.py
index 0a734b505..ce880d457 100644
--- a/tests/bot/test_pagination.py
+++ b/tests/bot/test_pagination.py
@@ -8,17 +8,42 @@ class LinePaginatorTests(TestCase):
 
     def setUp(self):
         """Create a paginator for the test method."""
-        self.paginator = pagination.LinePaginator(prefix='', suffix='', max_size=30)
-
-    def test_add_line_raises_on_too_long_lines(self):
-        """`add_line` should raise a `RuntimeError` for too long lines."""
-        message = f"Line exceeds maximum page size {self.paginator.max_size - 2}"
-        with self.assertRaises(RuntimeError, msg=message):
-            self.paginator.add_line('x' * self.paginator.max_size)
+        self.paginator = pagination.LinePaginator(prefix='', suffix='', max_size=30,
+                                                  scale_to_size=50)
 
     def test_add_line_works_on_small_lines(self):
         """`add_line` should allow small lines to be added."""
         self.paginator.add_line('x' * (self.paginator.max_size - 3))
+        # Note that the page isn't added to _pages until it's full.
+        self.assertEqual(len(self.paginator._pages), 0)
+
+    def test_add_line_works_on_long_lines(self):
+        """After additional lines after `max_size` is exceeded should go on the next page."""
+        self.paginator.add_line('x' * self.paginator.max_size)
+        self.assertEqual(len(self.paginator._pages), 0)
+
+        # Any additional lines should start a new page after `max_size` is exceeded.
+        self.paginator.add_line('x')
+        self.assertEqual(len(self.paginator._pages), 1)
+
+    def test_add_line_continuation(self):
+        """When `scale_to_size` is exceeded, remaining words should be split onto the next page."""
+        self.paginator.add_line('zyz ' * (self.paginator.scale_to_size//4 + 1))
+        self.assertEqual(len(self.paginator._pages), 1)
+
+    def test_add_line_no_continuation(self):
+        """If adding a new line to an existing page would exceed `max_size`, it should start a new
+        page rather than using continuation.
+        """
+        self.paginator.add_line('z' * (self.paginator.max_size - 3))
+        self.paginator.add_line('z')
+        self.assertEqual(len(self.paginator._pages), 1)
+
+    def test_add_line_truncates_very_long_words(self):
+        """`add_line` should truncate if a single long word exceeds `scale_to_size`."""
+        self.paginator.add_line('x' * (self.paginator.scale_to_size + 1))
+        # Note: item at index 1 is the truncated line, index 0 is prefix
+        self.assertEqual(self.paginator._current_page[1], 'x' * self.paginator.scale_to_size)
 
 
 class ImagePaginatorTests(TestCase):