sir-lancebot icon indicating copy to clipboard operation
sir-lancebot copied to clipboard

Bookmark enhancements

Open ChrisLovering opened this issue 3 years ago • 4 comments

Relevant Issues

Closes #918

Description

This simplifies the bookmark implementation, and adds the ability to delete bookmarks in DMs.

To reduce complexity, the delete bookmark functionality actually allows users to delete any sir-lancebot message within their own DMs, and is labeled as such.

Did you:

ChrisLovering avatar Apr 26 '22 17:04 ChrisLovering

@ChrisLovering Enhance faster!

Will you be finishing up this PR sometime soon?

Xithrius avatar May 27 '22 22:05 Xithrius

Overall it works nicely! Thanks for working on this! I have a few documentation suggestions to improve user usability, and also I'd like to repeat the help command unideal behaviour.

image

Using .help is a bit confusing since it doesn't work in DMs nor any channel except for #bot-commands.

IMO it's even more confusing if we allow it in DMs, since the Help command only shows the command directly, which makes users think they can run &delete directly image Not too sure what to do here.

ChrisLovering avatar May 29 '22 08:05 ChrisLovering

IMO it's even more confusing if we allow it in DMs, since the Help command only shows the command directly, which makes users think they can run &delete directly

Looked into that bug and it seems to be because specifying root_aliases causes bookmark delete to be registered as a command rather than subcommand, which causes it to be listed even though subcommands usually aren't, and this isn't done correctly (this isn't specific to DMs).

Tentative patch that seems to fix it:

diff --git a/bot/exts/core/help.py b/bot/exts/core/help.py
index db3c2aa6..f7df0760 100644
--- a/bot/exts/core/help.py
+++ b/bot/exts/core/help.py
@@ -277,7 +277,7 @@ class HelpSession:
             else:
                 results.append(f"<{name}>")

-        return f"{cmd.name} {' '.join(results)}"
+        return f"{cmd.qualified_name} {' '.join(results)}"

     async def build_pages(self) -> None:
         """Builds the list of content pages to be paginated through in the help message, as a list of str."""
@@ -304,8 +304,9 @@ class HelpSession:
         prefix = constants.Client.prefix

         signature = self._get_command_params(self.query)
+        paginator.add_line(f"**```\n{prefix}{signature}\n```**")
+
         parent = self.query.full_parent_name + " " if self.query.parent else ""
-        paginator.add_line(f"**```\n{prefix}{parent}{signature}\n```**")
         aliases = [f"`{alias}`" if not parent else f"`{parent} {alias}`" for alias in self.query.aliases]
         aliases += [f"`{alias}`" for alias in getattr(self.query, "root_aliases", ())]
         aliases = ", ".join(sorted(aliases))

If this seems right I don't mind if you want to kaizen this in here or want me to open a separate PR.

Alternative solutions would be:

  • Remove the root alias here
  • Add a root alias called delete
  • Make the help command not display subcommands with root aliases

wookie184 avatar May 29 '22 22:05 wookie184

I've gone ahead and applied @wookie184's suggested fix and rebased onto main.

ChrisLovering avatar Jul 10 '22 11:07 ChrisLovering

Force push is to rebase onto main

ChrisLovering avatar Aug 19 '22 09:08 ChrisLovering