Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[google_maps_flutter] Avoid unnecessary redraws #1933

Merged
iskakaushik merged 4 commits into from
Aug 28, 2019
Merged

[google_maps_flutter] Avoid unnecessary redraws #1933

iskakaushik merged 4 commits into from
Aug 28, 2019

Conversation

@ppicas
Copy link
Contributor

@ppicas ppicas commented Jul 30, 2019

Description

This PR fixes a problem with current GoogleMap widget implementation that recreates all map elements every time the widget is built.

Related Issues

flutter/flutter#37174

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.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@ppicas ppicas requested a review from iskakaushik as a code owner July 30, 2019 13:31
kjeremy
kjeremy reviewed Jul 30, 2019
View reviewed changes
packages/google_maps_flutter/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## 0.5.20+2

* Don't recreate map elements if they didn't change since las widget build.
Copy link

@kjeremy kjeremy Jul 30, 2019

Choose a reason for hiding this comment

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

last

@sergicastellsague
Copy link

sergicastellsague commented Aug 5, 2019

@iskakaushik can you please give it some love?

@kjeremy
Copy link

kjeremy commented Aug 7, 2019

Can we get this in? It's hurting one of our apps.

@ppicas ppicas force-pushed the avoid_unnecessary_redraws branch 3 times, most recently from be24d67 to d7e7cfc Compare August 12, 2019 08:33
@ppicas
Copy link
Contributor Author

ppicas commented Aug 12, 2019

@iskakaushik do you know why is failing build IPAs task?

@kjeremy
Copy link

kjeremy commented Aug 12, 2019

Looks like it simply couldn't connect.

@kjeremy
Copy link

kjeremy commented Aug 19, 2019

What do we need to do to push this across the finish line?

@sergicastellsague
Copy link

sergicastellsague commented Aug 20, 2019

Friendly reminder @iskakaushik

iskakaushik
iskakaushik reviewed Aug 20, 2019
View reviewed changes
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left a few comments. I think this deserves a minor version upgrade. Can we change to 0.5.21. Thanks.

packages/google_maps_flutter/lib/src/circle_updates.dart Outdated
.map(idToCurrentCircle)
.toSet();

bool currentNotEqualsPrevious(Circle _current) {
Copy link
Contributor

@iskakaushik iskakaushik Aug 20, 2019

Choose a reason for hiding this comment

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

  1. For parameters, we don't need to use _ prefix as they are already scoped. Can we change this to Circle current.
  2. I think the method reads better if we call it: hasChanged(Circle current).
  3. Add dart-docs for this method, something along the lines of what we compare it against, etc.

Note, this applies for all Polygon/Polyline/Markers etc.

Copy link
Contributor Author

@ppicas ppicas Aug 28, 2019

Choose a reason for hiding this comment

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

Yeah, hasChanged is a better name. I'll change it.

packages/google_maps_flutter/test/circle_updates_test.dart
expect(addedCircle, equals(c2));

expect(platformGoogleMap.circleIdsToRemove.isEmpty, true);

Copy link
Contributor

@iskakaushik iskakaushik Aug 20, 2019

Choose a reason for hiding this comment

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

I think having a test with multiple circles, where we only change a couple of them and compute the updates would be interesting. Same for others too.

Copy link
Contributor Author

@ppicas ppicas Aug 28, 2019

Choose a reason for hiding this comment

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

Cool, I'll will add some tests.

@iskakaushik
Copy link
Contributor

iskakaushik commented Aug 20, 2019 •
edited
Loading

sorry for the delay here.

@ppicas ppicas force-pushed the avoid_unnecessary_redraws branch from d7e7cfc to 0105ecb Compare August 28, 2019 14:35
@iskakaushik
Copy link
Contributor

iskakaushik commented Aug 28, 2019

LGTM

@iskakaushik iskakaushik merged commit 94290d9 into flutter:master Aug 28, 2019
@ppicas ppicas mentioned this pull request Aug 28, 2019
Closed
This was referenced Aug 28, 2019
Closed
Closed
@sergicastellsague
Copy link

sergicastellsague commented Aug 29, 2019

good job

@joaquinperaza
Copy link

joaquinperaza commented Sep 12, 2019

I think this cause #39737

@ppicas ppicas deleted the avoid_unnecessary_redraws branch September 12, 2019 07:35
@amirh
Copy link
Contributor

amirh commented Sep 18, 2019

@joaquinperaza can you confirm by testing with just this change reverted?

@joaquinperaza
Copy link

joaquinperaza commented Sep 18, 2019 •
edited
Loading

@amirh yes, google_maps_flutter: "0.5.20+6" is the last version that doesn’t cause the problem, if I update, then polylines don’t update on change.

@KlausJokisuo KlausJokisuo mentioned this pull request Oct 18, 2019
Closed
13 tasks
@tzvc
Copy link
Contributor

tzvc commented Oct 25, 2019

Is it really necessary to compare the onTap attribute of Marker in the == overload?
It doesn't need to trigger a change on the platform side (from what I understand) and cause unnecessary redraw when using closures.

What do you guys think?

@ppicas
Copy link
Contributor Author

ppicas commented Oct 28, 2019

Hi @theochampion, I think that it make sense. I've seen that you already created a pull request. Thanks!

@tzvc
Copy link
Contributor

tzvc commented Nov 8, 2019

@ppicas Feel free to upvote it to accelerate the review process ;)

Also, as you worked on map performance, you might be interested in joining the conversation here flutter/flutter#41731

mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
@ppicas @mormih
[google_maps_flutter] Avoid unnecessary redraws (flutter#1933)
5bec448
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@ppicas
[google_maps_flutter] Avoid unnecessary redraws (flutter#1933)
4f0063e
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@iskakaushik iskakaushik iskakaushik left review comments

@kjeremy kjeremy kjeremy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ppicas @sergicastellsague @kjeremy @iskakaushik @joaquinperaza @amirh @tzvc @googlebot