Add support for pagination to the PetShop by implementing the feature abstractly on the IconMenu

[BlvckBytes]

Greetings!

I am currently helping out a rather comfortably-sized server by providing voluntary technical support, and we actually ran into the hard-limit of 54 maximum slots on your PetShop-UI. Since the owner has kindly asked me to try to add pagination, so that more entries become accessible, I took a stab at it; please do not view this PR as necessarily final - I just want to roughly outline my idea, get some feedback on it, and check whether we can lift this artificial limitation together.

First of all, I do not see a reason as to why loading pets as defined in the YAML into the PetShop needs to be constrained to any number of slots; this map is either only queried, or applied to the IconMenu, which already has a protection against "out-of-range" slots itself; thus, I removed this check, and just let the Queue of filler-items empty till' completion.

Secondly, I tried to go about adding the actual feature of pagination as centrally and as non-invasively as possible, as to not needlessly disrupt the codebase. By calling IconMenu#setPaginationIdentifier with a valid key-part, pagination-mode will be entered by setting pageSizeInSlots to a non-null value, if applicable. Once entered, IconMenu#getSize will now always respond with a constant value - namely the total page size. IconMenu#getOption as well as the click-handler now merely translate slot-offsets, as to provide access to elements on the current page, while the former also renders out the navigation-items, and ensures an otherwise vacant last row.

All values are configurable, with the following schema (using representative values):

config.yml:

MyPet:
  Pagination:
    <identifier>:
      TotalRows: 6
      PreviousPage:
        Type: ARROW
        Title: '&bVorherige Seite'
        Lore:
        - ' '
        - '&7Aktuell auf {currentPage}&8/&7{numberOfPages}'
      NextPage:
        Type: ARROW
        Title: '&bNächste Seite'
        Lore:
        - ' '
        - '&7Aktuell auf {currentPage}&8/&7{numberOfPages}'

We really appreciate the work you have put into this plugin and would like to continue using it! As to provide a seamless update-experience, it would greatly help to have these changes merged, so I do not need to maintain my own fork; I also believe that many other users could benefit from such a change, no matter how insignificant it may seem at first glance.

Please excuse the possible cases of me being ignorant to existing internal utilities and having rolled my own, as I am not yet familiar with the majority of your project.

Thank you for your time in advance! :)

Best regards,
BlvckBytes

[Jakllp]

I started something like this once but it wasn't at all configurable so this is nice.

I'll take a look at it at some point (might be a bit... End of the semester and all that) but thanks a lot :)

[Jakllp]

One quick question tho (without looking at the code yet...):
Did you check that this doesn't clash with /petswitch - for... reasons that have been there since before I took over this plugin /petswitch actually has pages
MAYBE we could even just use this system for that as well

[BlvckBytes]

Oh wow, I didn't actually expect a response, let alone this quickly! :) Many developers seem to ghost their users nowadays.

Well, usually I go to town in terms of configurability on my own plugins, but then again, I have written my own mapper, parser and pre-processor; in this situation, I really just went with an implementation based on first principles, relying only on bukkit's framework. Thank you for your kind words! :)

The basic thought was all about keeping it generic - I can enable each instance of IconMenu to perform pagination by merely calling a single function on it, providing the config's identifier. As far as I can judge from a quick glance, MyPetSelectionGui - as employed on the CommandSwitch - wraps yet another instance of IconMenu; I also do not get any pagination in-game. The UI simply grows dynamically, and the hard-limit of 45 entries seems to be present over there just as well.

The only occurrence of an attempt at pagination I could find was MyPetAdminSelectionGui, which is actually unused, besides another unused import-statement which refers to it.

Feel free to take your time - we're all set for now. Just wanted to make this mental note for the both of us.

// Edit:

Whoops! I definitely shouldn't have read that class in a hurry. You're right, there's actually pagination-logic included in the aforementioned wrapper. One could easily adapt this to my solution, as to unify the system a bit. But as it stands, my alterations will not cause a clash - at least I don't think. I will try and store a bunch of pets real quick!

[Jakllp]

Didn't read that edit in time for my last comment (which comment).
Also: Hello neighbor (German-Dev here)

As for storing pets: /petadmin create will allow you to easily go over the default limit

[BlvckBytes]

Sei mir gegrüßt!

Hätte ich den Admin-Befehl bloß vorher gekannt - hab' mir die Finger im UI wund geklickt, :,). Habe das Plugin selbst tatsächlich noch nicht großartig benutzt - bin aber auch selbst schuld, wenn ich die Doks nicht lese, xD.

I was contemplating on whether I will start this conversation out using the German language, as I saw your country of residence on your profile, but I figured that if other people would be interested in joining in as well, I do not want to prevent them from doing so; hope that's okay with you!

I just added another commit, migrating the pagination to my proposed solution. As you can see, a lot of duplicate code can be saved. My solution does not - at this very moment - hide the buttons if there's just a single page, but that would be a simple addition (would add a flag to configure this behavior too though, as I would prefer a static navigation). But as it stood, my modifications did not clash with this custom pagination-implementation either - just thought I let you know.

[Jakllp]

Thanks!

I just saw the way you implemented the config stuff - I'll definitely change that before merging (mypet has a central config system - but you really don't have to do that yourself)

Also the strings in the config kinda conflict the current locale system so I'd have to think about how to implement that (probably just use less words and just... Arrows or something or add strings to the translation... We'll see)

Glad you got MyPet to compile (some people get stuck with errors bc of the github maven repo)
And English is basically the way we support the plugin (except for the german channels over on the Discord)

[BlvckBytes]

I did not know about your config-system, sorry! As I've said, it was a quick proof-of-concept, and I'm happy to rework it; it is me, after all, who is requesting this feature.

I did not add any configuration-values yet, as the plugin just falls back to standard behavior without the keys present. They were just copied from the file we actually use on the server. If you were referring to the keys, I would be fine with you tuning their naming-scheme to your liking - I just considered them to be self-documenting and easy to understand.

Oh yeah, the first few minutes had me wondering too, believe me. Seems like one needs to provide a bearer-token to pull the dependencies from GitHub. I avoided the headache altogether by simply cloning- and installing the repositories manually. I have had my fair share of sleepless hours over the mess that is Maven, ;).

[BlvckBytes]

I believe I now understood how - at least from a higher point of view - your locale-system works; translations are maintained via crowdin and kept as files in a separate repository, which is then "shaded" into the final JAR-file during the build-process, using the scm-plugin. The main utility, providing access to translations, is called de.Keyle.MyPet.api.util.locale.Translation.

While I could easily adapt my proposal to this system, I am a bit indecisive about where I would actually put the keys; they aren't necessarily messages, nor names - maybe one could introduce a new category called Icons? Also, I guess TotalRows and Type would then have to be separated out from the translatable, textual values too.

To sum it up, I'd most definitely need a bit of assistance on this part, as I do not possess the required insights, I'm afraid. Would be cool though, if this little feature would become publicly implemented! :) As I've said though, there's absolutely no hurry; we're all set in the meantime. I will also join your Discord, just in case.