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

Fix gray borders #1211

Merged
merged 4 commits into from
Apr 9, 2022
Merged

Fix gray borders #1211

merged 4 commits into from
Apr 9, 2022

Conversation

ibrierley
Copy link
Collaborator

This introduces a maxBounds parameter (I believe leaflet.js uses this param, so tried to be consistent with naming). This will take a LatLng Bounds object, and try to make sure that the map display area isn't outside the bounds.

It will try and "adjust" to the nearest edge point, rather than just failing, to allow smoother dragging if it would otherwise be too small.

Use LatLngBounds(LatLng(-90, -180.0), LatLng(90.0, 180.0)) if wanting to contain the full map, and have no gray borders, otherwise a smaller area to contain.

@JaffaKetchup JaffaKetchup self-requested a review April 8, 2022 20:30
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Looks OK to me on Web. I note that the map can no longer scroll horizontally infinitely, which might be important to some people - see #1201?

@ibrierley
Copy link
Collaborator Author

Fair point (assuming you mean the extra wrap or whatever its called as opposed to infinite, I think infinite will need a separate "fix" in general).

@JaffaKetchup
Copy link
Member

In that case, I'm happy for you to merge this 👍

@stx
Copy link

stx commented Apr 8, 2022

Works great. Excellent work @ibrierley!

@ibrierley
Copy link
Collaborator Author

@JaffaKetchup I just tested it a bit further what happens normally, with the extended pan. I think you can only get to see those extended east/west parts if you have a grey border at the top/bottom. So I think this is as ok as it probably can be for the moment.

So I think let's merge this as it's an added feature and shouldn't affect existing uses, and if we come along to things like getting infinite scroll, or someone has some other related issue around this, we can try an adapt. I do think your point about that is relevant

@ibrierley ibrierley merged commit 895fca8 into fleaflet:master Apr 9, 2022
@JaffaKetchup
Copy link
Member

@ibrierley I'm just updating the documentation. What is the difference between bounds and maxBounds now? Does bounds even work? Thanks.

@ibrierley
Copy link
Collaborator Author

Weird, it was working earlier, I tested it. Now I can't get it to work, even checking out earlier versions from ages ago, so think I'm missing something. I'll try and debug this eve if I get chance. (I note mapController bounds example works, but the map options feels broken or something).

@ibrierley
Copy link
Collaborator Author

Or was I testing maxBounds, and mapOptions bounds hasn't worked for ages. It will be good to try and figure this out anyway.

@JaffaKetchup
Copy link
Member

Interesting. Just wondering though, what should the difference be? They sound like they'd do the same thing.

@JaffaKetchup
Copy link
Member

Yeah, I think bounds is broken and maxBounds works. For the time being, I'll remove bounds from the docs.

@ibrierley
Copy link
Collaborator Author

Just did a bit of digging. Basically, bounds is making the map initially fit to those bounds. I.e to focus on an area (but not restrict) like London. Ideally it should be called fitToBounds or something, it will create the lowest zoom and center to fit in those bounds....and doesn't currently work(with our examples anyway), I wonder if anyone uses it :D. (Note it works in the mapController params). I'd be interested to know if anyone does use it.

maxBounds forces the map/tiles not to go outside of the area of those bounds. So you could set it to LatLngBounds(LatLng(-90,-180),LatLng(90,180)) and it will make sure the users visual area doesn't go outside of those bounds to prevent a grey area, or smaller bounds to restrict to a more specific localised area (eg London), so you could never view any other area outside of it.

I think the problem with bounds/fitToBounds on the Map side (and why mapController bounds works), is that when the map is initialised, it has no valid widget size (but it does a moment later), so it tries to figure how to fit the map into those bounds, but as it has no size to calculate from, it produces garbage which is later rejected. As mapController normally takes a short while to sort itself out, it can probably get a valid widget size by the time it runs and works.

We can fix it if wanted, but I don't like this fix really (happy to be convinced otherwise!), maybe as it feels like its in the wrong place or something..but feels like it should be fixed by widgets, not staged checked code....

void fitBoundsIfLateInit(BoxConstraints constraints) {
    if(options.bounds != null && mapState.originalSize != null &&
        mapState.originalSize!.x == 0.0 && constraints.maxWidth != 0.0) {
      mapState.setOriginalSize(constraints.maxWidth,constraints.maxHeight);
      mapState.fitBounds(options.bounds!, options.boundsOptions);
    }
  }

If that is plopped into flutter_map_state.dart and called inside the LayoutBuilder, eg...

LayoutBuilder(
        builder: (BuildContext context, BoxConstraints constraints) {

          fitBoundsIfLateInit(constraints);

I wish I could figure a more elegant solution, but my brain isn't very good with delayed widget sizes and stuff. It's possible it currently works for some people who have a tightly controlled widget sizing, and our examples in a Column confuses it at first, as I think that can cause sizing issues at first (again, I don't know enough on the intricacies of nested widget sizing) .

@JaffaKetchup
Copy link
Member

I see. Maybe for the moment we'll just leave it broken (or you can apply your patch). Really some work needs to be done on making FlutterMap less asynchronous somehow - I think I remember discussing that with @kengu.
We almost need to have a loading widget with no fixed size or size usage (just very simple), then load in the map (with more complex sizing usage) when possible and everything is ready.

It's up to you, but I'll just open an issue for now to make it known.

@stx
Copy link

stx commented Apr 11, 2022

We use bounds: to set the initial camera position and then we update it later via controller.centerZoomFitBounds -> controller.move if the bounds change. It works perfectly, including with this PR.

@JaffaKetchup
Copy link
Member

@stx How do you specify it's sizing? @ibrierley mentioned that it might happen depending on how it's laid out (Column or SizedBox for example).

@stx
Copy link

stx commented Apr 11, 2022

@JaffaKetchup We use it many ways - Expanded() within columns on mobile app screens, fixed width/height on web, etc. - works great everywhere.

@JaffaKetchup
Copy link
Member

Hmm. @ibrierley did you have the code snippet you tested it with?

@stx
Copy link

stx commented Apr 11, 2022

If it helps, here's the helper function we use to specify the LatLngBounds:

  /// Returns the bounds from a list of [LatLng] points.
  static LatLngBounds getBoundsFromLatLng(List<LatLng> list) {
    double? x0;
    double? x1;
    double? y0;
    double? y1;

    for (int i = 0; i < list.length; i++) {
      LatLng latLng = list[i];

      if (x0 == null || x1 == null || y0 == null || y1 == null) {
        x0 = x1 = latLng.latitude;
        y0 = y1 = latLng.longitude;

        continue;
      }

      if (latLng.latitude > x1) {
        x1 = latLng.latitude;
      }
      if (latLng.latitude < x0) {
        x0 = latLng.latitude;
      }
      if (latLng.longitude > y1) {
        y1 = latLng.longitude;
      }
      if (latLng.longitude < y0) {
        y0 = latLng.longitude;
      }
    }

    return LatLngBounds(
      LatLng(x1 ?? 0, y1 ?? 0),
      LatLng(x0 ?? 0, y0 ?? 0),
    );
  }

@ibrierley
Copy link
Collaborator Author

I just tested it with the examples home page. One thing as mentioned I dug out, was that certain outer widgets can cause issues, like Columns which the examples use (hard to know if thats relevant here). I tried to add an Expanded widget which one SO answer to a similar problem suggested, but that was outside the Layout builder which may not help.

It could be, try it without our examples, that it works ok...but either way, I don't really like a solution that isn't solid for a user in all cases if that makes sense (we've had enough grief with mapController bugs :D).

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.

3 participants