-
Notifications
You must be signed in to change notification settings - Fork 0
Add tab completion for commands and arguments #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: dmccoystephenson <[email protected]>
Co-authored-by: dmccoystephenson <[email protected]>
…tions Co-authored-by: dmccoystephenson <[email protected]>
Co-authored-by: dmccoystephenson <[email protected]>
Co-authored-by: dmccoystephenson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements tab completion functionality for the Mailboxes plugin to enhance user experience by providing command and argument suggestions. The implementation adds comprehensive auto-completion support for all main commands and their context-specific arguments.
Key changes:
- Added
onTabCompletemethod to intercept tab completion requests for/mailboxesand/mcommands - Implemented context-aware argument completion for config options, list filters, and send command parameters
- Added permission-aware suggestions for the
-attachflag in the send command
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Config option names for tab completion (in camelCase to match config file) | ||
| private static final List<String> CONFIG_OPTIONS = Arrays.asList( | ||
| "debugMode", | ||
| "maxMessageIDNumber", | ||
| "maxMailboxIDNumber", | ||
| "preventSendingMessagesToSelf", | ||
| "assignmentAlertEnabled", | ||
| "unreadMessagesAlertEnabled", | ||
| "welcomeMessageEnabled", | ||
| "quotesEnabled", | ||
| "attachmentsEnabled", | ||
| "maxAttachmentStackSize" | ||
| ); |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config option 'version' is a valid option in ConfigService but is intentionally excluded from CONFIG_OPTIONS since it cannot be set by users. However, this should be documented with a comment explaining why 'version' is excluded, as it may not be obvious to future maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment explaining why 'version' is excluded. Commit: e4dbece
| private List<String> getSendCompletions(CommandSender sender, String[] args) { | ||
| if (args.length == 2) { | ||
| // Player names and -attach flag | ||
| List<String> completions = filterCompletions( | ||
| Bukkit.getOnlinePlayers().stream() | ||
| .map(Player::getName) | ||
| .collect(Collectors.toList()), | ||
| args[1] | ||
| ); | ||
| // Also suggest -attach flag if user has permission | ||
| completions.addAll(getAttachFlagSuggestion(sender, args, args[1])); | ||
| return completions; | ||
| } | ||
| // Suggest -attach flag for subsequent positions if not already present | ||
| if (args.length >= 3) { | ||
| return getAttachFlagSuggestion(sender, args, args[args.length - 1]); | ||
| } | ||
| return new ArrayList<>(); |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -attach flag suggestion is provided both at args.length == 2 (line 186) and args.length >= 3 (line 191). This means -attach will be suggested at position 2 (which is the player name position) alongside player names. The -attach flag should only be suggested after the player name is provided, not at the same position as the player name. Consider restricting -attach suggestions to args.length >= 3 only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed -attach flag to only suggest at position 3+ (after player name). Removed from position 2 suggestions. Commit: e4dbece
| return options.stream() | ||
| .filter(option -> option.toLowerCase().startsWith(input.toLowerCase())) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter method uses case-insensitive comparison (.toLowerCase()) but creates new strings for each comparison during streaming. For small lists this is fine, but consider caching the lowercased input outside the stream to avoid repeated toLowerCase() calls on the input string.
| return options.stream() | |
| .filter(option -> option.toLowerCase().startsWith(input.toLowerCase())) | |
| String lowerInput = input.toLowerCase(); | |
| return options.stream() | |
| .filter(option -> option.toLowerCase().startsWith(lowerInput)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cached lowercased input outside the stream to avoid repeated toLowerCase() calls. Commit: e4dbece
| if (args.length == 1) { | ||
| // Main subcommands | ||
| List<String> subcommands = Arrays.asList("help", "config", "list", "send", "open", "delete", "archive"); | ||
| return filterCompletions(subcommands, args[0]); |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding permission checks for the main subcommands in tab completion to avoid showing commands that users don't have access to. For example, only show "config" in completions if the user has 'mailboxes.config' permission. This is consistent with how CommandInterpreter.java checks permissions before executing commands and prevents information leakage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added permission checks for all main subcommands and their completions to prevent information leakage. Commit: e4dbece
| // Boolean config options (stored in lowercase for case-insensitive comparison) | ||
| private static final Set<String> BOOLEAN_CONFIG_OPTIONS = new HashSet<>(Arrays.asList( | ||
| "debugmode", | ||
| "preventsendingmessagestoself", | ||
| "assignmentalertenabled", | ||
| "unreadmessagesalertenabled", | ||
| "welcomemessageenabled", | ||
| "quotesenabled", | ||
| "attachmentsenabled" | ||
| )); |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config option constants are duplicated here and should be maintained centrally. The ConfigService already defines these options in its saveMissingConfigDefaultsIfNotPresent method. Consider extracting these to a shared constants class or making ConfigService expose a method that returns the list of valid config options. This will prevent inconsistencies when new config options are added in the future.
| // Boolean config options (stored in lowercase for case-insensitive comparison) | |
| private static final Set<String> BOOLEAN_CONFIG_OPTIONS = new HashSet<>(Arrays.asList( | |
| "debugmode", | |
| "preventsendingmessagestoself", | |
| "assignmentalertenabled", | |
| "unreadmessagesalertenabled", | |
| "welcomemessageenabled", | |
| "quotesenabled", | |
| "attachmentsenabled" | |
| )); | |
| // Boolean config options (stored in lowercase for case-insensitive comparison). | |
| // Derived from CONFIG_OPTIONS to avoid duplicating constant names. | |
| private static final Set<String> BOOLEAN_CONFIG_OPTIONS = new HashSet<>( | |
| CONFIG_OPTIONS.stream() | |
| .filter(option -> | |
| option.endsWith("Enabled") | |
| || option.endsWith("Mode") | |
| || option.startsWith("prevent")) | |
| .map(String::toLowerCase) | |
| .collect(Collectors.toSet()) | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment noting that BOOLEAN_CONFIG_OPTIONS is derived from CONFIG_OPTIONS to maintain consistency. The constants are kept inline for now as they require manual maintenance when adding new config options (the pattern-based suggestion would incorrectly classify some options). Commit: e4dbece
|
@copilot address comments |
…mizations Co-authored-by: dmccoystephenson <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Auto complete does not list offline players or message IDs |
Co-authored-by: dmccoystephenson <[email protected]>
Added tab completion for offline players in send command and message IDs for open/delete/archive commands. Commit: 6018c96 The send command now includes both online and offline players in suggestions. The open, delete, and archive commands now suggest message IDs from the player's mailbox (both active and archived messages). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private List<String> getMessageIdCompletions(CommandSender sender, String[] args) { | ||
| if (args.length == 2 && sender instanceof Player) { | ||
| Player player = (Player) sender; | ||
| dansplugins.mailboxes.objects.Mailbox mailbox = persistentData.getMailbox(player); | ||
|
|
||
| if (mailbox != null) { | ||
| List<String> messageIds = new ArrayList<>(); | ||
|
|
||
| // Add active message IDs | ||
| for (dansplugins.mailboxes.objects.Message message : mailbox.getActiveMessages()) { | ||
| messageIds.add(String.valueOf(message.getID())); | ||
| } | ||
|
|
||
| // Add archived message IDs | ||
| for (dansplugins.mailboxes.objects.Message message : mailbox.getArchivedMessages()) { | ||
| messageIds.add(String.valueOf(message.getID())); | ||
| } | ||
|
|
||
| return filterCompletions(messageIds, args[1]); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getMessageIdCompletions method can add duplicate message IDs if a message with the same ID exists in both active and archived lists. While this might be unlikely based on the design, it's safer to use a Set or check for duplicates before adding to ensure each message ID appears only once in the tab completion suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by using a Set to deduplicate message IDs in all command classes (OpenCommand, DeleteCommand, ArchiveCommand). Commit: 61483dd
| private List<String> getSendCompletions(CommandSender sender, String[] args) { | ||
| if (args.length == 2) { | ||
| // Player names (both online and offline) only at position 2 | ||
| List<String> playerNames = new ArrayList<>(); | ||
|
|
||
| // Add online players | ||
| for (Player player : Bukkit.getOnlinePlayers()) { | ||
| playerNames.add(player.getName()); | ||
| } | ||
|
|
||
| // Add offline players | ||
| for (OfflinePlayer player : Bukkit.getOfflinePlayers()) { | ||
| if (player.getName() != null) { | ||
| playerNames.add(player.getName()); | ||
| } | ||
| } | ||
|
|
||
| return filterCompletions(playerNames, args[1]); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getSendCompletions method adds both online and offline players to the completion list without checking for duplicates. Online players will appear in both getOnlinePlayers() and getOfflinePlayers(), resulting in duplicate entries in the tab completion suggestions. Consider using a Set to store player names to automatically deduplicate, or check if a name already exists before adding offline players.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by using a Set to deduplicate player names in SendCommand. Online players who also appear in the offline list are now automatically deduplicated. Commit: 61483dd
| // Add offline players | ||
| for (OfflinePlayer player : Bukkit.getOfflinePlayers()) { | ||
| if (player.getName() != null) { | ||
| playerNames.add(player.getName()); | ||
| } | ||
| } |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling Bukkit.getOfflinePlayers() on every tab completion can cause significant performance issues, especially on servers with a large player history. This method returns all players who have ever joined the server and can be very slow. Consider caching the offline player list with a reasonable expiration time, or limiting the results to only recent players, or using a more efficient player lookup mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tab completion logic now lives in SendCommand.getTabCompletions(). While getOfflinePlayers() can still be slow, this matches the actual command behavior (SendCommand uses UUIDChecker which also calls getOfflinePlayers). The performance concern is documented and would require architectural changes beyond tab completion (e.g., implementing a player cache in UUIDChecker). Commit: 61483dd
|
@copilot Fields and methods related to tab completions should live in the command classes that they are associated with. |
Co-authored-by: dmccoystephenson <[email protected]>
Moved tab completion methods to individual command classes. Each command now has its own getTabCompletions() method, and Mailboxes.java delegates to these methods. Config options and constants moved to ConfigCommand, player name completion to SendCommand, message ID completion to OpenCommand/DeleteCommand/ArchiveCommand, etc. Commit: 61483dd Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: dmccoystephenson <[email protected]>
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.