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.

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

displayio: make Group use a python list internally #4233

Merged
merged 8 commits into from
Mar 2, 2021
Merged

displayio: make Group use a python list internally #4233

merged 8 commits into from
Mar 2, 2021

Conversation

deshipu
Copy link

@deshipu deshipu commented Feb 20, 2021 •
edited
Loading

The displayio.Group should use a python list for keeping its members, so that we don't have to set max_size explicitly, and we can use list-like methods like sort on it.

@deshipu
Copy link
Author

deshipu commented Feb 20, 2021

Looking further into the inheritance mechanisms, it seems that more than one base class for a subclass is not supported: https://github.com/adafruit/circuitpython/blob/main/py/objtype.c#L108

That means that displayio.Group can't be a subclass of Layer and List at the same time.

@deshipu deshipu mentioned this pull request Feb 21, 2021
Closed
@deshipu
Copy link
Author

deshipu commented Feb 21, 2021

I can't make the Group a subclass of List, because then mp_instance_cast_to_native_base will return the list struct, when we want the group struct. I'm instead trying to wrap the list inside the group, and forward the methods we need.

@deshipu
Copy link
Author

deshipu commented Feb 21, 2021

Note that it would be possible to add just the sort method without using a list internally — that function can sort any continuous array of python objects.

@deshipu deshipu force-pushed the displayio-group-list branch from d23a7ed to fa1b6af Compare February 21, 2021 16:21
@deshipu deshipu changed the title (WIP) displayio: make Group inherit from the python list displayio: make Group inherit from the python list Feb 22, 2021
@tannewt
Copy link
Member

tannewt commented Feb 22, 2021

@deshipu Is this ready for review? Could you update the title to match your implementation? Thanks!

@deshipu deshipu changed the title displayio: make Group inherit from the python list displayio: make Group use a python list internally Feb 23, 2021
@deshipu deshipu force-pushed the displayio-group-list branch from df5989d to 5b503ab Compare February 23, 2021 09:30
@deshipu
Copy link
Author

deshipu commented Feb 23, 2021

Sorry, I missed the title is wrong. I rebased on top of main now, and removed the unnecessary changes. It's ready for reviews, thank you.

tannewt
tannewt requested changes Feb 23, 2021
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this!

I think you should add a deprecation notice in the docs of max_size (yay!!!) to say it'll be removed in 7.x and make it do nothing internally.

shared-module/displayio/Group.c Outdated Show resolved Hide resolved
shared-module/displayio/Group.c Outdated Show resolved Hide resolved
shared-bindings/displayio/Group.c Show resolved Hide resolved
@deshipu
Copy link
Author

deshipu commented Feb 24, 2021

Thank you for your review, I will work on this further over the weekend.

@kmatch98 kmatch98 mentioned this pull request Feb 25, 2021
Closed
@deshipu
displaio: don't store group children native object
6e0ce23 
Since we want to expose the list of group's children to the user,
we should only have the original objects in it, without any other
additional data, and compute the native object as needed.
@deshipu
Replace displaio.Group.children with a python list
121c6bc 
This is a first go at it, done by naive replacing of all array
operations with corresponding operations on the list. Note that
there is a lot of unnecessary type conversions, here. Also, list_pop
has been copied, because it's decalerd STATIC in py/objlist.h
@deshipu
Separate mp_obj_list_pop so it can be used outside of objlist.c
e505c59
@deshipu
Add displayio.Group.sort() method
e995375
@deshipu
Remove unused typedef for group children
9c41753
@deshipu
Remove max_size from displayio.Group
38fb7b5 
Still accept it as an argument. Add deprecation note.
@deshipu deshipu force-pushed the displayio-group-list branch from 5a49467 to 38fb7b5 Compare February 27, 2021 19:52
@deshipu
Separate out mp_obj_list_insert for use in display.Group
24473b7 
Note that for some reason this makes the binary 500 bytes larger!
tannewt
tannewt requested changes Mar 2, 2021
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Code looks good to me! Thank you! ugame10 looks unhappy though unfortunately.

@deshipu
Copy link
Author

deshipu commented Mar 2, 2021

ugame10 has been enhappified

@deshipu deshipu requested a review from tannewt March 2, 2021 14:52
tannewt
tannewt approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Great! Thank you!

@tannewt tannewt merged commit e4f0e47 into adafruit:main Mar 2, 2021
This was referenced Mar 3, 2021
Closed
Closed
@kmatch98 kmatch98 mentioned this pull request Apr 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@deshipu @tannewt