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

Little cleaning of BlCompositeBackground #412

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

jecisc
Copy link
Member

@jecisc jecisc commented Jan 24, 2024

This change cleans a little BlCompositeBackground

  • Avoid to copy collections twice in #withAll:
  • Do not initialize with an empty collection since all users are updating this collection later
  • Use getter
  • Remove an abbreviation to use something more readable
  • Improve the rendering of the class comment

Fixes #366

This change cleans a little BlCompositeBackground

- Avoid to copy collections twice in #withAll:
- Do not initialize with an empty collection since all users are updating this collection later
- Use getter
- Remove an abbreviation to use something more readable
- Improve the rendering of the class comment

Fixes pharo-graphics#366
@tinchodias
Copy link
Collaborator

The only point I would discuss is the preference to use background getter instead of the instvar. In last time my opinion is to directly use the instvar if this is defined by the same class... I don't find using your own instvar as something wrong. And tools in Pharo are much more precise to detect writes and reads (vs. looking for senders and implementors).

What is your opinion?

But this doesn't stop merging the PR.

@tinchodias tinchodias merged commit fe039b6 into pharo-graphics:dev-1.0 Jan 25, 2024
6 checks passed
@jecisc
Copy link
Member Author

jecisc commented Jan 25, 2024

I tend to use getters in case we add defensive strategies later.

But that's a personal opinion, if you prefer direct accesses I'll use that without problem :)

@jecisc jecisc deleted the composite-background branch January 25, 2024 09:28
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.

Spurious copy and initialization in compositeBackground
2 participants