Makie.jl icon indicating copy to clipboard operation
Makie.jl copied to clipboard

Improve and use `shared_attributes` for attribute passthrough

Open ffreyer opened this issue 5 months ago • 5 comments

Description

We have shared_attributes(parent, TargetType) as a function that filters attributes from parent applicable to TargetType but we are not using it. This pr aims to extend the function and use it consistently in recipes. That should help fix a lot of the issues about attributes not being passed along/having no effect in recipes.

Some thoughts:

  • Allowing shared_attributes to skip/drop a set of given attribute names means that some valid attributes will not get set. That sounds like something we should not encourage. Edit: It is quite useful in some cases, e.g. if you want to pass strokecolor to a poly but not a scatter or fxaa to a mesh but not lines.
  • Renaming attributes is a common and valid task (e.g. strokecolor => color), so shared_attributes should handle that
  • Replacing attributes is also common, but typically requires a multiline map (etc). Because of that I think it's preferable to have attr[:name] = map(...) instead of something like obs = map(...); shared_attributes(..., :name => obs).
  • There are some attributes that only make sense at the top level, e.g. the inspector ones. These should be automatically cleared.
  • Warnings for missing valid attributes might be useful to find further issues with missing attribute passthrough.

First Version

The usage of the first version looked something like this:

# filters attributes applicable to TargetType from parent plot
# remove some top level/local attributes
# renames based on rename list
attr = shared_attributes(parent, TargetType, [:name_in_parent => :name_in_target, ...])

# removing attributes
pop!(attr, :name)

# replacing/adding
attr[:name] = new_attr

This has a couple of problems:

  • External pop! and replacing/adding makes "warn_on_unset_attribute" pretty useless since a good chunk of attribute mutation happens afterwards
  • Replacements invite mistakes as setindex!(::Attributes, val, name) updates Observables if val is not an Observable. E.g. lines_attr[:fxaa] = false would change the parent attributes fxaa value, which could also be used in another plot where it should remain true.
  • The rename array is unintuitive. As a replacement it would be written in reverse order as attr[:name_in_target] = parent[:name_in_parent]

Second Version (c04f450 and after)

attr = shared_attributes(
    parent, TargetType, 
    :name_to_delete, ...; 
    name_to_replace_or_add = value, ...
)

The second version solves the issues above:

  • Everything can/should happen in the shared_attributes call so we can track defaulted attributes.
  • Replacements (given as kwargs) are handled by shared_attributes to not change values of the parent.
  • Renaming doesn't exist explicitly anymore. They are handled via replacements/kwargs, i.e. new_name = parent[:old_name].

Related Issues/prs

  • Fixes #4458
  • Closes #2206 (shared_attributes should be a preferable alternative)
  • Fixes #2733
  • Closes #3263
  • Closes #4435

Type of change

  • Internal cleanup with potential for breaking changes in attribute passthrough

Checklist

  • [x] Added an entry in CHANGELOG.md (for new features and breaking changes)
  • [ ] Added or changed relevant sections in the documentation
  • [ ] Added unit tests for new algorithms, conversion methods, etc.
  • [ ] Added reference image tests for new plotting functions, recipes, visual options, etc.

ffreyer avatar Sep 22 '24 16:09 ffreyer