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

[FEATURE] Clean up examples #1545

Closed
JosefWN opened this issue Jun 4, 2023 · 7 comments · Fixed by #1632
Closed

[FEATURE] Clean up examples #1545

JosefWN opened this issue Jun 4, 2023 · 7 comments · Fixed by #1632
Labels
feature This issue requests a new feature P: 3 (low) (Default priority for feature requests)

Comments

@JosefWN
Copy link
Contributor

JosefWN commented Jun 4, 2023

What do you want implemented?

  • With the introduction of flutter_map_animations, included in the docs, perhaps we should remove the old example at pages/animated_map_controller.dart which provides a poorer solution?
  • The EPSG4326 (WGS84) example is showing a distorted map which can be a bit confusing to new users, and even to me every time I run through the examples to test my changes. The capabilities announced by the WMS server list EPSG4326 as available, but even if you zoom in, the labels are so distorted that I can't even read most of them, giving the impression that something is wrong.
  • The secondary tap example is quite flaky, sometimes the notification of the tap appears 10-30 seconds after the tap itself (even if there is no visible notification). Suddenly, for example, a number of these queued notifications appear.
  • The tile loading error handle gives the error Invalid argument(s): No host specified in URI in flight mode, which seems a bit cryptic. Failed DNS lookup? Perhaps we should return a more informative error to the library user, not just in the example but in general. Nit: Sometimes these UI notifications, just like the ones in the secondary tap example, load quite a bit later than when the event occurs (i.e. when I pan without internet connection, tiles fail to load, and after a number of seconds, I get the message).
  • Tile Layer reset does not reload tiles when the reset button is pressed, requires a pan for example, to reload. Until then, the whole map is grey.
  • In Point to LatLng, is rotating the map by clicking the rotation button supposed to change the (center?) coordinate of the logo?
  • Many maps, including OSM which we are using, require attribution which we don't always display in the examples.

What other alternatives are available?

No response

Can you provide any other information?

No response

Severity

Minimum: Not required for my use

@JosefWN JosefWN added the feature This issue requests a new feature label Jun 4, 2023
@JaffaKetchup
Copy link
Member

With the introduction of flutter_map_animations, included in the docs, perhaps we should remove the old example at pages/animated_map_controller.dart which provides a poorer solution?

We probably should remove it, but the question comes as do we want to add it back with the plugin behind the scenes? We shouldn't really be 'advertising' plugins as our own work/part of fleaflet.


The EPSG4326 (WGS84) example is showing a distorted map which can be a bit confusing to new users, and even to me every time I run through the examples to test my changes. The capabilities announced by the WMS server list EPSG4326 as available, but even if you zoom in, the labels are so distorted that I can't even read most of them, giving the impression that something is wrong.

Do you think it should be removed then? If that's what the CRS is (confusing), then that's what it is, if you see what I mean. My experience with CRSs other than the normal Mercator (etc) is very limited.


The secondary tap example is quite flaky, sometimes the notification of the tap appears 10-30 seconds after the tap itself (even if there is no visible notification). Suddenly, for example, a number of these queued notifications appear.

For me, it appears to be instant, if there was no snackbar already displayed. Showing the coords in a snackbar was probably a mistake (due to its slow update time), so we should probably find a different way of showing it.


The tile loading error handle gives the error Invalid argument(s): No host specified in URI in flight mode, which seems a bit cryptic. Failed DNS lookup? Perhaps we should return a more informative error to the library user, not just in the example but in general. Nit: Sometimes these UI notifications, just like the ones in the secondary tap example, load quite a bit later than when the event occurs (i.e. when I pan without internet connection, tiles fail to load, and after a number of seconds, I get the message).

When running on the web and using the Chrome DevTools > Network > Throttling > Offline, I actually get a different error in the snackbar, and it happens instantly for me:

ImageCodecException: Failed to detect image file format using the file header.
File header was [0x3c 0x21 0x44 0x4f 0x43 0x54 0x59 0x50 0x45 0x20].
Image source: encoded image bytes

In terms of a more descriptive error we could return, I'm not sure what that might be. If there's no connection, that's the issue - but sometimes it appears as a SocketException, sometimes as you're finding it, and sometimes as other things. Open to changing error handling though.


Tile Layer reset does not reload tiles when the reset button is pressed, requires a pan for example, to reload. Until then, the whole map is grey.

We're aware that this has been broken, and we assume it broke with the TileLayer rewrites in v4 (or maybe v3, not sure?). Tbh, I'm not even sure the reset functionality is necessary anymore?


In Point to LatLng, is rotating the map by clicking the rotation button supposed to change the (center?) coordinate of the logo?

I think it's working as it's supposed to be.


Many maps, including OSM which we are using, require attribution which we don't always display in the examples.

We display it on the front page on startup, and wherever the attribution needs to be different. Looking at the more in-depth guidelines at https://wiki.osmfoundation.org/wiki/Licence/Attribution_Guidelines, we meet the requirements. The RichAttributionWidget was designed specifically with these requirements in mind.

Alternatively, the attribution may be placed adjacent to the map or on a splash screen or pop-up shown when a user starts the app, device, website, etc.

You may use a mechanism to fade/collapse the attribution under certain conditions:

  • immediately with a dismiss interaction, for example clicking an ‘x’ in the corner of a dialog
  • automatically on map interaction such as panning, clicking, or zooming
  • automatically after five seconds. This also applies to splash screens or pop-ups.

If the attribution has been collapsed, the user must still be able to find the licence information if they look for it, for example from an ‘(i)’ button in the corner of the map or an ‘About’ option in a menu.

If attribution is presented to the user upon application startup, it does not need to be presented to the user every time the user looks at or interacts with the application. (For clarity, when an application returns to the foreground from the background, startup attribution does not need to be presented again.)


In summary;

Things to do for sure:

  • Remove the current animations page from the example
  • Replace snackbars with an alternative when showing data that might change quickly

Still open questions:

  • Should plugins (such as the animation plugin) be shown in the example app?
  • Should we remove the CRS page(s?) from the example app?
  • How should we handle tile loading errors, both in the API and in the example?
  • Does the TileLayer reset API need to exist anymore?

@JosefWN
Copy link
Contributor Author

JosefWN commented Jun 4, 2023

Should we remove the CRS page(s?) from the example app?

I think it makes sense to have some custom CRS examples, my example (EPSG:3413) displays some tricky edge cases in that CRS to prevent bugs I've previously fixed from reoccurring over time. Then again they could be tests instead, but sometimes it also helps to have a visual presentation.

As for EPSG4326 specifically, I don't know how useful it is. If we decide to keep it we might try to get tiles that are better suited for that projection perhaps, at least with respect to the labels, and zoom the example in on a point where the distortion of the map itself isn't as apparent as it is when fully zoomed out. Could be less confusing.

@JosefWN
Copy link
Contributor Author

JosefWN commented Jun 4, 2023

Ah, also, regarding:

Should plugins (such as the animation plugin) be shown in the example app?

Since we are already keeping a list of plugins in the docs, we could probably delegate the responsibility of plugin usage to the plugins themselves? Less maintenance on our end. What I'm suggesting is simply removing the animations page from the examples might be sufficient.

@JaffaKetchup
Copy link
Member

Welp, I've got another 2 weeks of exams coming up, but afterward I might get some time to get round to all of this - anyone else feel free to jump in in the meantime!

@JaffaKetchup
Copy link
Member

The tile loading error handle gives the error Invalid argument(s): No host specified in URI in flight mode, which seems a bit cryptic. Failed DNS lookup? Perhaps we should return a more informative error to the library user, not just in the example but in general. Nit: Sometimes these UI notifications, just like the ones in the secondary tap example, load quite a bit later than when the event occurs (i.e. when I pan without internet connection, tiles fail to load, and after a number of seconds, I get the message).

May have been fixed by #1555 for #1554.

@JaffaKetchup JaffaKetchup added the P: 3 (low) (Default priority for feature requests) label Jun 19, 2023
@rorystephenson
Copy link
Contributor

I think plugins should not be used in the example app for a couple of reasons:

  1. When making breaking changes to flutter_map we would have to wait until the plugins in the example app have a compatible version released.
  2. It creates a maintenance burden, choosing which plugins to include and removing/adding them over time.

So I agree that we should just remove the animated map example and let users rely on the documentation for finding plugins. We could even suggest that plugin authors tag their plugins with flutter-map or flutter-map-plugin to help users find plugins on pub.dev.

I'm going to have a look at making some of the changes suggested in this issue (nice job on cataloging them all @JosefWN ).

@rorystephenson
Copy link
Contributor

I've opened a PR to address some of these changes, so far it removes the animated MapController and fixed the TileLayer reload.

Some comments:

I think these probably deserve a separate issue:

  • For the secondary tap notifications it's not the use of SnackBar that's the problem, at least not locally for me. I placed some print statements in the code and the secondary tap callbacks don't always fire when they should. So there seems to be a bug in the gesture detection.
  • Similarly the Tile load error callback notification delay is probably also not caused by using SnackBar. I think that sometimes the http request takes a while to fail and other times it doesn't. I don't know what the underlying reason is for that but I think as soon as it actually fails the notification appears. There are many things to consider on this issue e.g. should we wrap all tile loading errors in a TileLoadError exception, can/should we make tile loading fail immediately when airplane mode is enabled etc.

For examples which are confusing and/or not useful for users I think we should:

  • Add tests for all examples which exist to demonstrate an important behaviour and aren't already covered with a test.
  • Consider removing them. If it is an example which is only useful when working on a certain behaviour then you can temporarily add an example whilst developing and then add tests for it and remove the example when it is done.
  • Consider whether we want to have a separate test app which helps diagnose/demonstrate issues around some complicated parts of FlutterMap. An example would be a gesture detection example which gives info on what gesture was chosen and why. This means the example app can concentrate on just demonstrating features and how to implement them.

@JaffaKetchup JaffaKetchup linked a pull request Jul 16, 2023 that will close this issue
@JaffaKetchup JaffaKetchup linked a pull request Oct 7, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue requests a new feature P: 3 (low) (Default priority for feature requests)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants