Skip to content

Divider lines for GirdLayout #48

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

Merged
merged 4 commits into from
Sep 1, 2021
Merged

Conversation

FoamyGuy
Copy link
Contributor

@FoamyGuy FoamyGuy commented Aug 29, 2021

Resolves: #44

This update includes configuration options for GridLayout to allow divider lines to be drawn within the grid. It supports a simple True / False to enable or disable the lines and optional extra configurations to specify lines only on certain rows or columns.

There is a new example to illustrate this functionality. Here is how it looks when run on a PyPortal:
image

The top grid simply turns divider lines on and uses the default configuration to draw them around every cell.

The lower grid uses the extra configurations to draw divider lines before rows with indexes 1 and 2 and draws no vertical divider lines.

@FoamyGuy FoamyGuy requested a review from a team August 29, 2021 21:10
@kattni
Copy link
Contributor

kattni commented Aug 30, 2021

Thank you so much for adding this! I'll test it on the project that caused me to request this in the first place, either today or tomorrow.

@kattni kattni self-requested a review August 30, 2021 15:54
Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

This is perfect! Exactly what I was looking for. Works great on the MacroPad project I'm still working on - I was able to eliminate 6 lines of vectorio and vectorio-related code.

I noticed that if I enable divider_lines in my example, in which the grid spans the entire display, the vertical line on the right side, and the horizontal line across the bottom do not display on my screen. Let me know if this is intentional. If this is unclear, I can provide my code.

I also have a question below about one of the features.

cell_padding=4,
divider_lines=True,
h_divider_line_rows=(1, 2), # Lines before rows 1 and 2
v_divider_line_cols=(), # No vertical divider lines
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be required if you specify h_divider_lines_rows. Can you clarify the reasoning behind this? I'm wondering if it should default to none, instead of defaulting to having lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to break the API down into two main use cases, the simplest being on/off for divider lines to get drawn at all. The user can enable them by passing divider_lines=True in the constructor. This will cause it to draw all available lines around every cell.

The second and slightly more advanced use case is the user wants to specify which cells and in which orientation to draw lines some of the lines. In this case with the current API the passes divider_lines=True and then passes the desired configurations with the h_divider_line_rows and v_divider_line_cols arguments. If either of those arguments is omitted then it reverts to the standard divider_lines=True behavior which is to draw all available lines. So currently passing the empty tuple is necessary in order to specify that it should not draw any lines for the given orientation.

Maybe it would be simpler if the user passes either h_divider_line_rows or v_divider_line_cols then they do not need to also pass divider_lines=True I think that would allow the logic to work opposite how it does currently. It could start from drawing no lines, and only draw the ones specified by h_divider_line_rows and v_divider_line_cols since divider_lines=True would not be passed it would not draw all available lines if either of those is omitted.

I can look into this further and work on those changes tonight.

It seems I may have unintentionally changed the positioning behavior with this PR as well. I noticed just now that running the example in the MacroPad repo results in the labels getting shifted further down than they should be and causing the bottom row to get drawn half way off of the screen. I will try to resolve this issue when I work on it as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the shift and forgot to mention it! I didn't even think about the fact that it was an inadvertent change. Indeed, I had to alter my y position by 7 pixels to get everything back in position.

@FoamyGuy
Copy link
Contributor Author

@kattni if you can provide your code that will help me understand how the lines are getting drawn. It may be the case that the divider lines get drawn 1 pixel outside of the content cells so in order to span the entire screen and have the outer border visible the total size for the GridLayout might need to be a few pixels smaller than the size of the screen. I'm not certain if this the case or not though. I can look into it and if so perhaps try to change it so that the outer divider lines will get drawn on the exact edges based on the total size of the GridLayout.

@kattni
Copy link
Contributor

kattni commented Aug 31, 2021

@FoamyGuy I had to create a zip - GitHub still doesn't support .py files being uploaded. This code requires a MacroPad, and the included shortcuts.py file. I did not include all the necessary libraries - it sounds like you're already running the MacroPad library examples so you shouldn't have trouble running this.

macropad-code.zip

@kattni
Copy link
Contributor

kattni commented Aug 31, 2021

@FoamyGuy Also what is all of this? Is it necessary?

code.py output:
int(0 * 64 / 2) + 3
int(0 * 120 / 6) + 3
(3, 3)
---
pos_y: 0 - size_y: 6
False
int(1 * 64 / 2) + 3
int(0 * 120 / 6) + 3
(35, 3)
---
pos_y: 0 - size_y: 6
False
int(0 * 64 / 2) + 3
int(1 * 120 / 6) + 3
(3, 23)
---
pos_y: 1 - size_y: 6
False
int(1 * 64 / 2) + 3
int(1 * 120 / 6) + 3
(35, 23)
---
pos_y: 1 - size_y: 6
False
int(0 * 64 / 2) + 3
int(2 * 120 / 6) + 3
(3, 43)
---
pos_y: 2 - size_y: 6
False
int(1 * 64 / 2) + 3
int(2 * 120 / 6) + 3
(35, 43)
---
pos_y: 2 - size_y: 6
False
int(0 * 64 / 2) + 3
int(3 * 120 / 6) + 3
(3, 63)
---
pos_y: 3 - size_y: 6
False
int(1 * 64 / 2) + 3
int(3 * 120 / 6) + 3
(35, 63)
---
pos_y: 3 - size_y: 6
False
int(0 * 64 / 2) + 3
int(4 * 120 / 6) + 3
(3, 83)
---
pos_y: 4 - size_y: 6
False
int(1 * 64 / 2) + 3
int(4 * 120 / 6) + 3
(35, 83)
---
pos_y: 4 - size_y: 6
False
int(0 * 64 / 2) + 3
int(5 * 120 / 6) + 3
(3, 103)
---
pos_y: 5 - size_y: 6
False
int(1 * 64 / 2) + 3
int(5 * 120 / 6) + 3
(35, 103)
---
pos_y: 5 - size_y: 6
False

@FoamyGuy
Copy link
Contributor Author

@kattni no definitely not necessary. It was troubleshooting print statements left in by accident. I noticed that when I tested the stock MacroPad example earlier and I've got it deleted in my local copy now, it will be removed with the next commit.

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Sep 1, 2021

@kattni okay the latest commit addresses the feedback from above, plus a few more things:

  • debugging prints have been removed
  • divider lines moved inside the height/width of the GridLayout (I need to test with your project code still, will do that tomorrow. Currently I've only tested with a different script that made a GridLayout the same size as the display.)
  • the constructor arguments related to divider lines have been changed to work as described above. In short: if you pass v_divider_line_cols or h_divider_line_rows you do not need to pass divider_lines=True and the defaults for the specific configured rows / cols has been effectively reversed. Due to this change it's no longer necessary to pass an empty tuple if you want either row or column lines only. Omitting one or the other of those configuration parameters will result in no divider lines for that direction.
  • I noticed and fixed an issue where sometimes the divider lines were missing one pixel on the right of horizontal lines or the bottom of vertical lines due to int() being used and always rounding decimals down.

It was not in this most recent commit, but from one of the earlier ones in this PR that GridLayout was modified to use anchored_position of the content items if they support it. This actually uncovered a bug that was previously causing labels to be drawn further up on the display than they should be due to label's default x,y positioning being left/center instead of left/top like the GridLayout code was assuming. The example in this repo has the position on the screen set to 10,10 which hid this problem so I didn't notice it until now.

As a result of this fix the positioning in the MacroPad grid example needs to be tweaked a bit to move the labels back onto the display. The larger cell padding was used to essentially make up for this issue, but with the issue resolved now the cell_padding can and should be lowered. I've opened a PR #29 over there to make that change. It should get merged at the same time as this one if possible.

Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

Looks great! I tested it with specifying lines between certain cells, and displaying lines around all cells.

It took a little tweaking on my code to get the line across the bottom to show up, but it's initial lack of appearance was due to my settings, not a failure with this update. Everything looks good now! I'll get this and the other PR you mentioned merged.

Thanks so much for doing this!

@kattni kattni merged commit e5e8b5a into adafruit:main Sep 1, 2021
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Sep 2, 2021
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.

Add lines between the cells in the GridLayout
2 participants