Clean the "update" event signature

[Indigo744]

This is a ticket to talk about the event signature.

In #85, you said you wanted to clean this signature. I agree it became quite complicated and an overhaul would be good.
Before:

$(".container").trigger('update', [updatedOptions, newPlots, deletedPlots, opt]);

After:

var options = {
  'mapOptions' : {}, // was updatedOptions
  'replace' : true/false // whether mapsOptions should entirely replace current map options, or just extend it,
  'newPlots' : {},
  'newLinks' : {},
  'deletedPlots: [],  // was opt.deletedPlots
  'deletedLinks' : [],  // was opt.deletedLinks
  'setLegendElemsState' => '...'  // was opt.setLegendElemsState
  'afterUpdate' => ...,  // was opt.afterUpdate
  'animDuration' : ...  // was opt.animDuration
};
$(".container").trigger('update', [options]);

Several questions:

• shall it be backward compatible ? I think not. It would be hard to maintain.
• what happened to resetPlots/resetAreas ? Actually, I never quite understood their purpose...
• deletedPlots and deletedLinks could better be called to reflect what it is. Like DeletePlotNames and DeleteLinkNames?

[neveldo]

Hello,

shall it be backward compatible ? I think not. It would be hard to maintain.

I agree with you, we could forget the backward compatibility.

what happened to resetPlots/resetAreas ? Actually, I never quite understood their purpose...

The initial purpose of these options was to be able to reset all options currently set for the plots, areas and links before updating them. For example, you add a plot with fill:red and opacity:50%. If you update that plot with fill:blue, the plot will be blue but still with an opacity of 50%. By setting resetPlot to true, the previous opacity option would be removed at update. It's a bit weird because resetXXX doesn't delete anything from the map.

This is why I have suggested a more straightforward option 'replace' that could also be named 'clear' or 'reset' which the purpose would be to reset the map at it initial state (by deleting all plots, links, areas, legends, clearing current options object, etc). But I'm wondering if such an option would be usefull, because it can be achieved easily by emptying the container through jQuery and by calling again $.mapael() on it to recreate the map with the new configuration.

deletedPlots and deletedLinks could better be called to reflect what it is. Like DeletePlotNames and DeleteLinkNames?

I agree with you, but a prefer 'deletePlotKeys' or 'deletePlotIds' (the same for the links). What do you think ?

By the way, maybe I should open a new issue for that, but I was thinking about the 'setLegendElemsState' option today. I think it should be great to be able to tell which kind of elements we want to hide or show : 'areas', 'plots' or 'links', and for which interval of values (if the elements are associated to one or more value(s), it has to be compatible with multiple valued elements). For example : "I want to show only the plots which the associated value is between 1000 and 1200) :

{ 'plot' : {'min': 1000, 'max': 1200},  'area' : {'min': 10, 'max': 20}}
{ 'plot' : 'all'} 
{ 'plot' : 'none'}
(...)

It would allow to play with some sliders in order to allow users to select ranges of values for which we want to show the elements. What do you think about it ?

[neveldo]

I just saw your PR #106 that add "all" value for deletedPlots and deletedLinks options. Maybe this is the good alternative for the resetPlots and resetLinks options ?

[Indigo744]

I agree with you, but a prefer 'deletePlotKeys' or 'deletePlotIds' (the same for the links). What do you think ?

I do agree with deletePlotKeys :-)

I just saw your PR #106 that add "all" value for deletedPlots and deletedLinks options. Maybe this is the good alternative for the resetPlots and resetLinks options ?

Yes, now that I understand the concept behind the reset options, I think this is more straightforward indeed to offer a delete all option.

By the way, maybe I should open a new issue for that, but I was thinking about the 'setLegendElemsState' option today. I think it should be great to be able to tell which kind of elements we want to hide or show : 'areas', 'plots' or 'links', and for which interval of values (if the elements are associated to one or more value(s), it has to be compatible with multiple valued elements).

Nice idea! But we should open a new issue to discuss its implementation indeed.

[Indigo744]

Here is a proposal for the new signature:

var options = {
  mapOptions: {},             // was updatedOptions
  replaceOptions: false       // whether mapsOptions should entirely replace current map options, or just extend it,
  newPlots: {},               // was newPlots
  newLinks: {},               // was opt.newLinks
  deletePlotKeys: [],         // was deletedPlots
  deleteLinkKeys: [],         // was opt.deletedLinks
  setLegendElemsState: true,  // was opt.setLegendElemsState
  animDuration: 0,            // was opt.animDuration
  afterUpdate: function(){}   // was opt.afterUpdate
};
$(".container").trigger('update', [options]);

[neveldo]

Hello,

Nice idea! But we should open a new issue to discuss its implementation indeed.

You are right :-)

I agree with your signature proposal, I think it the "udate" event will be a lot more clear and clean with this new signature !

[Indigo744]

FYI, I opened the corresponding issue here: #110

[Indigo744]

I just saw that in your website, you are saying:

To download mapael, go to the GitHub repository.

http://www.vincentbroute.fr/mapael/#overview

I fear if they come here and download the main/latest, they will face all the breaking changes we just introduced.
Hence, my suggestion is:

• do not ask to come to the GitHub repo to download, provide directly the link to the supported version (like with the blue button above, https://github.com/neveldo/jQuery-Mapael/archive/1.1.0.zip)
• or move the development of the new version in a "next" branch

Personally, I prefer the first point for now. What do you think?

[Indigo744]

Also, regarding these lines:

if (typeof opt.mapOptions === "object") {
    if (opt.replaceOptions === true) options = opt.mapOptions;
    else $.extend(true, options, opt.mapOptions);
}$

I wonder if options = opt.mapOptions; shouldn't be:

options = $.extend(true, {}, Mapael.defaultOptions, opt.mapOptions);

What do you think?

[Palakila]

Hi there. I am new to mapael and btw really like the library (and have been able to impress a lot of people with it, nice work!), but I am facing this dilemma that Indigo744 mentioned about breaking changes. I was using the released version 1.1.0 up to early this week but needed the fixes for the ability to delete dynamically created links and the support for the 'all' behavior when deleting plots and links.

So I switched to referencing the master version but got broken when the update signature changed. My current solution was to take a permalink of the version just before the signature change but was wondering if this is the right approach also. Would be great if breaking changes like the function signature one for example would be done on a separate branch but keep the bug fixes to existing version in master or a released branch. Bottom line is that would be great to be able to benefit from the bug fixes on the currently release but protected from breaking changes.

Thanks

[Indigo744]

To be honest, the 'all' behavior is a new feature and not a bug fix. Hence, mapael was not updated to 1.1.1 or 1.2.0. The currently supported version for production is still 1.1.0 for now. This master branch is inherently in development. Playing with a version in dev is done at your own risks!

That said, you are right we should have be more careful with this, and that's why I brought up the point here. Next time we could use a specific branch.

The only approach you have here is the one you said. A proper update of your software will be needed later when the new mapael version will be released.

[neveldo]

Hi,

I'm glad you appreciate mapael ! As Indigo744 said, the master branch should be used in production at your own risk. However, I agree with you, when developping the next major versions of the plugin, a better approach will be to create a separate branch in order to be able to to maintain the current stable version with bugfixes.

[Indigo744]

@neveldo, did you also see this comment ? #105 (comment)
Just to be sure ;-)

[neveldo]

Indeed, sorry I missed this comment ! You are right, it should be options = $.extend(true, {}, Mapael.defaultOptions, opt.mapOptions);, I just fixed the line : d665d8d

You are also right regarding the download link that could be misleading, so I have changed the target to a direct download of the 1.1.0 version : http://www.vincentbroute.fr/mapael/ ( neveldo/mapael-documentation@2658ed0 ). By the way, the missing files in the documentation repository are coming soon :)

[Indigo744]

I think we can close this issue now :-)