jieter / Leaflet.Sync

Synchronized view of two maps.
http://jieter.github.io/Leaflet.Sync/examples/dual.html
BSD 3-Clause "New" or "Revised" License
235 stars 53 forks source link

Synchronized cursors between the 2 maps #27

Closed sylvain-m closed 8 years ago

sylvain-m commented 8 years ago

Hello,

I'm no expert in Github, and this is not really an issue but a pull request (I didn't succeed to pull request for this script). I managed to integrate in a page 2 synchronized maps : thank you for this great script !! However, I miss a very useful feature: synchronized cursors between the 2 maps. These cursors allow you to accurately compare the location pointed to by the mouse. This is very useful for diachronic cards (comparing 2 dates).

Is this already possible, or is this an improvement to be made to script? Thank you for your help and congratulations for your great work!

Sylvain M.

sylvain-m commented 8 years ago

Here is an example of what i would like, based on OpenLayers 2 : http://geobretagne.fr/sviewer/dual.html

jieter commented 8 years ago

@sylvain-m nice suggestion. Since you are not providing a working solution, that makes this an issue and not a pull request (you requesting me to pull some code from your repository into mine).

Anyway, this might be an interesting feature to have. It's not possible in the current state of the script, but I guess it wouldn't be that hard to add it. Would you like to try to add it? I'm willing to help you in the process.

sylvain-m commented 8 years ago

Would you like to try to add it?

For sure ! But i'm not a real webmapper :-$

I adapt the codes I find here and there, but I'm not able to create them myself! The URL of my page is not yet public (there are still too many loose ends), but if I can send it privately, it would be very useful for our project. I understand that you agree to help me implement this function on my page? (I do not speak much English, so I'm afraid to misinterpret your proposal)

jieter commented 8 years ago

I would be willing to guide you while adding cursor sync to Leaflet.Sync, not that I'll do all the coding for you.

baipi commented 8 years ago

You can easily add it, could be something like that :


cursor1 = L.circleMarker([0,0], {radius:25, fillOpacity: 0.1, color: '#FFF48D', fillColor: '#FFFFFF'});
cursor1.addTo(map1);
cursor2 = L.circleMarker([0,0], {radius:25, fillOpacity: 0.1, color: '#FFF48D', fillColor: '#FFFFFF'});
cursor2.addTo(map2);

map1.on('mousemove', function (e) {
   cursor1.setLatLng(e.latlng);
   cursor2.setLatLng(e.latlng);
 });
map2.on('mousemove', function (e) {
   cursor1.setLatLng(e.latlng);
   cursor2.setLatLng(e.latlng);
 });
sylvain-m commented 8 years ago

Great !!! How quickly Baipi! I managed to integrate your code, and it works (almost) perfectly! There are just two small problems:

sylvain-m commented 8 years ago

I have solve the first point with this code

     cursor1 = L.circleMarker([0,0], {radius:25, fillOpacity: 0.1, color: '#FFF48D', fillColor: '#FFFFFF'}).addTo(map1);
     cursor2 = L.circleMarker([0,0], {radius:25, fillOpacity: 0.1, color: '#FFF48D', fillColor: '#FFFFFF'}).addTo(map2);
     map1.on('mousemove', function (e) { cursor2.setLatLng(e.latlng); cursor1.cursorr.setLatLng(e.latlng); });
     map2.on('mousemove', function (e) { cursor1.setLatLng(e.latlng); cursor2.cursorr.setLatLng(e.latlng); });

Second point is still here : cursor of the "real" mouse on the screen is a hand and not a circle

jieter commented 8 years ago

The next step: integrate this cleanly into leaflet.sync code... ;)

baipi commented 8 years ago

Had a CSS class to your circleMarker with the property "cursor" at "none". http://leafletjs.com/reference.html#path

JS:

cursor1 = L.circleMarker([0,0], {radius:25, fillOpacity: 0.1, color: '#FFF48D', fillColor: '#FFFFFF', className: 'noCursor'}).addTo(map1);

CSS:

.noCursor{
  cursor:none;
}
sylvain-m commented 8 years ago

