-
Notifications
You must be signed in to change notification settings - Fork 303
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
Revamp content display API #2693
Conversation
src/main/java/com/onarandombox/MultiverseCore/commands/GamerulesCommand.java
Outdated
Show resolved
Hide resolved
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.
Some notes for now. I'll have to look again later. Heading to bed.
src/main/java/com/onarandombox/MultiverseCore/commands/ListCommand.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
private Collection<String> getListContents(@NotNull CommandSender sender) { | ||
Player player = (sender instanceof Player) ? (Player) sender : null; | ||
private ContentParser newWorldListContentParser() { |
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.
I think that this might be more readable as a separate class. It could just be a private static one in this class maybe?
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.
yea can change that. Probably use a singleton instance?
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.
hmm dont really want do use static class so I don't have to pass in the plugin
instance to the private class, so current just doing something like private class WorldListContentParser implements ContentParser {
and then .addContentParser(new WorldListContentParser());
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.
What's wrong with passing the plugin instance into the new class's constructor?
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.
tbh nothing wrong, but then I can't really use a singleton instance anyways, as not a good idea to store plugin objects in static like this right? Like if a new instance plugin instance is created, it may cause issues? So I just decided to make it inner class nonstatic so it uses the instance from the parent class.
* @param sender The target which the content will be displayed to. | ||
* @param content The content to display. | ||
*/ | ||
void parse(@NotNull CommandSender sender, @NotNull List<String> content); |
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.
I think this might be better if it returns the content list instead of is given one to add to. I think this is just a gut feeling and maybe isn't entirely right but I feel like a list you're given to modify smells a bit more than just returning a list.
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 reason why this doesn't just return a list of strings so we only need one common list. Instead of each parsing creating a new list as then adding it to another common list... which is potentially is a lot of lists.
For whether if we delete/modify existing entries from other parsers, either we allow it as a feature, or just instead pass a wrapper around the List that only allow adding of elements.
But all these won't be needed if we decide to only allow one content parser.
src/main/java/com/onarandombox/MultiverseCore/display/ContentDisplay.java
Show resolved
Hide resolved
public interface ContentFilter { | ||
/** | ||
* Checks if a particular string should be displayed. | ||
* | ||
* @param value String to check on. | ||
* @return True if should be display, false otherwise. | ||
*/ | ||
boolean checkMatch(String value); | ||
|
||
/** | ||
* Gets whether content needs to be filtered by this filter. | ||
* | ||
* @return True if content should be filtered, false otherwise. | ||
*/ | ||
boolean needToFilter(); | ||
} |
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.
First glance, this seems like it could easily be a single method, functional interface. If the need to filter is false, then the checkMatch should just return true. In that case, you could also just use the built in Predicate interface.
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.
Yea, then reason I d have the second method, is so that there is no need to loop through the content to filter is there nothing to filter. And when displaying to user, it output whether or not filter was applied to the content.
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.
Example use cases:
// Only send output of filter string used if needed.
if (filter.needToFilter()) {
sender.sendMessage(String.format("%s[Filter '%s']", ChatColor.GRAY, filter));
}
// Only loop to filter if needed, to save performance
if (filter.needToFilter()) {
return content.stream().filter(filter::checkMatch).collect(Collectors.toList());
}
return content;
src/main/java/com/onarandombox/MultiverseCore/display/handlers/SendHandler.java
Show resolved
Hide resolved
* Use singleton pattern for DefaultContentFilter with getInstance method. * Have a default SendHandler. * Don't need streams for small dataset. * Private WorldListContentParser class to improve readability.
implemented in #2824 |
improve based on @dumptruckman comments
content display API changes
Other additions