[Feature] MessagePaginator: Paginator-alike Menu but for raw Messages #58
Conversation
It's a copy-paste-replace of Paginator but it's useful nevertheless.
|
|
||
| /** | ||
| * Sets the {@link java.util.function.Consumer Consumer} to perform if the | ||
| * {@link com.jagrosh.jdautilities.menu.Paginator Paginator} times out. |
There was a problem hiding this comment.
Documentation can be partially copied from existing menus of the same structure, however this and all other references to Paginator should be modified to respect the new entity.
IE: Paginator -> MessagePaginator
|
|
||
| private Message renderPage(int pageNum) | ||
| { | ||
| return messageFunction.apply(pageNum, pages); |
There was a problem hiding this comment.
While I do think the idea is good theoretically, I feel like this should be replaced with an index ordered collection of Message instances.
My worry is that by making the provision based on a function, people will opt to generate a new Message instance on each call.
I am not opposed to this, but I'd like @jagrosh's opinion on it before we consider this functionality appropriate to merge.
There was a problem hiding this comment.
I'm basically generating 1 embed per render currently. How about both approaches?
There was a problem hiding this comment.
My setup:
with(MessagePaginator.Builder()) {
setEventWaiter(Bot.eventWaiter)
allowTextInput(true)
setItems(pages.size)
setLeftRightText("previous", "next")
setEmbedFunction { p, _ -> pages[p - 1].onHelp(event) }
build()
}.paginate(event.channel, page)pages is a List<HelpFactory>, a class that creates MessageEmbeds as required.
There was a problem hiding this comment.
while I also agree to the Collection idea, atm you can lazy-load embeds as wanted. And let's be honest, it's just a few CPU cycles to generate vs RAM (Imagine 1000 users parsing a Wikipedia page into 20 parts each and all Message and MessageEmbeds loaded, while a lot of users just stop reading in page 4)
There was a problem hiding this comment.
Theoretically you could externally load them into a collection and poll them yes.
I am only raising the concern for the sake of discussing it, as it is a design decision to make.
Also I approve of the Kotlin ❤️
There was a problem hiding this comment.
Keeping the function is fine, but it would be wise to just render them once on init and store them in a list instead of calling the function every time to render
Shengaero
added
Proposal
Shengaero
changed the title
Shengaero
added
Version: 2.2
and removed
Proposal
| import java.util.function.Consumer; | ||
|
|
||
| /** | ||
| * A {@link com.jagrosh.jdautilities.menu.Menu Menu} implementation that paginates a set of one or more pages. |
There was a problem hiding this comment.
Would benefit from an addition like "using a custom function for generating the Message(Embed) objects to show"
|
|
||
| private Message renderPage(int pageNum) | ||
| { | ||
| return messageFunction.apply(pageNum, pages); |
There was a problem hiding this comment.
Keeping the function is fine, but it would be wise to just render them once on init and store them in a list instead of calling the function every time to render
| { | ||
| private BiFunction<Integer,Integer,Message> function = (page, pages) -> null; | ||
| private Consumer<Message> finalAction = m -> m.delete().queue(); | ||
| private int items = 1; |
There was a problem hiding this comment.
This is actually the amount of pages as confirmed by variable name and usage of the actual Menu implementation. Therefore renaming this variable would be wise to reflect that.
| public MessagePaginator build() | ||
| { | ||
| Checks.check(waiter != null, "Must set an EventWaiter"); | ||
| Checks.check(function != null, "Must include at least one item to paginate"); |
There was a problem hiding this comment.
This warning is a bit off, as it checks if a function is present rather than amount of items
| } | ||
|
|
||
| /** | ||
| * Sets the number of items that will appear on each page. |
There was a problem hiding this comment.
This sets the page count, and not the amount of items per page
| * | ||
| * @return This builder | ||
| */ | ||
| public Builder setItems(int num) |
There was a problem hiding this comment.
Should be renamed to something like setPageCount(int) for more clarity
| * | ||
| * @return This builder | ||
| */ | ||
| public Builder showPageNumbers(boolean show) |
There was a problem hiding this comment.
This is NOP and should be removed if unused
| * | ||
| * @return This builder | ||
| */ | ||
| public Builder useNumberedItems(boolean number) |
There was a problem hiding this comment.
This is NOP and should be removed if unused
|
Not gonna do any further contributions on this PR. I moved on to use another Discord library. |
It's a copy-paste-replace of Paginator but it's useful nevertheless.