Is JS for cursor1 only, or for both cursor1 and cursor2 ? (Sorry, I'm really not good in javascript!)

baipi commented 8 years ago

No problem :) Do it for both, it should works :)

sylvain-m commented 8 years ago

I tried, but did not succeed... Here is the URL of test web page : [URL deleted]

(i will suppress it after you saw it)

jieter commented 8 years ago

hmm, you problem is the circlemarker on the map you are moving over doesn't get updated.

map1.on('mousemove', function (e) { cursor2.setLatLng(e.latlng); cursor1.cursorr.setLatLng(e.latlng); });
map2.on('mousemove', function (e) { cursor1.setLatLng(e.latlng); cursor2.cursorr.setLatLng(e.latlng); });

You should remove .cursorr twice. Also note the error messages in the console (press F12 in your browser): Uncaught TypeError: Cannot read property 'setLatLng' of undefined, repeatedly logged as you drag the map.

baipi commented 8 years ago
cursor1 = L.circleMarker([0,0], {radius:25, fillOpacity: 0.1, color: '#FFF48D', fillColor: '#FFFFFF', className: 'noCursor'}).addTo(map1);
cursor2 = L.circleMarker([0,0], {radius:25, fillOpacity: 0.1, color: '#FFF48D', fillColor: '#FFFFFF', className: 'noCursor'}).addTo(map2);
map1.on('mousemove', function (e) { 
    cursor2.setLatLng(e.latlng); 
    cursor1.setLatLng(e.latlng); // Not cursor1.cursorr.setLatLng(e.latlng);
});

map2.on('mousemove', function (e) { 
    cursor1.setLatLng(e.latlng); 
    cursor2.setLatLng(e.latlng); //  Not cursor2.cursorr.setLatLng(e.latlng); 
 });
baipi commented 8 years ago

@jieter is faster :)

Je vois que ça marche !

sylvain-m commented 8 years ago

Great !! Thank you both ! It is on: how effective and fast! Another good example of the effectiveness of Open Source, I will not fail to promote!

jieter commented 8 years ago

@sylvain-m

This is a rough implemenation in Leaflet.sync: https://github.com/turban/Leaflet.Sync/blob/sync-cursor/L.Map.Sync.js#L35-L52 It would be nice if someone is willing to complete that. @baipi ?

baipi commented 8 years ago

@jieter I'm looking for it as soon as possible :)

jieter commented 8 years ago

nice. Some considerations:

RalucaNicola commented 8 years ago

I tried to solve the unsync part (as an exercise to practice my beginner JavaScript skills), but I am not sure if it's correct. Could any of you guys have a look at my code?

https://github.com/RalucaNicola/Leaflet.Sync/blob/new-sync-cursor/L.Map.Sync.js In the unsync method I just remove the cursors corresponding to the map. Not sure however that this is the "proper" way: @jieter You mentioned that the event listeners should be removed, but if there is more than one cursor in the array, removing the event listener will stop the other cursors from working.

And one question: why do we need this mouseout event? If the mouse leaves the map, the cursor leaves the map too, which is ok...isn't it?

jieter commented 8 years ago

@RalucaNicola can you open a pull request, that will make it easier to review your patch.

RalucaNicola commented 8 years ago

@jieter I would, but I was fighting some detached HEAD last night, so the only way that worked for me to add my code was to make a new branch (new-sync-cursor). Should I open a pull request anyway? Basically I just added these 3 lines to remove the cursors: https://github.com/RalucaNicola/Leaflet.Sync/commit/99e13743745fca4c49e0e6aa8cf0e6183626a598

jieter commented 8 years ago

@RalucaNicola yeah, open a PR anyway. That will trigger the unit tests too. Would also be nice if you could add some unit tests to test this cursor syncing, or at least the proper cleanup after unsyncing.

Don't be afraid to propose incomplete changes, we can discuss and improve as we go.

RalucaNicola commented 8 years ago

@jieter ok, I hope I did it right, this was my first pull request https://github.com/turban/Leaflet.Sync/pull/29 it shows that some check failed though...I also tested if it works in the examples. Thank you for reviewing it!

jieter commented 8 years ago

closed via #30 and 0a9261dffa23dd52899ad452d5892242b2ed9a32