Things to the API patch#359
Conversation
hugmanrique
left a comment
hugmanrique
left a comment
There was a problem hiding this comment.
I like the additions of sendActionBar and sendTitle, but the other stuff would require too much maintenance + it should be in another separate PR (and patch)
| * @param message the message to send | ||
| */ | ||
| - @Deprecated | ||
| public void sendMessage(String message); |
There was a problem hiding this comment.
This method is deprecated for valid reasons. Legacy texts are converted to text components, but you should always use the latter.
There was a problem hiding this comment.
Then why md_5 doesn't specify it in javadocs as '@ deprecated' ?!
There was a problem hiding this comment.
You can already see it's deprecated from the annotation on the method; he probably felt it wasn't necessary to duplicate that line.
There was a problem hiding this comment.
Honestly, Deprecating string methods is stupid, as it serves a purpose to send a simple message.
One could argue the legacy codes support of this message are 'deprecated' and that the method isn't, but it's also possible to permanently support that system on the API, which I'm supportive of, as components are stupid for simple messaging.
I'm in favor of permanently keeping legacy codes around for simple messaging, and if mojang adds more advanced capabilities than current with components, then the legacy system just wouldn't expose it and people can move to comps then.
But even a sendMessage(ChatColor color, String msg); that doesn't use legacy codes would be useful to declare simple messages, but I see no harm in keeping legacy codes on sendMessage.
If md5 removed it from Bukkit API, I would add it back. It costs nothing to maintain.
| * | ||
| * @param messages the messages to send | ||
| */ | ||
| - @Deprecated |
| + * @deprecated This is unlikely the API you want to use. See {@link #sendActionBar(String)} or {@link #sendActionBar(BaseComponent)}, {@link #sendActionBar(BaseComponent...)} for a more proper ActionBar API. | ||
| */ | ||
| + @Deprecated // Waterfall | ||
| public void sendMessage(ChatMessageType position, BaseComponent... message); |
There was a problem hiding this comment.
This method is perfectly valid, why would we deprecate it?
| + * @deprecated This is unlikely the API you want to use. See {@link #sendActionBar(String)} or {@link #sendActionBar(BaseComponent)}, {@link #sendActionBar(BaseComponent...)} for a more proper ActionBar API. | ||
| */ | ||
| + @Deprecated // Waterfall | ||
| public void sendMessage(ChatMessageType position, BaseComponent message); |
| { | ||
| - proxy.getLogger().log( Level.INFO, "{0} executed command: /{1}", new Object[] | ||
| + // Waterfall - check if the command sender is not the console | ||
| + if ( !sender.getName().equalsIgnoreCase( proxy.getConsole().getName() ) ) |
There was a problem hiding this comment.
Logging console commands might be useful for web terminals/screen/tmux... where multiple people might be watching the same console output
There was a problem hiding this comment.
And generally useful when checking the log file?
| + * @param task the task to run | ||
| + * @param delay the delay before this task will be executed, specified in ticks | ||
| + * @return the scheduled task | ||
| + */ |
There was a problem hiding this comment.
Bungee (and by extension, Waterfall) doesn't have a concept of ticks. This is only used in NMS based servers. You can pass 50 millis if you want to run a task every tick.
There was a problem hiding this comment.
In addition, you should specify either in Javadocs or the method name (or both) that this method accepts ticks. It isn't obvious what unit the delay is in unless you read the implementation code.
| @Override | ||
| public void sendTitle(Title title) | ||
| { | ||
| + this.currentTitle = title; // Waterfall |
There was a problem hiding this comment.
I don't see any current title expiration code
There was a problem hiding this comment.
Heck, we have the fancy sendMessage with chat position. I don't see these sendActionBar function a useful abbreviation. It might even add further confusion in combination with semdMessage with action bar position.
I'd say waterfall should not encourage alternatives to working api in bungee.
And btw. there are 3 message types. When you deprecate the function with the enum and don't add functions with chat and system type difference, its not good.
|
And "Add things to the API" as description is also not ok. What you did here belongs into multiple commits/patches with more descriptive naming. (Apart from most changes I and the previous reviewer think are not good) |
|
Yeah, on a second thought, adding an extra method every time a new chat position is created is not scalable. |
| + * | ||
| + * @param message the message you wish to send | ||
| + */ | ||
| + public void sendActionBar(String message); |
There was a problem hiding this comment.
Add @Deprecated to this method.
|
That's why i didn't wanted to PR, but everyone said 'just PR to waterfall'. I KNEW that it will be rejected... |
|
You need to be able to take feedback. The quality of your submission was low, and people have provided feedback on what was wrong so that you can learn to improve the quality of your contributions. |
6 participants
This patch adds some things to the API. It adds feature to schedule tasks with ticks, and adds some things to the player interface, also removes 2 non reason deprecations.