Network: Improve option handling

[wimrijnders]

This issue has been created to discuss the improvement of the option handling for network. The option handling is complex and diffuse, multiple methods of handling options are intertwined. This can use a thorough cleanup to make it more manageable.

The following comments collects notes from a discussion in the lobby between @rasmusblockzero and self.

[rasmusblockzero]

@wimrijnders do you except this to affect the nodesHandler option handling too? If so it will affect public APIs and probably should be flagged with that ?

[wimrijnders]

@rasmusblockzero Yes, I expect it to affect all option handling within network. The trick is to make it so that the API does not change!

[rasmusblockzero]

one possibility

Prototype chaining

The various option groups are kept separate and linked to each other via the prototype construction inherent to javascript.

For a given node (edge comparable), the chain would be:

node options -> group options -> nodesHandler options -> default options

...for every node in a network.

The chaining can be done with existing function util.bridgeObject(). It is currently used to connect nodeHandler.options and node[i].options.

PROS

• After the chains have been set up, usage of the options is completely straightforward. Retrieving an option becomes a simple, intuitive action.

CONS

• When options change, you'd have to redo the entire chain.
• Options are hierarchical. To make this fully work, it's necessary to chain the prototypes at the deeper levels as well. Update: util.bridgeObject() does this already, this is not a problem.

Further Considerations

• Usage of util.bridgeObject is partially implemented, in the sense that code exists which works around the proto chains.
• This solution is elegant but had a decent threshold to comprehend and is not the easiest to debug.
• Special cases: e.g. multi-fonts have a fall back to their parent option. Update: This can be solved by adding an access function for fonts.

[rasmusblockzero]

possibility two

Copying/merging/flattening

Instead of chaining the values can be copied/merged each time update comes so that the node's this.options is a complete representation of the values.

Comparing to the current code, it would make the implementation more similar to the formattingValues (and replacing those), and it would remove the need for a special flattening in the nodes (getFormattingValues()).

PROS

• It's straightforward to read and debug, one structure with all the values - no chaining
• It is pretty easy to handle all the cases of settings that can be made in different forms

CONS

• It's likely a bit less performant than chaining - particularly when changes are done on the nodeHandler and all node need to copy the new value

Further considerations

• it would force all implementations to consider different structures in the incoming options.
• It would possibly put some new demands on the applications to provide all font values when it want to update 'enable'

[rasmusblockzero]

third possibility

NOTE: This approach is not incompatible with the first option Prototype chaining. It is possible to use the chaining and add utility functions for the 'hard' parts.

Separate Options class

A NodeOption class that would be instantiated for each level and having a reference to the one above. This is a bit like the chaining but can have build in knowledge about the rules for the more complex options.
The class will have a a more elaborate get function that request parent object(s) if hasProperty doesn't exist.

E.g. if the nodeOption.shadow.enabled is set to false the object knows that the getter for shadow.color should return null - or ask the nodesHandler.shadow.color or throw an exception (depending on the specified rules). Another example would be that the nodeOption.image would return the selected image when selected - if such exists.

PROS:

• A lot of the logic can be gathered in one place, making the draw and measures outsire of the options cleaner
• This would also mean that the inheritance rules and the rules between different sub-values (selected etc.) would be more explicit

CONS:

• It would be one more structure to maintain parallel to the ones between node, groups, nodeHandler
• It might be more confusing in the draw calls why the selected version of the values are magically used (but this already exist with the parsing of getFormattingValues())

[rasmusblockzero]

fourth possibility

Make checks more explicit at each place it's used

A more simple approach, make all options available through out the process so for each place in the draw there has to be if cases for if the value exist in any of the levels

PROS:

• No magic, easier to learn

CONS:

• A lot of copy and paste all over the place, most likely really messy

[rasmusblockzero]

@wimrijnders I don't think the public API need to change but the outcome of certain actions might change, right? It will not be backward compatible in a strict sense. Even if I think it's better and I think that all applications can adapt to these changes shouldn't that better be flagged?

[rasmusblockzero]

@wimrijnders I don't understand "If you don't mind doing the extra work, you can also move over the the text for options Copying/merging/flattening, Separate Options class and Make checks more explicit at each place it's used to each of these comments."

[wimrijnders]

@rasmusblockzero That was a way of asking you to do the text copies, in a polite manner. Which you did, so thank you! 😄

[wimrijnders]

I don't think the public API need to change but the outcome of certain actions might change, right?... shouldn't that better be flagged?

Yes, definitely. In practice, this means that at least the MINOR-release version will go up.

[rasmusblockzero]

right

…

On 22 May 2017 at 08:45, wimrijnders ***@***.***> wrote: I don't think the public API need to change but the outcome of certain actions might change, right?... shouldn't that better be flagged? Yes, definitely. In practice, this means that at least the MINOR-release version will go up. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3060 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ASqgQ7NGn-0ppZu7vAdJ7nu5NrB9uYiPks5r8S7zgaJpZM4Nd8pg> .

--

------------------------------ Rasmus Hedin CTO and CoFounder Block Zero +46706066776 rasmus@blockzero.se

[wimrijnders]

@rasmusblockzero I just added following in the prototype chain comment:

UPDATE: util.bridgeObject() does this already, just verified this.

It appears that my largest worry (chaining of option members) is unfounded. This is good news; it means that full prototype chaining is easier than I thought.

[rasmusblockzero]

Yes, Sorry. I could have explained that better that is what my understanding was too.

Are you then set on using the chaining?

There might be a hybrid between chaining and separate class structure:

• use chaining but attach a getFont property feature to each object that can do some clever introspection.

[wimrijnders]

Are you then set on using the chaining?

It's not a hard target. I just think it's the most elegant solution.
Edit: From #3025, I understand that it was actually also the intention of the original developers. Which gives it some support.

There might be a hybrid between chaining and separate class structure

👍 I have no problem with this at all. It is, in fact, a good idea.
It can work because util.bridgeObject() creates a completely new object for every object passed into it. It is a simple matter to attach some useful utility functions onto it.
Hold this thought. I like it.

[wimrijnders]

@rasmusblockzero Take a look at Network.setOptions(), roundabout line 187, the block starting with:

    // if the configuration system is enabled, copy all options and put them into the config system
    if (this.configurator && this.configurator.options.enabled === true) {

This looks suspiciously like flattening, i.e. option 2 above. Do you agree?

The result is used to initialize this.configurator. Still, I would hope that the flattening is not necessary.

[rasmusblockzero]

I believe that this is connected to the built in configurator only
http://visjs.org/examples/network/other/configuration.html

I think the deepExtend is a recursive copying function yes, but not flattening.

The configurator is cool, it's a bit odd to have it built into the framework IMO but I can totally see the point of if.

[wimrijnders]

The conclusions of this thread:

• Prototyping is the decision make by the original coders and is therefore by default the way to go.
• For the hard stuff, attach a property feature to each object that can do some clever introspection, e.g. getFont() for font handling.

This will be the driving principle for further work on the options. In general, this means cleaning up the code breaking the prototyping principle and see if this can be modified to smart properties.

@rasmusblockzero, thanks for your input here!