fix: MaterialColor Swatch Map comparison by ayaankhan98 · Pull Request #61974 · flutter/flutter

ayaankhan98 · 2020-07-21T19:10:41Z

Description

fixed the map comparison bug.
since _swatch is a Map and the comparison other._swatch == _swatch; will always return false if the Maps have different hashCodes.

therefore the == is replaced by MapEquality().equals(other._swatch,_swatch)

Related Issues

Fixes #61888

Tests

This is the bug reproducible code

import 'package:flutter/material.dart';

void main() {
 var map = {0:Colors.lightBlue,1:Colors.deepOrange,2:Colors.blueGrey};
 var first = new MaterialColor(0, map);
 var second = new MaterialColor(0, map);
 var third = new MaterialColor(0, {0:Colors.lightBlue,1:Colors.deepOrange,2:Colors.blueGrey});
 print("Comparison of the same map object in MaterialColor constructor: " + (first == second).toString());
 print("Comparison of different map objects with same values in MaterialColor constructor: " + (first == third).toString());
}

before changing the == by MapEquality().equals(other._swatch,_swatch)
the output was

I/flutter (17640): Comparison of the same map object in MaterialColor constructor: true
I/flutter (17640): Comparison of different map objects with same values in MaterialColor constructor: false

after implementing changes the output is as expected

I/flutter (17640): Comparison of the same map object in MaterialColor constructor: true
I/flutter (17640): Comparison of different map objects with same values in MaterialColor constructor: true

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

fluttergithubbot · 2020-07-21T19:10:43Z

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.

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

Piinks

Hi @ayaankhan98 welcome! And thank you for the contribution!

Can you revert the unrelated formatting changes here? You can refer to the Flutter Style Guide here.
The analyzer also appears to have caught some exceptions here:

info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:207:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:208:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:209:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:220:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:390:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:391:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:392:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:403:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:447:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:448:43 • always_put_control_body_on_new_line
   info • Prefer const with constant constructors • packages/flutter/lib/src/painting/colors.dart:451:9 • prefer_const_constructors
   info • Specify type annotations • packages/flutter/lib/src/painting/colors.dart:451:9 • always_specify_types
  error • Missing type arguments for generic type 'MapEquality<dynamic, dynamic>' • packages/flutter/lib/src/painting/colors.dart:451:9 • implicit_dynamic_type

Lastly, this should also have a test. That will make sure this works as intended, and more importantly it will ensure it is not undone later by another change.

ayaankhan98 · 2020-07-22T10:51:44Z

Hi @ayaankhan98 welcome! And thank you for the contribution!

Can you revert the unrelated formatting changes here? You can refer to the Flutter Style Guide here.
The analyzer also appears to have caught some exceptions here:

info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:207:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:208:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:209:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:220:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:390:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:391:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:392:20 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:403:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:447:33 • always_put_control_body_on_new_line
   info • Separate the control structure expression from its statement • packages/flutter/lib/src/painting/colors.dart:448:43 • always_put_control_body_on_new_line
   info • Prefer const with constant constructors • packages/flutter/lib/src/painting/colors.dart:451:9 • prefer_const_constructors
   info • Specify type annotations • packages/flutter/lib/src/painting/colors.dart:451:9 • always_specify_types
  error • Missing type arguments for generic type 'MapEquality<dynamic, dynamic>' • packages/flutter/lib/src/painting/colors.dart:451:9 • implicit_dynamic_type

Lastly, this should also have a test. That will make sure this works as intended, and more importantly it will ensure it is not undone later by another change.

okay Thanks @Piinks i will fix these issues soon.

ayaankhan98 · 2020-07-22T14:36:13Z

Lastly, this should also have a test. That will make sure this works as intended, and more importantly it will ensure it is not undone later by another change.

@Piinks where should i place test for this, in flutter/packages/flutter/test/material/colors_test.dart or in flutter/packages/flutter/test/painting/colors_test.dart

Piinks · 2020-07-22T16:58:28Z

@Piinks where should i place test for this, in flutter/packages/flutter/test/material/colors_test.dart or in flutter/packages/flutter/test/painting/colors_test.dart

✅ flutter/packages/flutter/test/painting/colors_test.dart - that should be a good place! Thanks! :)

