Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observe TileLayerOptions changes #584

Merged

Conversation

maRci002
Copy link
Contributor

Hello, this PR adds support for detecting URL changes / tileSize changes / updateInterval changes and reloads Tiles if necessary.

change layer

closes #583

@maRci002
Copy link
Contributor Author

maRci002 commented May 4, 2020

Old tiles can be overwritten too with new ones without reload see #583.

@kengu
Copy link
Contributor

kengu commented May 13, 2020

@johnpryan - I think this PR should be prioritized, together with #577. It solves some real problems that blocks actual usage in the wild. We are using flutter_map in an open source app for search and rescue, which demands good behavior when network conditions are poor. These PRs resolves some long-standing issues for us.

@maRci002 - this is awesome work, thanks!

@kengu
Copy link
Contributor

kengu commented May 13, 2020

@maRci002 - I think there are some additional use-cases to cover, which I'm currently looking into. Most importantly; tracking connectivity changes from offline to online. Currently, there is no way of externally forcing a invalidation of tiles with Tile.loadError=true. One solution could be to add the method MapController.evictErrorTiles(). This could be used to evict all error tiles when the parent widget of FlutterMap detects an offline to online transitions has occurred. I think there are probably more use-cases for this type of capability. This solutions allows for handling of connectivity issue decoupled from flutter_map.

@maRci002
Copy link
Contributor Author

@kengu this PR is ready to merge.

#577 is still under development however you are right TileLayer shouldn't detect connection issues because an app may have multiple TileLayers but is is still possible one or more layer can be indenpendent from internet forinstance AssetTileProvider. abstract class TileProvider may has a method which return true if it depends on internet connection so Flutter Map can check each layer when connection restores.

@kengu
Copy link
Contributor

kengu commented May 13, 2020

@kengu this PR is ready to merge.

#577 is still under development however you are right TileLayer shouldn't detect connection issues because an app may have multiple TileLayers but is is still possible one or more layer can be indenpendent from internet forinstance AssetTileProvider. abstract class TileProvider may has a method which return true if it depends on internet connection so Flutter Map can check each layer when connection restores.

Yes, that my line of thinking also. Did a quick test locally using the same approach as you have in this PR. Just adding a "offline" member to the options and checking if it is changed and false, which evicts all error tiles works like a charm when combined with the connectivity pacakge.

Working example: https://github.com/DISCOOS/flutter_map/blob/track_change_and_evict_errors/lib/src/layer/tile_layer.dart#L392

@maRci002
Copy link
Contributor Author

maRci002 commented May 13, 2020

Connectivity package will trigger even if you just connect to router but it may not has connection to internet.

If you just play with offline flag trick without setting any evictErrorTileStrategy then this could happen:

  1. Open map, it loads 1,2,3,4 tiles
  2. Internet gone
  3. User pans map to 5,6,7,8 tiles, they will be error tiles (don't forget image provider cache these errors: NetworkImage docs: The image will be cached regardless of cache headers from the server.)
  4. User pans map to like 101, 102, 103, 104 they will be error tiles <- _tiles won't hold 1,2,3,4 and 5,6,7,8 tiles anymore because those tiles aren't current or retained anyomore because user panned far away.
  5. Internet comes back
  6. 101, 102, 103, 104 tiles will be reloaded
  7. User pans back to 5,6,7,8 tiles these will be loaded as error since _tiles did not held any reference to these tiles anymore when connection restored so there wasn't any chance to evict them. <- can be fixed if you use forinstance notVisible stategy mixed with offline trick.

@kengu
Copy link
Contributor

kengu commented May 14, 2020

@maRci002 - that's right. I'm actually using EvictErrorTileStrategy.dispose and added what's probably missing in #577 which is disposing all error tiles in _update method. This works like a charm together.

I see that for dispose you have written evict error Tiles during _pruneTiles / _abortLoading calls which is is not enough for the offline trick, since visible tiles are not disposed then.

Although, the impact of not setting EvictErrorTileStrategy is minimized when combining connectivity package with data_connection_checker, which is what I've opted for. Made a singleton class of the combination named ConnectivityService, and wrapped it in a NetworkSensitive widget which rebuild it's children each time connection type or status changes . In the parent widget of FlutterMap just TileLayerOptions.offline = Provider.of<ConnectivityStatus>(context).status in the build method for FlutterMap and tiles will reload automatically. The only remaining "error situation" now it a real error from the tile-server, which is normal operation.

@kengu kengu mentioned this pull request May 15, 2020
@S-Man42
Copy link

S-Man42 commented May 26, 2020

@maRci002 Could you please provide the source code of your example above. I find it very interesting.

@maRci002
Copy link
Contributor Author

@S-Man42 I don't have the example code but it ist just a FloationActionButton which changes WMSTileLayerOptions' baseUrl via setState.

However since #585 needs this PR I made there an example just register your String url; and write your own setState() logic which replaces url

@S-Man42
Copy link

S-Man42 commented May 27, 2020

That works for me very well! Great! Unfortunately I cannot use in my app currently, because I am using the Marker Popup Plugin which depends on flutter_map 1.4.0 and has problems with your git commit checkout. We need to get this merged!

@fryette
Copy link

fryette commented May 31, 2020

Any chance to merge it? Same issue as for @S-Man42
@porfirioribeiro @avioli

Copy link

@S-Man42 S-Man42 left a comment

Choose a reason for hiding this comment

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

Found no problems with the code.
Checked out - worked as expected.

Copy link
Contributor

@avioli avioli left a comment

Choose a reason for hiding this comment

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

LGTM, but I have no write access.

@@ -952,11 +1034,14 @@ class Tile implements Comparable<Tile> {
_imageStream?.removeListener(_listener);
Copy link

@fryette fryette Jun 14, 2020

Choose a reason for hiding this comment

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

I think we also need to dispose the animation controller, that was a reason for crash

Suggested change
_imageStream?.removeListener(_listener);
animationController?.dispose();
_imageStream?.removeListener(_listener);

@fryette
Copy link

fryette commented Jun 14, 2020

I used this commit for about month and found that we need also to dispose Animation Controller
@maRci002 could you please push change?

@Lidgett
Copy link

Lidgett commented Jun 19, 2020

Any chance of merging this? Having the option to reset/reload the tiles is pretty essential in my opinion.

@JanikoNaber
Copy link

Please we need one additional approval. Could anyone have a look at this PR?
This function is great!

@fryette
Copy link

fryette commented Jun 29, 2020

@maRci002 please approve the proposed change

@johnpryan johnpryan merged commit b419321 into fleaflet:master Jul 8, 2020
@maRci002 maRci002 mentioned this pull request Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues when change map tiles URL dynamically
9 participants