Skip to content
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

Update Pagination to not pad if only one page exists. #734

Merged
merged 2 commits into from
May 23, 2016
Merged

Update Pagination to not pad if only one page exists. #734

merged 2 commits into from
May 23, 2016

Conversation

dualspiral
Copy link
Contributor

@dualspiral dualspiral commented May 19, 2016

When #639 was merged, some of my pagination broke - see NucleusPowered/Nucleus#158. In this particular case:

  • A one page MOTD which has the potential to have more than one page now fills the entire chat box, which is a waste of chat space when there is only one page to display.
  • I have code that sets the number of lines to display to the console to Integer.MAX_VALUE, so that the console can just see the entire MOTD. However, since merging Pad out each page with empty lines, end pages with next page with ... fixes #638 #639, this causes an out of memory error because it's trying to create a huge list. (While I accept I could lower this value, I still want to use the styling that the Pagination service provides without any padding when I do not know how many lines I have.)

I think the change provided in #639 is a real usability win, but only when there is more than one page, and this breaks what plugins were expecting before. This PR returns the previous behaviour to one page items, while retaining the new behaviour when there is more than one page.

I have a test plugin that will demonstrate the behaviour: https://gist.github.com/dualspiral/067000c4b1fc80407e79bd51cce151d8

The commands are:

  • /pg <numberoflines>, put the word "text" into the pagination list the specified number of times, displaying 20 lines per page.
  • /pglg <numberoflines>, put the phrase "long text text text text text text text text text text text text text text text text text text text text text text text" into the pagination list the specified number of times, 20 lines per page.
  • /pgmax <numberoflines> is the same as /pg, but can have Integer.MAX_VALUE number of lines.

I tested with /pg 1, /pg 21, /pglg 10 and /pgmax 25.

* This was a particular issue if someone sets a large number of lines per page for display on the console, for example.
* If there is only one page of text, do not pad the page.
@ryantheleach
Copy link
Contributor

6:07 AM <@zml> originally console had everything output as one page
6:07 AM <@zml> iirc giving a lines per page of -1 meant unlimited length
6:07 AM <@zml> dualspiral, ryantheleach ^
6:07 AM <@zml> dunno if that's still the case
6:09 AM <ryantheleach> I suspect it might be, and even cancel the padding

@dualspiral
Copy link
Contributor Author

dualspiral commented May 19, 2016

originally console had everything output as one page

It's not any more, which is why I've been using Integer.MAX_VALUE. Others might be too.

iirc giving a lines per page of -1 meant unlimited length

It's not documented, so I wasn't aware of that (though I think I realised that was the case when whipping this up) - it's good to know. However, my other points do still stand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants