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

Add new canvas.RecolorSVG API #5345

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

dweymouth
Copy link
Contributor

Description:

Fixes #4815

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style and have Since: line.

return content, err
}

func colorizeImpl(src []byte, clr color.Color, errFn func(string, error)) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

This error function seems a bit strange in my opinion. Why not just use the return value of the function directly and call fyne.LogError or similar on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah now that I look at it again, it is a bit strange. It was to minimize both code changes and duplicated code. Fyne.LogError takes 2 arguments, so I guess the alternative is for the signature to be func colorizeImpl(src []byte, clr color.Color) ([]byte, string, error) so that the actual error and the context we add to it can still be passed separately to fyne.LogError. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Do you even need the extra string information? It just seems unintuitive from how Go error handling usually is made? We can lose a bit in the FyneLog error and add any necessary additional information in the error text?

@dweymouth dweymouth changed the title Add new canvas.ColorizeSVG API Add new canvas.RecolorSVG API Dec 29, 2024
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.

2 participants