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

Vfill widget #403

Merged
merged 15 commits into from
Apr 4, 2022
Merged

Vfill widget #403

merged 15 commits into from
Apr 4, 2022

Conversation

ppizarror
Copy link
Owner

@codecov
Copy link

codecov bot commented Apr 2, 2022

Codecov Report

Merging #403 (1678b13) into master (9c09143) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
+ Coverage   97.23%   97.24%   +0.01%     
==========================================
  Files          48       49       +1     
  Lines       12104    12155      +51     
==========================================
+ Hits        11769    11820      +51     
  Misses        335      335              
Impacted Files Coverage Δ
pygame_menu/__init__.py 94.87% <ø> (ø)
pygame_menu/widgets/__init__.py 100.00% <ø> (ø)
pygame_menu/widgets/widget/none.py 100.00% <ø> (ø)
pygame_menu/_widgetmanager.py 99.00% <100.00%> (+<0.01%) ⬆️
pygame_menu/menu.py 96.58% <100.00%> (+<0.01%) ⬆️
pygame_menu/widgets/core/widget.py 97.76% <100.00%> (+0.01%) ⬆️
pygame_menu/widgets/widget/__init__.py 100.00% <100.00%> (ø)
pygame_menu/widgets/widget/hmargin.py 100.00% <100.00%> (ø)
pygame_menu/widgets/widget/vfill.py 100.00% <100.00%> (ø)
pygame_menu/widgets/widget/vmargin.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c09143...1678b13. Read the comment docs.

@vnmabus
Copy link
Contributor

vnmabus commented Apr 3, 2022

Can you merge the master into this branch, so I can try the code with the border size improvements?

@ppizarror
Copy link
Owner Author

Can you merge the master into this branch, so I can try the code with the border size improvements?

Done 💯

@vnmabus
Copy link
Contributor

vnmabus commented Apr 3, 2022

It seems that the width of VFill is the same as the width of the original menu. In my case, I adjust the width of the menu to the size of the widgets after they are all created. This worked before no matter what the initial size of the menu was, but if I add VFill widgets, I have to set the original width to 1 (0 is not allowed), otherwise the original width is kept.

I think that vertical alignment widgets should always have 0 width (or 1 if that is not supported). I am not sure if that affects other parts of the library.

@ppizarror
Copy link
Owner Author

ppizarror commented Apr 3, 2022

Are you resizing the widgets? Because in tests, the following (without any further change) is true:

menu = MenuUtils.generic_menu()
b = menu.add.button('nice')
vf1 = menu.add.vertical_fill()
self.assertEqual(vf1.get_width(), 0)

@vnmabus
Copy link
Contributor

vnmabus commented Apr 3, 2022

Hum, if I print the size of each widget I obtain:

0
196
0
222
0
222
0
222
0
222
0
196
0
248
0
170
0

where the 0's are the VFill widgets, as expected.

However, menu.get_size(widget=True) returns (625, 717)! How can that be?

@ppizarror
Copy link
Owner Author

ppizarror commented Apr 3, 2022

Can you provide a minimal working example? The following works as expected, thus, I cannot reproduce your issue:

menu = MenuUtils.generic_menu()
b = menu.add.button('nice')  # Add button
self.assertEqual(menu.get_size(widget=True), b.get_size())

# Now add 1 vfill, this should use all available height
vf1 = menu.add.vertical_fill()
self.assertEqual(vf1.get_width(), 0)
self.assertEqual(menu.get_size(widget=True), (b.get_width(), b.get_height() + vf1.get_height()))

@ppizarror
Copy link
Owner Author

Hum, if I print the size of each widget I obtain:

0
196
0
222
0
222
0
222
0
222
0
196
0
248
0
170
0

where the 0's are the VFill widgets, as expected.

However, menu.get_size(widget=True) returns (625, 717)! How can that be?

@vnmabus are you using a column/row layout?

@vnmabus
Copy link
Contributor

vnmabus commented Apr 3, 2022

@vnmabus are you using a column/row layout?

Nope.

@ppizarror
Copy link
Owner Author

@vnmabus are you using a column/row layout?

Nope.

Then I need an example. Maybe this widget introduces a bug, or you have spotted a new bug

@vnmabus
Copy link
Contributor

vnmabus commented Apr 3, 2022

The problem DOES NOT OCCUR if I remove widget_alignment=pygame_menu.locals.ALIGN_LEFT from the theme. I hope this clarify something for you.

@vnmabus
Copy link
Contributor

vnmabus commented Apr 3, 2022

So, this is a minimal working example:

import pygame
import pygame_menu


pygame.init()
surface = pygame.display.set_mode((800, 600))

theme = pygame_menu.Theme(
    widget_alignment=pygame_menu.locals.ALIGN_LEFT,
)

menu = pygame_menu.Menu(
    "",
    800,
    600,
    center_content=True,
    theme=theme,
)
# Now add 1 vfill, this should use all available height
vf1 = menu.add.vertical_fill()
print(menu.get_size(widget=True))

b = menu.add.button('nice')  # Add button
print(menu.get_size(widget=True), b.get_size())

# Now add 1 vfill, this should use all available height
vf2 = menu.add.vertical_fill()
print(menu.get_size(widget=True), b.get_size())

@ppizarror
Copy link
Owner Author

ppizarror commented Apr 4, 2022

@vnmabus I've fixed the issue (and added the tests!). The problem was I overwrote NoneWidget.set_alignment, so VFill (and others) had an incorrect alignment; thus, when calculating the maximum and minimum x position, it assumed it was centered and used the whole container.

Let me know your thoughts 😎 . Greetings!

@vnmabus
Copy link
Contributor

vnmabus commented Apr 4, 2022

This fixes it, thanks!!

@ppizarror
Copy link
Owner Author

This fixes it, thanks!!

I'll merge to master then. Let me know about anything else. If you need, I can publish a new PyPI version

@ppizarror ppizarror merged commit 5709842 into master Apr 4, 2022
@ppizarror ppizarror deleted the vfill-widget branch April 4, 2022 00:58
@vnmabus
Copy link
Contributor

vnmabus commented Apr 9, 2022

Can you publish in PyPI? I have to make a PR and need to upgrade the dependency to your package.

@ppizarror
Copy link
Owner Author

Can you publish in PyPI? I have to make a PR and need to upgrade the dependency to your package.

Done! See https://github.com/ppizarror/pygame-menu/releases/tag/4.2.7, https://pypi.org/project/pygame-menu/

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.

2 participants