-
Notifications
You must be signed in to change notification settings - Fork 927
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
Use baselayerchange/overlaylayerchange instead of layeradd/layerremove #5474
base: master
Are you sure you want to change the base?
Conversation
|
I also saw a problem in PR: when you click the Map Data checkbox, the cookie with the current location is no longer updated |
a359de9
to
a32249e
Compare
Okay, firing events from I also replaced |
a32249e
to
5b37b54
Compare
With the latest changes I was able to render Lake Huron locally in a reasonable amount of time (<20s, including 4.5s API response time; 455k nodes). Also map data display is extremely fast now (e.g. 100ms instead of 3s). That is definitely a significant speed-up. Ideas for follow up PRs:Furthermore, switching from XML to JSON has improved the response time from 20s to 8s (both values include 4.5s API time). For reference, I've uploaded the quick hack here: https://gist.github.com/mmd-osm/d45ff3b4f41908b1c075d70d1b6fdebe (edit: added fetch based + error handling alternative). We should think about adding some error handling: |
Jquery uses In a next step switching to |
5b37b54
to
30403cb
Compare
We probably need to fire If we follow the same principles as |
Do I understand correctly that you are talking about the current behavior of the website, and not after PR? At least in Firefox, changing |
@AntonKhorev 's comment wasn't exactly clear to me either. I think it's the following. Note that once you change the layer in the URL (in the example below from "C" to "H"), the share link "layer" attribute is updated accordingly: This doesn't seem to work anymore with the changes in this PR. Besides, there may be some timing issue in the current code, as the behavior is not 100% reproducible for me, even today. Sometimes the map layer changes along with the share link, in other cases, the old layer is still shown. |
Ah, I understand. I only tested changing the letters that represent the overlay (D/N/G). Changing the letters that respond to tile layers works now. PR broke it. I'll try to fix it |
Leaflet ships with a layers control which lets you switch between layers. That thing is responsible for firing
The problem is that our layers menu is not the only thing that can switch layers. All other places where layer change happens need also to fire Similar story with |
…e for speed up Map Data layer render
30403cb
to
ad9dcfe
Compare
Description
This is fix #5466 After merging openstreetmap/leaflet-osm#43 flame graph looks like this:
https://share.firefox.dev/4a2Wc9Z
Most of the time is spent on two handlers that update information about the current location and elements in the right sidebar.
Unfortunately, replacing
layeradd layerremove
withbaselayerchange overlaylayerchange
(#5466 (comment)) is difficult, since in addition to the Map Data layer, other elements may be displayed on the map. For example, the currently selected object on the map. It is not clear how to separate changes to Map Data objects and changes to the selected element. It is necessary to track changes in the active element, otherwise at least the Edit button will break.Therefore, I implemented a little ugly, but safer in terms of breaking business logic. When adding the Map Data layer, I set the pause flag in the slowdown handlers. While the Map Data is rendering, the user cannot interact with the interface, so it seems safe to pause the event handlers.Total 15s vs 0.5s when rendering 5000 elements.
https://share.firefox.dev/4h4IK7C