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

PAINTROID-420 Support more than four layers #1136

Merged

Conversation

Electronix1337
Copy link
Contributor

@Electronix1337 Electronix1337 commented Aug 3, 2022

I first implemented this ticket with the existing Listview and after finishing the implementation I experienced that performance when dragging and scrolling is too bad to be acceptable. Therefore I researched for a good alternative and found the Recyclerview which is a bit more work to implement and make it work but performance is better and overall Recyclerview seems more flexible. I then implemented the recyclerview and after that still experienced dropping frames which was caused by calling notifyDataSetChanged() too often, which is too generic for the recyclerview. Also changed the layout of the layermenu from a relative layout to a contraint layout which also improves performance. Changed number of MAX_LAYERS to 100. Also implemented a memory check, to verify that there is enough heap space to allocate new layer/bitmap. When user drags layer to bottom or top of the list, the list will automatically scroll down/up respectively and also visibly change with the last/first visible layer.

Also implemented two tests, one to check if 100 layers can be added, if there is enough space, and also assert that "add layer" button will be disabled and one test to see if 10 layers can be added and that the layer count is correct.

For the sake of testing I tried adding a 2MB picture to each of the 100 layers and saving it as a .ora and .catrobat-image to see performance of saving and also if it works in general. The resulting file was 188 MB large and the process of saving took around 1-2 minutes. I shortly investigated if this could be improved by any means but did not find anything, as we have to save each bitmap separately and we are already compressing it with a PNG format. Also the loading of such a large file takes quite some time but this could be improved in another ticket maybe.

https://jira.catrob.at/browse/PAINTROID-420

Your checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

  • Include the name of the Jira ticket in the PR’s title
  • Include a summary of the changes plus the relevant context
  • Choose the proper base branch (develop)
  • Confirm that the changes follow the project’s coding guidelines
  • Verify that the changes generate no compiler or linter warnings
  • Perform a self-review of the changes
  • Verify to commit no other files than the intentionally changed ones
  • Include reasonable and readable tests verifying the added or changed behavior
  • Confirm that new and existing unit tests pass locally
  • Check that the commits’ message style matches the project’s guideline
  • Stick to the project’s gitflow workflow
  • Verify that your changes do not have any conflicts with the base branch
  • After the PR, verify that all CI checks have passed
  • Post a message in the #paintroid Slack channel and ask for a code reviewer

@Electronix1337 Electronix1337 force-pushed the PAINTROID-420_Support_100_Layers branch 4 times, most recently from e27548e to 15c6231 Compare August 4, 2022 09:24
Changed number of MAX_LAYERS to 400. Also implemented a memory check, to verify that there is enough heap space to allocate new layer/bitmap.
@Electronix1337 Electronix1337 force-pushed the PAINTROID-420_Support_100_Layers branch from 15c6231 to c9f74e3 Compare August 5, 2022 08:21
Copy link
Contributor

@AneiMakovec AneiMakovec left a comment

Choose a reason for hiding this comment

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

LGTM. More than 4 layers can be added, scrolled through, grabbed and dragged around, as expected. Tests are also ok.

Copy link
Member

@wslany wslany left a comment

Choose a reason for hiding this comment

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

Thanks!!

@wslany wslany merged commit 3253d71 into Catrobat:develop Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants