Skip to content

Added functionality for negative numbers in keyboard adjust. #1522

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.

Sign up for GitHub

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

Jump to bottom
Open
N0rmalUser wants to merge 4 commits into
base: dev-3.x
Choose a base branch
from
Open

Added functionality for negative numbers in keyboard adjust. #1522

N0rmalUser wants to merge 4 commits into from

Conversation

@N0rmalUser
Copy link

@N0rmalUser N0rmalUser commented Jun 22, 2024 •
edited
Loading

Description

Added functionality for negative numbers in keyboard adjust.

Type of change

If the function gets a negative number in adjust, it applies them to the lower buttons in the same way that positive numbers affect the upper buttons. Negative numbers have a higher priority in formatting.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

def test_markup():
    builder = InlineKeyboardBuilder()
    for i in range(12):
        builder.button(text=f"test{i}", callback_data=f"test{i}")
    builder.adjust(3, 1, -2)
    return builder.as_markup()

@router.message(Command('test'))
async def test_handler(msg: Message) -> None:
    await msg.answer("This is TEST!!!", reply_markup=test_markup())

2024-06-22_05-51-48

def test_markup():
    builder = InlineKeyboardBuilder()
    for i in range(1):
        builder.button(text=f"test{i}", callback_data=f"test{i}")
    builder.adjust(-2)
    return builder.as_markup()

2024-06-22_06-00-31

def test_markup():
    builder = ReplyKeyboardBuilder()
    builder.row(*(KeyboardButton(text=f"test-{index}") for index in range(6)))
    builder.button(text="Add", callback_data="add")
    builder.button(text="Cancel", callback_data="cancel")
    builder.adjust(3, -1, -1)
    return builder.as_markup()

2024-06-22_05-56-57

Test Configuration:

  • Operating System: Windows 10
  • Python version: 3.11.9

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@github-actions github-actions bot added the 3.x Issue or PR for stable 3.x version label Jun 22, 2024
@github-actions
Copy link

github-actions bot commented Jun 22, 2024 •
edited
Loading

✔️ Changelog found.

Thank you for adding a description of the changes

JrooTJunior
JrooTJunior requested changes Jul 15, 2024
View reviewed changes
Copy link
Member

@JrooTJunior JrooTJunior left a comment

Choose a reason for hiding this comment

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

This feature is very cool, I thought about such functionality for a long time (about 1.5 year ago) and it's in my deep backlog with low priority, thanks for trying to implement it.

We currently have an alternative way to do this using a combination of constructors (added in v3.0.0rc1 #1236), but it's not the same as what you're doing here.

So, regarding your solution, I think the design you chose is a little strange because it's not intuitive. I have another idea of ​​what it might look like:

builder = InlineKeyboardBuilder()
# ... add buttons

builder.adjust(2, 3, ..., 5)

In this example, ... (circular object) is used as a separator between parts, it should be specified only once per configuration and means that the left part of the values ​​should be used for the first part of the markup, and then the right part should be used for the last part of the markup.
Technically, we can find the position of the circular object in the size list and split the positions for the first and last pieces by that configuration.

I think this is more intuitive than negative values.

Let's discuss it.

aiogram/utils/keyboard.py
return self

def adjust(self, *sizes: int, repeat: bool = False) -> "KeyboardBuilder[ButtonType]":
def _markup_constructor(self, sizes: list, buttons: list, repeat: bool) -> list:
Copy link
Member

@JrooTJunior JrooTJunior Jul 14, 2024

Choose a reason for hiding this comment

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

typehint list is not correct:

  1. You must specify what should actually be on this list as it is a generic type
  2. We still support Python 3.8, but PEP 585 was added in Python 3.9, so you should use List[T] from the typing module instead of list[T]

aiogram/utils/keyboard.py
:param repeat:
:return:
"""
if sizes:
Copy link
Member

@JrooTJunior JrooTJunior Jul 14, 2024

Choose a reason for hiding this comment

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

This solution means we can mix positive and negative values ​​in the size configuration (e.g. 2, -2, 3, -5) and it's a strange and unexpected format, so that too needs to be taken into account and limit the movement of positive and negative values.

CHANGES/1522.feature.rst
@@ -0,0 +1,28 @@
====
Copy link
Member

@JrooTJunior JrooTJunior Jul 14, 2024

Choose a reason for hiding this comment

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

In reStructuredText format, it is necessary to underline the headings as long as the heading itself.

There is also the headings should not be used inside changelog, especially first-level headers, as this will break the semantics of the change history page.

JrooTJunior
JrooTJunior reviewed Jul 15, 2024
View reviewed changes
tests/test_utils/test_keyboard.py
[12, True, [3, -1, -4], [3, 3, 1, 1, 4]],
],
)
def test_adjust(self, count, repeat, sizes, shape):
Copy link
Member

@JrooTJunior JrooTJunior Jul 15, 2024

Choose a reason for hiding this comment

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

What if we have fewer buttons than the total of all sizes?

@JrooTJunior JrooTJunior requested a review from Copilot April 5, 2025 19:12
Copilot AI reviewed Apr 5, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • CHANGES/1522.feature.rst: Language not supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JrooTJunior JrooTJunior JrooTJunior requested changes

Copilot code review Copilot Copilot left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@N0rmalUser N0rmalUser

Labels

3.x Issue or PR for stable 3.x version

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@N0rmalUser @JrooTJunior