Make Button rendering consistent #4246
Conversation
For the docTabs.current < 0 case, modified this function to call docTabsRenderer.moveIndicator (pos 0,0, size 0,0) to give consistent rendering when first drawn and after all tabs have been removed.
Changed initial rendering to match the result after a button click
|
This is strange. There is only one source file that got changed - all the rest are testdata .xml files. It's acting as if the test data did not get pushed before the tests were run. |
|
Should "dialog-custom-custom-buttons.xml" be present? |
|
@pbrown12303 Would you mind giving the PR a more descriptive title? |
widget/button.go
Outdated
| tapBG := canvas.NewRectangle(color.Transparent) | ||
| tapBG.FillColor = &color.NRGBA{R: 0, G: 0, B: 0, A: 0} |
There was a problem hiding this comment.
This seems strange. The canvas.NewRectangle() constructor already sets FillColor. Is this line really needed?
There was a problem hiding this comment.
Yes. The tap animation does not return the color to transparent (a nil color). It explicitly sets it to this value. The other alternative would be to alter the animation logic to return the color to nil at the end, but I think this would be a Pandora's box. I certainly do not know enough about animation to even begin to assess the feasibility of this kind of change.
There was a problem hiding this comment.
What I am trying to say is that you can replace color.Transparent on the line above instead of setting the field directly after.
However, if this is just a workaround for the animation doing strange things it seems like we ought to fix the animation instead? We are using color.Transparent all over the place and if we need this workaround everywhere then something seems very wrong.
Thoughts? @andydotxyz
There was a problem hiding this comment.
Yes, I see. I didn't realize that the value supplied to the NewRectangle() function was the background color - I thought it was the line (edge) color.
With respect to the use of color.Transparent, I suspect it is only an issue with animation. I'll let you and @andydotxyz discuss and draw conclusions - I'll go with whatever you decide.
There was a problem hiding this comment.
Colour.Transparent is not nil...
https://cs.opensource.google/go/go/+/refs/tags/go1.21.1:src/image/color/color.go%3Bl=345
not sure what nil is being referenced!
There was a problem hiding this comment.
Indeed. That was my though as well
There was a problem hiding this comment.
Sorry, I was wrong about transparent being nil: it is a struct with only the A value specified. In any case, I found a better fix. I modified the Button animation so that if the fade (i.e. the A value of the color) reaches zero, the assigned value for the bg.FillColor is color.Transparent, which was the default initial value. Updated test results accordingly.
|
@andydotxyz Added missing dialog-custom-custom-buttons.xml |
Removed explicit setting of tapBG.FillColor in CreateRenderer Changed bg.FillColor assignment logic in newButtonTabAnimation so that if the fade reaches zero, the assigned color is color.Transparent Updated xml rendering test results to reflect this change.
|
Looks like the expected test results did not get pushed before the tests were run. |
GitHub doesn't usually get the order wrong. Are you sure all the fixes are pushed? |
|
Everything is pushed. GitHub has no way of knowing the relationship between the _test.go files and the .xml test results. The only linkages are the quoted text strings that are the arguments to the test.Assert... functions. |
That doesn't seem very likely. All other tests (except for a few spurious tests failing sometimes) are passing |
GitHub doesn't understand it true - but it does know when a commit has been pushed and will clearly wait for all the files to be present before it starts the CI run... |
|
It looks to me that both failures came from the same test case. I have asked it to re-run just in case, but it seems to me that "go test -tags mobile ./..." is failing every time and for the same reason. |
|
Interesting - although the mobile tests showed PASS on windows 10, the tests were never executed. I updated the test results manually - I hope I got all of them. Is there a mechanism for running the mobile tests on a Windows 10 platform? |
|
I don't think the mobile driver supports running in Windows? |
|
Looks good to me - @Jacalz ? |
Changed the default rendering of Button so that the animation background rectangle starts off with the same rendering as it has after the button has been clicked
Description:
Changed the default rendering of Button so that the animation background rectangle starts off with the same rendering as it has after the button has been clicked
Unfortunately, this affected all of the xml testdata files that contain buttons (87 of them). All test result files have been updated and the full fyne test suite passed with the excaption of some errors in internal/driver/glfw which do not appear to have anything to do with this change.
Fixes #4243
Checklist:
Where applicable: