Skip to content

Truncate charinfo results#1062

Merged
MarkKoz merged 6 commits into
masterfrom
bug/util/897/truncate-charinfo
Jul 23, 2020
Merged

Truncate charinfo results#1062
MarkKoz merged 6 commits into
masterfrom
bug/util/897/truncate-charinfo

Conversation

@MarkKoz
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz commented Jul 21, 2020

Fixes #897

Just uses the LinePaginator. The raw field is present and has the exact same value on every page. I'm concerned it may be unclear whether the value is only for the current page or for the entire input (the truth is the latter).

bild

I also updated the command to use the new send_denial utility function since the existing code was quite similar to what the helper does. The difference is that send_denial has those comedic titles.

bild

bild

MarkKoz added 2 commits July 21, 2020 13:50
Pagination ensures the results will never go over the char limit for
an embed.

Fixes #897
Fixes BOT-3D
@MarkKoz MarkKoz added t: bug Something isn't working a: frontend Related to output and formatting p: 3 - low Low Priority a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) labels Jul 21, 2020
@MarkKoz MarkKoz requested a review from a team as a code owner July 21, 2020 21:22
@MarkKoz MarkKoz requested review from dementati and manusaurio and removed request for a team July 21, 2020 21:22
@ghost ghost added the needs 2 approvals label Jul 21, 2020
Copy link
Copy Markdown
Contributor

@Numerlor Numerlor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the whole raw message should be fine, at least it should be clear that it's the whole text after switching pages.

Implementation looks fine for the issue, but with the paginator in place, could we use a bigger char limit? Currently it'll only trigger in edge cases like the one in the issue

Copy link
Copy Markdown
Contributor

@kosayoda kosayoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Now that we paginate the characters, can we reduce the number of lines per page so the embed doesn't get chunky? We could also do with an increase with a number of characters allowed, I don't mind it being 10 lines per page, with a 50 character max.

Comment thread bot/cogs/utils.py Outdated
@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 1 approval labels Jul 23, 2020
Since the raw field is displayed on every page, but pages are
incomplete, it may be unclear whether the field's value is for the
current page or for all pages.

Co-authored-by: Kieran Siek <kieransiek@protonmail.com>
@ghost ghost added needs 1 approval and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jul 23, 2020
Pagination means more characters can be supported without cluttering
anything. It also means infinite lines, so there's no longer a need to
squeeze out the most from a single page. Reducing the line limit leads
to a smaller, tidier presentation.
@MarkKoz
Copy link
Copy Markdown
Contributor Author

MarkKoz commented Jul 23, 2020

could we use a bigger char limit

I don't mind it being 10 lines per page, with a 50 character max.

I was tempted but I ended up waiting for someone else to suggest it 👍

@MarkKoz MarkKoz requested a review from kosayoda July 23, 2020 06:39
Comment thread bot/cogs/utils.py Outdated
Co-authored-by: Kieran Siek <kieransiek@protonmail.com>
Copy link
Copy Markdown
Contributor

@kosayoda kosayoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naisu

@ghost ghost removed the needs 1 approval label Jul 23, 2020
@MarkKoz MarkKoz merged commit 0ee8a47 into master Jul 23, 2020
@MarkKoz MarkKoz deleted the bug/util/897/truncate-charinfo branch July 23, 2020 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: frontend Related to output and formatting a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 3 - low Low Priority t: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Truncate the charinfo embed to fit within 2048 characters

3 participants