Remove the need to hook resources in for overriding in theme override #4926
Conversation
canvas/image.go
Outdated
| return io.NopCloser(bytes.NewReader(i.Resource.Content())), nil | ||
| content := i.Resource.Content() | ||
| if res, ok := i.Resource.(fyne.ThemedResource); i.isSVG && ok { | ||
|
|
There was a problem hiding this comment.
I think, this empty line can be deleted.
container/theme_test.go
Outdated
| o.Refresh() | ||
| changed := w.Canvas().Capture().(*image.NRGBA) | ||
|
|
||
| assert.NotEqual(t, plain.Pix, changed.Pix) |
There was a problem hiding this comment.
Tests only containing NotEqual are always a little strange. I think an image comparison or at least a markup comparison would have been better here (and in the text test below).
There was a problem hiding this comment.
at least a markup comparison would have been better here (and in the text test below).
Unfortunately a markup comparison won't show the differences because the widget state is identical, it is the theme context applied that changes. So no markup difference - only render pixel colour.
There was a problem hiding this comment.
Added image tests as requested
There was a problem hiding this comment.
- verify that no longer theme some icons is really intended
- remove stray empty line
In general: The PR was quite hard to grasp despite it isn’t that big. It would have helped to have commits organized like so:
- Add the new param to
GetSvgandSetSvgin a first commit without using them yet but including all the necessary test adjustments. - Move the logic and include the necessary tests and test adjustments.
In short, a commit should be focused (only address a single aspect) and complete (in the sense that it does not fail nor miss tests).
| @@ -8,15 +8,15 @@ | |||
| <widget pos="32,8" size="9x20" type="*widget.RichText"> | |||
| <text alignment="center" bold size="9x19">A</text> | |||
| </widget> | |||
| <image fillMode="contain" pos="8,8" rsc="menuDropDownIcon" size="iconInlineSize" themed="foreground"/> | |||
There was a problem hiding this comment.
AFAICS, this (and other) previously were explicitly themed with the (default) foreground color. Now, it says that it is no longer themed anymore and thus will use the image’s colors, right? Is this really wanted?
There was a problem hiding this comment.
There was inconsistency before - named resources did not seem to have theme information (at least not some of them?) for example this happened on one test:
<image pos="66,7" rsc="menuExpandIcon" size="iconInlineSize"/>
<image pos="8,7" rsc="account.svg" size="iconInlineSize" themed="default"/>
I have updated the markup test renderer to be more consistent which has added changes. A smaller change set than before which seems promising - but overall it is now explicit about what theme is applied on all resources I think...
I agree, and sorry for the lack of cleanliness - I have been travelling and desperately trying to get the required work done between flights, presentations and other commitments during this release process. |
Description:
Fixes #4596
Checklist: