Fix painting material toggle (#78733) #78744

eEQK · 2021-03-21T21:17:11Z

This PR fixes painting material toggle, as it was a bit weird.

Fixes #78733

This is how it looks like on stable branch: https://www.dartpad.dev/ec2251e3d3f653f144cf5ac20a0c9a55?null_safety=true

Current master:

With my changes:

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

flutter-dashboard · 2021-03-21T21:17:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

eEQK · 2021-03-22T17:40:19Z

Updated the PR with goldens.
switch_test.big.on.png:

switch_test.small.on.png:

AUTHORS

goderbauer · 2021-04-08T21:58:48Z

packages/flutter/lib/src/material/switch.dart

+    const double radiusAdditionalOffset = _kTrackRadius / 2;
+    final double parentWidthOffset = size.width / 2 - trackWidth + radiusAdditionalOffset;


I am not fully following along how the parentWidthOffset has been determined, can you explain that?

I thought it would just be (size.width - _kSwitchWidth) / 2.0.

I've refactored the implementation a bit to make it easier to follow, hope that helps

This makes it .. more confusing to follow along. I think the previous implementation was easier to understand. Can you just give a verbal explanation of why the parentWidthOffset has to be size.width / 2 - trackWidth + radiusAdditionalOffset over just (size.width - _kSwitchWidth) / 2.0.

do you mind pointing out what's more confusing?

Also, while refactoring I've found that my previous implementation was wrong as I was confused due to the amount of informations. I read the material design switch spec which made things clearer and so I believe the current solution to be correct.

@goderbauer perhaps I could add some comments? I think it's really hard to assign meaningful names to the variables I'm modifying, 1-2 comment lines per logic block would probably explain a lot

Some comments would probably help.

@goderbauer do you mind having a look? I hope it's better now

skia-gold · 2021-04-14T19:04:18Z

Gold has detected about 2 untriaged digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/78744

flutter-dashboard · 2021-04-14T19:30:32Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #78744 at sha d97734ddc53a0e82227da07914be5b5ed7063abf

flutter-dashboard · 2021-05-04T20:48:18Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Piinks · 2021-06-10T17:49:27Z

(triage) Hi @eEQK, it looks like this PR has not been updated in a while, and several test are failing. Do you plan to return to this change?

eEQK · 2021-06-10T19:29:56Z

I didn't receive any response since my last commit, so I just assumed someone has yet to look at the updated PR. I believe the tests were failing on all PRs at the time, although I'm not sure why goldens are still pending. I'll sync my code with master and let's see whether they're fixed.

skia-gold · 2021-06-10T20:16:45Z

Gold has detected about 27 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/78744

skia-gold · 2021-06-10T20:21:45Z

Gold has detected about 2 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/78744

skia-gold · 2021-06-10T20:36:45Z

Gold has detected about 29 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/78744

skia-gold · 2021-06-10T20:41:45Z

Gold has detected about 2 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/78744

Piinks · 2021-06-10T20:44:01Z

FYI @kjlubick, can you confirm if the skia bot is confused here?

eEQK · 2021-06-10T21:05:16Z

@Piinks looks like it's working now 🎉

Piinks · 2021-06-10T22:04:04Z

Thank you for the updates! I need to confirm the golden file changes before this moves forward. It looks like there may be a bug there (not on your end), this should not be all green since you are adding new golden images. I'll see what's going on there and circe back here. Thanks for your patience!

kjlubick · 2021-06-14T12:57:56Z

Something certainly appears confused on my end @Piinks; I'll look into it.

Piinks

LGTM! Golden files are all good. Thanks for your patience! Can you update your branch again? It looks like there is a merge conflict.

eEQK · 2021-06-19T05:44:32Z

Amazing, thanks. I've resolved the conflict

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 21, 2021

google-cla bot added the cla: yes label Mar 21, 2021

eEQK force-pushed the 78733-fix-toggle-painting branch from 5573096 to 359028c Compare March 22, 2021 17:39

HansMuller requested a review from goderbauer March 22, 2021 18:22

goderbauer reviewed Apr 8, 2021

View reviewed changes

eEQK force-pushed the 78733-fix-toggle-painting branch from 359028c to d97734d Compare April 14, 2021 18:56

eEQK requested a review from goderbauer April 14, 2021 18:58

flutter-dashboard bot added the will affect goldens Changes to golden files label Apr 14, 2021

eEQK force-pushed the 78733-fix-toggle-painting branch from d97734d to 2c85354 Compare May 5, 2021 17:48

eEQK force-pushed the 78733-fix-toggle-painting branch from 2c85354 to f953b98 Compare June 10, 2021 20:11

eEQK force-pushed the 78733-fix-toggle-painting branch from f953b98 to 058350c Compare June 10, 2021 20:19

Piinks mentioned this pull request Jun 10, 2021

Do not merge - Testing Gold Infra #84389

Closed

Piinks approved these changes Jun 18, 2021

View reviewed changes

Piinks added the a: fidelity Matching the OEM platforms better label Jun 18, 2021

Fix painting material toggle (flutter#78733)

b7fa723

eEQK force-pushed the 78733-fix-toggle-painting branch from 058350c to b7fa723 Compare June 19, 2021 05:42

Piinks added the waiting for tree to go green label Jun 21, 2021

fluttergithubbot merged commit 8ffe7c5 into flutter:master Jun 21, 2021

		const double radiusAdditionalOffset = _kTrackRadius / 2;
		final double parentWidthOffset = size.width / 2 - trackWidth + radiusAdditionalOffset;

Fix painting material toggle (#78733) #78744

Fix painting material toggle (#78733) #78744

Conversation

eEQK commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Mar 21, 2021

Uh oh!

eEQK commented Mar 22, 2021

Uh oh!

Uh oh!

goderbauer Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

eEQK Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

goderbauer Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

eEQK Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

eEQK May 5, 2021

Choose a reason for hiding this comment

Uh oh!

goderbauer May 5, 2021

Choose a reason for hiding this comment

Uh oh!

eEQK May 5, 2021

Choose a reason for hiding this comment

Uh oh!

skia-gold commented Apr 14, 2021

Uh oh!

flutter-dashboard bot commented Apr 14, 2021

Uh oh!

flutter-dashboard bot commented May 4, 2021

Uh oh!

Piinks commented Jun 10, 2021

Uh oh!

eEQK commented Jun 10, 2021

Uh oh!

skia-gold commented Jun 10, 2021

Uh oh!

skia-gold commented Jun 10, 2021

Uh oh!

skia-gold commented Jun 10, 2021

Uh oh!

skia-gold commented Jun 10, 2021

Uh oh!

Piinks commented Jun 10, 2021

Uh oh!

eEQK commented Jun 10, 2021

Uh oh!

Piinks commented Jun 10, 2021

Uh oh!

kjlubick commented Jun 14, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

eEQK commented Jun 19, 2021

Uh oh!

Uh oh!

eEQK commented Mar 21, 2021 •

edited

Loading