-
Notifications
You must be signed in to change notification settings - Fork 31
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
Allow to hide information about pages in the menu #471
base: master
Are you sure you want to change the base?
Allow to hide information about pages in the menu #471
Conversation
else: | ||
buffer = '' | ||
|
||
buffer += '\n' |
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.
That whole block looks quite logically redundant to me. Couldn't we just extend buffer
as we go? For example (untested):
buffer = ''
if self.title:
buffer += _translate_text(self.title, player_index)
if self.show_pages:
buffer += f' [{page.index + 1}/{self.page_count}]'
buffer = buffer.strip() + '\n'
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.
Yes, I agree. Changed and tested the code, everything works fine.
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 would like to suggest the following changes:
buffer = ''
if self.title:
buffer += _translate_text(self.title, player_index)
if self.show_pages:
if buffer:
buffer += ' '
buffer += f'[{page.index + 1}/{self.page_count}]'
if buffer:
buffer += '\n'
For 2 reasons:
- We don't want to add a leading space if there is no title.
- We don't want to add a new line if there is no title and no pagination.
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.
Done
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.
Works great, thank you! Ideally, we should also add the same features to ESC menus for consistency.
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.
Added support for "ListESCMenu" and "PagedESCMenu"
P.S. This type of menu works in "dod" as well
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.
Also, I noticed a bug that the "ESC" menu does not close if you click the "Close" button (bottom right) or the cross (top right). Is it normal?
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 am planning to add translation for the "Back", "Next" and "Close" buttons. Or make them language-neutral (at least), such as "<", ">" and "X", but this will be in a separate pull request.
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.
Also, I noticed a bug that the "ESC" menu does not close if you click the "Close" button (bottom right) or the cross (top right). Is it normal?
I'm not entirely sure, but I think the client does not tell the server when the menu is closed via these buttons, so it is getting resent until a selection is actually made.
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.
Maybe consider these two buttons as selection "0" (Close)?
Do not add a leading space if there is no title. Do not add a new line if there is no title and no pagination.
Sometimes it is not necessary to show navigation, for example, in the admin menu (as in SM)