Suggestions

[zafrirron]

Hi @dceejay, thanks for this great node!

I added few enhancements into my fork (in dev private branches now), before submitting yet another pull request , please consider it and I'll send pull requests according to your guidance:

1. Grid options: grid option by @cloudybay
2. Mouse coordinates box option: Coordinates box by @PowerPan
3. Improved measuring tool: Measure Tool by @ppete2
4. Tooltip option: Tooltip by @aratcliffe
5. Left button inject commands: enhancing currently limited context menu, by adding injection of commands to allow different map / server map entity interactions (additional dynamic buttons added to the context menu when pressed sends predefined commands)
6. Enhanced Add marker option: add configurable add marker form, allowing setting additional map entity attributes.

[dceejay]

Hi, thanks for the ideas. I'm a bit wary of adding too many options as they all need to be configured which is going to get confusing for users (the config dialog is getting a bit too crowded already...) but...

1. Grid - like it - May need to to think about styling a bit to make it work with the many maps available - like the default grey map layer.
2. Not so sure - What does this one look like ? I have experimented with several in the past and they all looked a bit rubbish, or took up too much space, or didn't seem to add much value, so I always ended up removing again.
3. Yes - looks good - prefer without the bearings. (presumably to replace the existing one - though I like the fact the existing one only needs one button on the map.)
4. Do we really need this ? We already have popup and label. (though not totally against. It is just another variation I suppose)
5. Not sure I understand ? any chance of a screenshot ? what sort of interactions do you have a use case for ? dm me @ gmail if you like
6. Again - screenshot, more details ? (but in theory definitely maybe, but I definitely don't want this too complex. I'd still like it to be useable when map is inside a widget.)

So I would say the priority order is PR 1,3 More info/discuss 4,6,5,2 And please PR them one at a time

[dceejay]

1. [ ] 1. PR for Suggestion 1 - https://github.com/dceejay/RedMap/pull/79 - closed (didn't work). May be revived as part of moving to leaflet1.4 work
2. [x] 2. Added as part of 7e70f169a05d747430777fac295655a27d75e5dd
3. [ ] 3. (not so sure about this one)- not really feeling the need - but... maybe PR
4. [x] 4. Added as part of 2de1b0bec514b7fab1155f7233af1aeeb46b14d0
5. [x] 5. .contextmenu option added as part of release 1.5.32
6. [x] 6. control.contextmenu added as part of release 1.5.34

[zafrirron]

Hi, thanks again for a great work, Few suggestions regarding "tracks" option:

1. Have tracks unique "tracks" layer (will enable simple hide/show all tracks).
2. Allow getting number of track points as input parameter (more node-red style?).
3. Allow some track styling...(dots/line; color...)

although I've done this in my private branch, my PR's track record are quite embarrassing to offer my help again.... :-(

[dceejay]

1. Surely you want track to stay with their marker so when you turn off that layer of markers their tracks go too ?
2. Input parameter to what ? You set them in the node edit panel (very Node-RED style :-). You don't want to add that to every input message ? Or did you just mean as an overall command message style ?
3. Like it.. - but again - not sure you would want this on every input message ?

Your PR was probably fine... - if I was on later leaflet and you had actually tried it against the shipped version :-)... There is a branch that now is work in progress to later leaflet - but it keeps blowing it's stack for some reason... doesn't like popups... so I can't release it in that state.

[zafrirron]

1. User preference (I use both options, hide markers layers with/wo tracks)
2. Some node red nodes have the option to set parameter value in node edit or override it with message (I guess you are correct in this specific node, setting the track history size in every message doesn't make sense)
3. same as 2...so if the user doesn't supply initial value he can set the value by message with property set (like setting url in http request node).

[dceejay]

So if you have a tracks layer for every other type of layer you could end up with 2x the layers ? eg ships and ships_tracks, planes and planes_tracks, etc ? If so, on leaflet 0.7 the layer control is not scrollable so you would need to not use too many layers or it won't fit on the page.

Digging into the code.. I see I can already set the tracks (and area) options, as they get picked up from the data anyway... so maybe just need more docs ?

    var opt = {};
    opt.color = data.color || "#910000";
    opt.fillColor = data.fillColor || "#910000";
    opt.stroke = (data.hasOwnProperty("stroke")) ? data.stroke : true;
    opt.weight = data.weight || 2;
    opt.opacity = data.opacity || 1;
    opt.fillOpacity = data.fillOpacity || 0.2;
    opt.clickable = (data.hasOwnProperty("clickable")) ? data.clickable : false;
    opt.fill = (data.hasOwnProperty("fill")) ? data.fill : true;
	if (data.hasOwnProperty("dashArray")) { opt.dashArray = data.dashArray; }

[zafrirron]

@dceejay thanks! I checked the "weight"option and it worked, I agree it needs some docs....(even after your response, it took me a while to figure out that sending a weight property of/for a marker should affect the marker track, its "inheritance" of properties.

regarding layer, I thought only about a single "tracks" layer containing all tracks of all moving markers...if and only if the user had set it in the node config form (and if users decide push too many layers into a map...)

[zafrirron]

@dceejay One other observation I had on "tracks" option, is that I need to create some kind "same location" filter. I'm not sure about other implementations, but I do have some cases, where some markers get redraw on same location (the use case is changing some other marker attributes or "wrong redraw" implementation). tracks however I expect to show last X marker locations, so currently, sending X times same location will "shorten" the track until it disappears... with filtering sending same location will not update the tracked location list of that marker.

[dceejay]

Thinking about it - it's a fairly simple option to add to the tracks node - to let the user select the behaviour... - as-is, a single "tracks" layer, or a tracks layer per marker layer. Then it's up to the user not to over clutter the menu :-).

[zafrirron]

looks FANTASTIC! (shame on me, I will need to code it into my forked version)....

[dceejay]

pushed as 1.5.37

[zafrirron]

@dceejay One other observation I had on "tracks" option, is that I need to create some kind "same location" filter. I'm not sure about other implementations, but I do have some cases, where some markers get redraw on same location (the use case is changing some other marker attributes or "wrong redraw" implementation). tracks however I expect to show last X marker locations, so currently, sending X times same location will "shorten" the track until it disappears... with filtering sending same location will not update the tracked location list of that marker.

@dceejay , I'de appreciate your view on this....(before I do something wrong....)

[dceejay]

will be easy to add a check in the track node to only update if lat or lon have changed.

[dceejay]

and actually - should also be possible to allow updates to objects by adding to them in-situ... ie be able to send just name plus new parameter without lat lon... by keeping a list of objects and just adding to them as info comes in. Would make changing icons or colours a lot simpler.... maybe.

[zafrirron]

Thanks!

One other suggestion (I'm using in my implementation), is adding the popup option to other map entities (like area, circle, line) similar to the marker popup handler (defaults to "name").... (I didn't try to also add the context menu option but this may also be considered).

[zafrirron]

For removing unwanted fields of the marker default info box (where popup option not used), I use an array (including the wanted fields names) a default one replaced by optional injected one, using this default/optional array saving the need to delete each option separately (just before building the "words" variable - for cleaner code, since using the new popup option is encouraged it mostly for wm developers convenience).

[zafrirron]

Also consider running HTML validator on index.html (use https://validator.w3.org), I've cleaned some errors using it....

[zafrirron]

The new added "feedback" function (sending map in actions), I suggest to allow other actions than "feedback" and add property (for backwards compatibility, I've added the two as last function parameters function(n,v,p="",a="feedback)) .

The use case: most convenient way to handle "Map In" node is to attach a "large" "switch" node immediately after ""Map In" switching based on "action" name...and use it to allow setting of markers properties(p parameter). so: a = action name defaults to "feedback" n = marker name p = marker property v = property value

[zafrirron]

Leaflet V1.4 BTW - after working extensively for quite some time already with V1.4, I have not experienced any issues related to v1.4 so far....

[dceejay]

yes I have a 1.4 branch (leaflet-upgrade) almost ready to go... just "baking" it for a while to check before releasing it as beta. Will run html check - but nothing serious so far. just w3c being picky. Other popups - yes maybe - no-one has asked for them so... Feedback - not quite sure I get the use case fully, yes I understand the switch part. Is this to save having another switch to switch based on all the "feedback"s ? Why is property needed as a separate property, why not make value an object ?

[zafrirron]

For the new context menu option, while most of the html panel configuration and parameters should be set by server nodes. The only internal "map" parameter is the click position (for markers usually its should be also "known" by the server). Worth mentioning (DOC) the ability yo use the internal rclk.lat, rclk.lng in the context menu callbacks...

Use case: Creating a customised "add marker" map context menu panel, that includes multiple marker properties, using context menu option, bypassing the default "Add marker" (addThing) rightmenuMap option.

possible bug: in map.on('contextmenu', function(e) {... there is assumption that field name 'rinput' exists, this may not be the case when using customised rightmenu (consider using try {}..catch{}...)

possible bug: in function setMarker(data) {... at the end of the function there is handler for if (popped == true) {...else {popmark.openPopup();... this code raises exception when opening a rightMenu on the map (not on marker...)

[zafrirron]

yes I have a 1.4 branch (leaflet-upgrade) almost ready to go... just "baking" it for a while to check before releasing it as beta. Will run html check - but nothing serious so far. just w3c being picky. Other popups - yes maybe - no-one has asked for them so... Feedback - not quite sure I get the use case fully, yes I understand the switch part. Is this to save having another switch to switch based on all the "feedback"s ? Why is property needed as a separate property, why not make value an object ?

HTML - yesy nothing serious for me only hels while debugging front end... Other Popups - I found that creating popups for drawing elements enables efficient map functionality to handle these entities (delete, hide, send, color, opacity, edit....some of my actions...) Feedback - regarding the switch, indeed, a cleaner node red implementation is a single switch handling all map in actions... Objects - Correct! creating object is a better option, thanks.

[dceejay]

so function(n,v,(optional a)) would be ok and then send {action:a||"feedback", name:n, value:v}

[dceejay]

PS - I published the v2.0.0-beta to npm which uses leaflet 1.4.x if you wish to try it.

[zafrirron]

so function(n,v,(optional a)) would be ok and then send {action:a||"feedback", name:n, value:v}

Sure, thanks!

[zafrirron]

PS - I published the v2.0.0-beta to npm which uses leaflet 1.4.x if you wish to try it.

Thanks, I'will. (unfortunately. since I already forked away....my merges back are taking time...)...

[dceejay]

I added the other grid overlay back now ... it works on leaflet 1.x . And added popups to lines and areas (as long as clickable is set true).

Happy for the suggestions in parallel, I'm on a slower path than you :-)

[zafrirron]

Thanks!

I'm not sure, but isn't leaflet rectangle object type handled by "worldmap"? (only as polygons?)... use cases:

1. Rectangle attributes are special (were bounding box needed)
2. I'm adding Editing option to all red injected objects, rectangle editing however is different than polygon's

my code: (requires adding 'type' attribute = 'rectangle') near: else if (data.hasOwnProperty("area") && Array.isArray(data.area)) {

change to:

 var polyarea;
if (data.hasOwnProperty("type") && data.type == "rectangle") polyarea = L.rectangle(data.area, opt);
else polyarea = L.polygon(data.area, opt);

BTW - the nice part of externalizing the "popup" is the ability now to inject many additional functionality with almost no code changes (the "editing" option for example was added by adding the js files and setting a popup command button to : "<button id='editzone' class='tcmd' onclick='polygons[\"{{name}}\"].enableEdit();'>Edit</button>"; were {{name}} is a generic replacer in the injecting node to set object name...

[dceejay]

why are rectangles special ? Editable looks interesting - might make for a better drawing layer than the existing draw plugin ? it is yet another 75k library though so would need to be a replace I think.

[zafrirron]

Rectangles: since leaflet draw has two different shapes for rectangles and polygons, maintaining leaflet conventions should be a good reason... Editing a rectangle is different from editing a polygon in all editing tools, also when returning a shape from drawing tool, the type of the shape might be of importance to the user (in my case I use the shape type to allow certain options based on shape type).

Editable: I found this extension easier to use for allowing editing to existing "worldmap" injected shapes by use of only a popup button and sending the edited shape to NR by the "feedback" function in the popup html: feedback(\"{{name}}\",\"SaveZ\",{{saveData}});

{{name}} - area/rectangle/circle name {{saveData}} - any worldmap latlang array (polygons["{{name}}"]._latlngs[0] for example)

Also Editable allows editing for its created shapes...but requires rewriting the NR feedback

Anyway - adding a usable drawing tool to "worldmap" should include the ability to edit existing shapes, draw new shapes, save an edited/created shape, cancel currently edited/created activity...

I suggest allowing handling a single drawing entity at a time only (so if I'm drawing a new shape I can only cancel or save it - return the data to NR for handling. but allowing creating multiple shapes on the map at the same time, doesn't make a lot of sense)