Piinks

Thanks for updating this! Just a few nits. :)

Piinks

LGTM! Thank you!

The failing checks are not related to this change. Upstream is actually broken right now with this version solving problem: #62146. That will have to be fixed first, then this PR rebased for the checks to pass.

ayaankhan98 · 2020-07-23T19:13:39Z

The failing checks are not related to this change. Upstream is actually broken right now with this version solving problem: #62146. That will have to be fixed first, then this PR rebased for the checks to pass.

Okay, Thanks for help :)

goderbauer · 2020-07-29T21:48:43Z

@ayaankhan98 The issue has been fixed. Can you please rebase this change to make all the checks pass? Thanks.

ayaankhan98 · 2020-07-30T00:19:34Z

@ayaankhan98 The issue has been fixed. Can you please rebase this change to make all the checks pass? Thanks.

@goderbauer and @Piinks , i am facing some issue with my machine therefore i am unable to rebase the commits at this time. see if anyone of you can run the checks manually.

goderbauer · 2020-08-05T21:45:47Z

@ayaankhan98 Is your machine working again? Can you try rebasing it again? We'd have to create a new PR to do the rebase.

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 21, 2020

googlebot added the cla: yes label Jul 21, 2020

Piinks reviewed Jul 21, 2020

View reviewed changes

markusaksli-nc linked an issue Jul 22, 2020 that may be closed by this pull request

MaterialColor Swatch Map comparison always false #61888

Closed

goderbauer reviewed Jul 22, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/painting/colors.dart Outdated

Piinks reviewed Jul 23, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/painting/colors.dart Outdated

Comment thread packages/flutter/test/painting/colors_test.dart Outdated

Piinks approved these changes Jul 23, 2020

View reviewed changes

ayaankhan98 added 7 commits August 6, 2020 22:44

fix: merge conflict

0152fa8

reverted formatting changes

052897e

Added MaterialColor swatch comparison test

1b5976f

fix flutter analyzer error

f4cf73c

fixed all analyzer errors

e226315

removed collection using foundation

73c9fbf

style fix

311e24d

ayaankhan98 force-pushed the master branch from b172b25 to 311e24d Compare August 6, 2020 17:18

Piinks added the waiting for tree to go green label Aug 6, 2020

fluttergithubbot merged commit 4680ff3 into flutter:master Aug 6, 2020

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

fix: MaterialColor Swatch Map comparison (flutter#61974)

7955d5f

github-actions Bot locked as resolved and limited conversation to collaborators Aug 7, 2021

Navigation Menu

Repository navigation

fix: MaterialColor Swatch Map comparison - #61974

fix: MaterialColor Swatch Map comparison#61974
fluttergithubbot merged 7 commits into
flutter:masterfrom
ayaankhan98:master

ayaankhan98 commented Jul 21, 2020 •

edited by Piinks

Loading

Uh oh!

fluttergithubbot commented Jul 21, 2020

Uh oh!

Piinks left a comment

Uh oh!

ayaankhan98 commented Jul 22, 2020

Uh oh!

ayaankhan98 commented Jul 22, 2020

Uh oh!

Piinks commented Jul 22, 2020

Uh oh!

Uh oh!

Piinks left a comment

Uh oh!

Uh oh!

Uh oh!

Piinks left a comment

Uh oh!

ayaankhan98 commented Jul 23, 2020

Uh oh!

goderbauer commented Jul 29, 2020

Uh oh!

ayaankhan98 commented Jul 30, 2020 •

edited

Loading

Uh oh!

goderbauer commented Aug 5, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ayaankhan98 commented Jul 21, 2020 • edited by Piinks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Jul 21, 2020

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

ayaankhan98 commented Jul 22, 2020

Uh oh!

ayaankhan98 commented Jul 22, 2020

Uh oh!

Piinks commented Jul 22, 2020

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

ayaankhan98 commented Jul 23, 2020

Uh oh!

goderbauer commented Jul 29, 2020

Uh oh!

ayaankhan98 commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ayaankhan98 commented Jul 21, 2020 •

edited by Piinks

Loading

ayaankhan98 commented Jul 30, 2020 •

edited

Loading

goderbauer commented Aug 5, 2020 •

edited

Loading