displayio: make Group use a python list internally by deshipu · Pull Request #4233 · adafruit/circuitpython · GitHub
Skip to content

displayio: make Group use a python list internally#4233

Merged
tannewt merged 8 commits into
adafruit:mainfrom
pypewpew:displayio-group-list
Mar 2, 2021
Merged

displayio: make Group use a python list internally#4233
tannewt merged 8 commits into
adafruit:mainfrom
pypewpew:displayio-group-list

Conversation

@deshipu

@deshipu deshipu commented Feb 20, 2021 •
edited
Loading

Copy link
Copy Markdown

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

deshipu commented Feb 20, 2021

Copy link
Copy Markdown
Author

@deshipu deshipu mentioned this pull request Feb 21, 2021
Closed
@deshipu

deshipu commented Feb 21, 2021

Copy link
Copy Markdown
Author

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

deshipu commented Feb 21, 2021

Copy link
Copy Markdown
Author

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

tannewt commented Feb 22, 2021

Copy link
Copy Markdown
Member

@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

deshipu commented Feb 23, 2021

Copy link
Copy Markdown
Author

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

@tannewt tannewt left a comment

Copy link
Copy Markdown
Member

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.

Comment thread shared-module/displayio/Group.c Outdated
Comment thread shared-module/displayio/Group.c Outdated
Comment thread shared-bindings/displayio/Group.c Outdated
@deshipu

deshipu commented Feb 24, 2021

Copy link
Copy Markdown
Author

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

@tannewt tannewt left a comment

Copy link
Copy Markdown
Member

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

deshipu commented Mar 2, 2021

Copy link
Copy Markdown
Author

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

@tannewt tannewt left a comment

Copy link
Copy Markdown
Member

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