Network: Fix handling of multi-fonts #3486

wimrijnders · 2017-09-26T07:56:32Z

This PR serves to get the handling of the font options right. The fixing of the issue is more or less a side-effect. As a special note, with this PR the group options for fonts are finally handled correctly.

Guiding principles:

Option values should not be changed inline. They should remain as entered.
Priority order for options should be respected: node, group, default node, global default.

Changes

Got rid of Label#parseOptions(). This changes the option values, and we want to keep them as
specified.
Adjusted usage of groups, at least for the font options in groups.

Group font options were merged into the options of a given node instance, and the label
handling requires these to be separate. In fact, it is questionable if the group options
should be merged into the node instance options at all.
Unit tests for the setting of font options in all manners have been added.
Special Note: The added unit tests picked up an unrelated bug in util.deepExtend() and util.selectiveDeepExtend(); added an extra check on null there.

Further changes

Removed member edgeOptions in module EdgesHandler, it's just a reference to this.options
Removed member nodeOptions in module NodesHandler, it was useless for the same reason
Added/edited commenting extensively
Completely rewrote Label.propagateFonts(), additional notes :
- Special handling in the old code of multi-font mono has been dropped; if users want to
  have a non-proportional font for mono, that's their problem.
- Special handling of ratio has been dropped. This does not have any effect on the outcome. The value of ratio was always 1 in all the cases I have conceived to test.

Final Comment

If anything is to be concluded from this fix, it's that the option setup and handling within
Network is unnecessarily complicated. This urgently needs to be looked at.

In particular, the prototype chaining of options appears to be deficient. While the idea is
elegant in itself, the prototype order is not sufficient to handle all option sequences; in fact,
the code is rife with exceptions on it. Prototype chaining needs to be replaced with something
probably less elegant but better suited to the task.

This is the next escalation on the war against the Travis unit tests failing (which came into being by yours truly). By accident, I could recreate the unit test failure on my development machine. This led to a more directed effort to squash the bug. The insight here is that test `(window === undefined)` fails, but `(typeof window === 'undefined`)` succeeds. This undoubtedly has to do with the special status `window` has as a global object. Changes: - Added check on presence of `window` in `Canvas._requestNextFrame()`, fixed local source errors. - Added catch clause in `CanvasRendered._determinePixelRatio()` - small fix: raised timeout for the network `worldCup2014` unit test

- Small fixes and cleanup in `util.js` - Removed `util.protoExtend()`, not used anywhere

yotamberk

Really minor changes.
All in all, really good job! Cleans up the code a whole lot!

yotamberk · 2017-09-26T13:32:06Z

lib/network/modules/Groups.js

   */
-  get(groupname) {
+  get(groupname, create = true) {


I personally like more declarative variables. I think it should be: create => shouldCreate

yotamberk · 2017-09-26T13:45:04Z

lib/network/modules/NodesHandler.js

 var Node = require("./components/Node").default;
-var Label = require("./components/shared/Label").default;
+


You mean that there are two empty lines? I actually prefer this, for better visual separation. I do it between methods as well, and in other places where I think a bigger distinction between blocks is useful.

yotamberk · 2017-09-26T13:55:41Z

lib/network/modules/components/shared/Label.js

+    * @param {MultiName} multiName sub path for the mod-font
+    * @returns {ModOptions}
+    */
+    var getFontOptions = (multiName) => {


Why do you use var? Why not use const and let?
(for consistency)

A quick response on this one in passing:

I prefer var because chrome has issues dealing with const and let, it can't seem to display the values correctly inline (usually shows undefined even though it has a value). I find this bothersome to the point of forsaking them, even though let would be better.

If you have a solution for this, let me know.

But it seems that you douse let in the rest of the file. How does that work?

I really try to use let. But if it gets in the way of debugging, I revert it to var.

Oh well, I can change this since I'm done with debugging.

(I hope)

Update: Did var => let for `Label.js.

yotamberk · 2017-09-26T13:57:12Z

lib/network/modules/components/shared/Label.js

+
+    var fontString = "";
+    if (values.mod !== undefined && values.mod !== "") {  // safeguard for undefined - this happened
+      fontString += values.mod = " ";


I think you mean fontString += values.mod + " " ? It seems really weird to me if you meant to assign values.mod = " " and simultaneously add a space to fontString

Indeed. Good catch.

mbroad · 2017-09-27T01:36:18Z

lib/network/modules/NodesHandler.js

@@ -144,7 +142,9 @@ class NodesHandler {
      throw 'Internal error: mass in defaultOptions of NodesHandler may not be zero or negative';
    }

-    util.extend(this.options, this.defaultOptions);
+    //this.options = {};
+    //util.extend(this.options, this.defaultOptions);  // Candidate for the Hall of Shame!


Why keep the dead code?

Heh. For the epitaph, actually. Major stupidity was performed here. I do this often in my own code, so that I remember not to do that thing every time I encounter the comment.

But this is shared code, and keeping it in is just vanity. Hereby gone.

mbroad · 2017-09-27T02:38:35Z

lib/network/modules/components/shared/Label.js

@@ -2,11 +2,24 @@ let util = require('../../../../util');
 let ComponentUtil = require('./ComponentUtil').default;
 let LabelSplitter = require('./LabelSplitter').default;

+/**
+ * @typedef {'bold'|'ital'|'boldital'|'mono'|'normal'} MultiName
+ *


A better name for this type might be FontStyle

Ok, although I do want to convey that this is the list of names of special fonts that can be used when multi-font is enabled.

Will you settle for MultiFontStyle?

mbroad · 2017-09-27T02:39:55Z

lib/network/modules/components/shared/Label.js

+ */
+
+/**
+ * @typedef {{color:string, size:number, face:string, mod:string, vadjust:number}} ModOptions


Suggest MultiFontOptions

mbroad · 2017-09-27T02:44:30Z

lib/network/modules/components/shared/Label.js

+    //
+    // Main body of the method
+    //
+


I think this main body comment block can be dropped

ok. The idea was to indicate where the actual function code began, after the definitions of the local function. But I just moved them away, so....

mbroad · 2017-09-27T02:45:54Z

lib/network/modules/components/shared/Label.js

+   * Member fontOptions serves as an accumulator for the current font options.
+   * As such, it needs to be completely separated from the node options.
+   *
+   * @param {Object} options


Is this the ModOptions from above, or other options?

Other options, whatever the user specified or otherwise the defaults.

mbroad · 2017-09-27T02:58:30Z

lib/network/modules/components/shared/Label.js

-    if (!this.fontOptions.multi) return;
+  propagateFonts(pile) {
+    let fontPile = [];   // sequence of font objects to consider, order important
+    let mods = ['bold', 'ital', 'boldital', 'mono'];


This looks like a good candidate for a top level constant, rather than re initialized every time propagateFonts() is called.

mbroad · 2017-09-27T03:02:35Z

lib/network/modules/components/shared/Label.js

+    *
+    * @returns {object} object with all current own basic font properties
+    */
+    let getBasicOptions = () => {


I'm seeing a lot of nested function calls and wondering if it is a micro optimization to bring them up a level, so that they're initialized with the class, rather than initialized with every call to the function that encapsulates them.

OK. My long-term planning is to isolate this pile handling code in a separate class. This is the approach that I want to use to replace the prototype chaining in options. I thought it was too early to make a class of it now.

But fair enough, I can define them as private methods for the time being. Wrt. 'micro optimization', it's an easy task to raise them to class-level, and every teeny bit helps.

mbroad · 2017-09-27T03:05:37Z

test/Label.test.js

+
+    assert.equal(modBold(0).color, 'black');    // nodes default
+    assert.equal(modBold(1).color, 'black');    // more specific bold value overrides group value
+    assert.equal(modBold(2).color, 'black');    // idem


Why are these three changed, and the others aren't? I'm missing something here...

'Idem' : Same as previous comment in this column.

Overview:

The priority of the options is: node, group, default node, global default.

black is in 'default node'.

multi-font options, if defined, take precedence. The 'normal' font options are fall-backs for the multi-font options.

Specific:

Node 0 has no 'node' or 'group' options, hence uses 'default node'.

Nodes 1 has group2, which has a default font but no explicit bold font. The bold definition in 'default node' trumps this.

Node 2 has group1, the same logic as with Node 1 applies here.

Nodes 3 and 4 have bold fonts defined in the node instance. These trump everything.

Welcome to the wonderful world of multi-font options. Precisely this kind of thing is what made fixing the issue so hard. My head still hurts.

mbroad · 2017-09-27T03:13:42Z

test/Label.test.js

+    //
+    // Same initialization as previous with a color set for the default node font
+    //
+    data.nodes = new vis.DataSet(dataNodes);  // Need to reset nodes, changed in previous


This isn't really a reset is it?

It's a new dataset, but the since dataNodes is a mutable array of mutable objects, and not a function who's return value is a new array of objects, the nodes have not been reset.

Yes, but dataNodes is not changed anywhere, so the same original values are being entered in the DataSet instance, and DataSet copies values internally[*].

Crap, let me confirm that.
Update: Confirmed with added assertions, nodes dataset is reset to initial values. You had me worried for a moment.

[*] Code check: confirmed. true for non-object options, can't tell for sure for object options, but it does work as described.

mbroad · 2017-09-27T03:23:56Z

test/Label.test.js

+
+    done();
+  });
+


Great addition of tests! Well done!

I would suggest, however, refactoring these very large tests into a number of much smaller tests, in separate test classes where required, and making use of before and after to set up the state you need.

I guess; they're kind of lengthy, that's true. I'll have a think about how to carve this up.

Update: See commit; this is as far as I wish/dare to take it. I hope this is satisfactory for you.

Excellent! The only other suggestion I'll make is to follow the mocha convention for pending, or write the entire test and use skip, as described here: https://mochajs.org

This is a nit pick, and I'm approving as-is, with or without my suggestion.

wimrijnders · 2017-09-27T05:37:24Z

@mbroad review 👍, you're being sharp.

mbroad · 2017-09-27T10:45:55Z

lib/network/modules/components/shared/Label.js

+ * List of special styles for multi-fonts
+ * @private
+ */
+const multiFontStyle = ['bold', 'ital', 'boldital', 'mono'];


normal is omitted?

Yeah. this array is the list of 'special' fonts. Style 'normal' would be the catch-all default font, i.e. not special. It should not be in this particular list.

mbroad · 2017-09-27T11:03:29Z

test/Label.test.js

+
+    done();
+  });
+


Excellent! The only other suggestion I'll make is to follow the mocha convention for pending, or write the entire test and use skip, as described here: https://mochajs.org

This is a nit pick, and I'm approving as-is, with or without my suggestion.

wimrijnders · 2017-09-27T16:02:17Z

pending and skip conventions are new to me. I suppose the intention would be to get rid of the TODO's at the top of the unit test file(s)?

OK, I think this is a minor thing. I will leave it as is, but thanks for pointing it out.

wimrijnders · 2017-09-30T04:57:37Z

@justinharrel, since your comment on widthConstraint appears to have been deleted, I take it that the wrapping is not an issue? Just checking.

This fixes an oversight in almende#3486. Unit tests added, not only for null labels, but for all weird label values I could thing of.

* Network: Add extra check on null value during label handling This fixes an oversight in #3486. Unit tests added, not only for null labels, but for all weird label values I could thing of. * Enhanced unit tests, adjusted label check

justinharrell · 2017-10-02T17:42:25Z

I accidentally commented on commit rather than PR, its still there. widthConstraint still does not seem to work.

Should I just make a new issue?

wimrijnders · 2017-10-02T18:25:51Z

What another failing case? Sure, make a new issue. If you can give a test-case that would be welcome.

* The next fix on Travis unit test failure This is the next escalation on the war against the Travis unit tests failing (which came into being by yours truly). By accident, I could recreate the unit test failure on my development machine. This led to a more directed effort to squash the bug. The insight here is that test `(window === undefined)` fails, but `(typeof window === 'undefined`)` succeeds. This undoubtedly has to do with the special status `window` has as a global object. Changes: - Added check on presence of `window` in `Canvas._requestNextFrame()`, fixed local source errors. - Added catch clause in `CanvasRendered._determinePixelRatio()` - small fix: raised timeout for the network `worldCup2014` unit test * Preliminary refactoring in utils.js * Added unit tests for extend routines, commenting and small fixes * More unit tests for extend routines * - Completed unit tests for extend routines in - Small fixes and cleanup in `util.js` - Removed `util.protoExtend()`, not used anywhere * Added unit tests for known font options * Interim save before trying out another proto chain strategy * Fixed problem in first example almende#3408 * Removed silly file that shouldn't be there * Added unit test for multi-fonts * Comment edits * Verufy unit tests, small adjustments for groups * Further work on getting unit tests to work. PARTS NEED TO BE CLEANED UP! * Further tweaks to get unit tests working * All unit tests passing * Fixes due to linting * Small edits * Removed prototype handling from font pile * Fixes during testing examples of almende#3408 * Added unit test for edge labels, small fixes * Added unit tests for shorthand string fonts; some tests still failing * All unit tests pass * Removed Label.parseOptions() * Completed shorthand font tests, code cleanup, fixed choosify for edges * Addressed review comments * Addressed review comments, cleanup

…#3511) * Network: Add extra check on null value during label handling This fixes an oversight in almende#3486. Unit tests added, not only for null labels, but for all weird label values I could thing of. * Enhanced unit tests, adjusted label check

@macleodbroad-wf

via @macleodbroad-wf, @wimrijnders (almende/vis#3228, almende/vis#3470, almende/vis#3486, almende/vis#3511, almende#3520) , almende#3518, almende#3575, almende#3565, almende#3603, almende#3646)

wimrijnders added 30 commits September 10, 2017 09:49

Preliminary refactoring in utils.js

ab58653

Merged with develop

a4d37db

Merge with 'develop'

4ef764c

Added unit tests for extend routines, commenting and small fixes

d3eb615

More unit tests for extend routines

51a517d

- Completed unit tests for extend routines in

fbdbc3d

- Small fixes and cleanup in `util.js` - Removed `util.protoExtend()`, not used anywhere

Added unit tests for known font options

990c899

Fixed conflicts

3b2532d

Interim save before trying out another proto chain strategy

4860af5

Fixed problem in first example almende#3408

a26643a

Removed silly file that shouldn't be there

2559e82

Added unit test for multi-fonts

79ca391

Comment edits

305a8a7

Verufy unit tests, small adjustments for groups

bb095d3

merged with develop, fixed conflicts

fd662ed

Merge branch 'develop' into refactorExtendRoutines

b64beaf

Merge branch 'refactorExtendRoutines' into fix_modFonts

5a9006c

Further work on getting unit tests to work. PARTS NEED TO BE CLEANED UP!

522928c

Merge branch 'develop' into fix_modFonts

c8c2f59

Further tweaks to get unit tests working

3b3af16

All unit tests passing

9cf0ec0

Fixes due to linting

3c33056

Small edits

33b081a

Merged and fixed conflicts

2ffe6e8

Removed prototype handling from font pile

6547a9c

Fixes during testing examples of almende#3408

770044c

Added unit test for edge labels, small fixes

9b35a36

Added unit tests for shorthand string fonts; some tests still failing

1224cb8

All unit tests pass

230f663

wimrijnders added 2 commits September 26, 2017 07:08

Removed Label.parseOptions()

24214c4

Completed shorthand font tests, code cleanup, fixed choosify for edges

dff9037

wimrijnders added Confirmed Bug Network labels Sep 26, 2017

yotamberk suggested changes Sep 26, 2017

View reviewed changes

wimrijnders added 2 commits September 26, 2017 18:25

Addressed review comments

f2e9521

Merge branch 'develop' into fix_modFonts

a331a18

wimrijnders mentioned this pull request Sep 26, 2017

IE performance improvements #3425 #3489

Merged

mbroad suggested changes Sep 27, 2017

View reviewed changes

Addressed review comments, cleanup

9da8d47

wimrijnders mentioned this pull request Sep 27, 2017

Not able to change edge label font properties. #3490

Open

mbroad approved these changes Sep 27, 2017

View reviewed changes

wimrijnders mentioned this pull request Sep 27, 2017

Error with adding a node to the dataset after setOptions #3025

Open

yotamberk approved these changes Sep 29, 2017

View reviewed changes

yotamberk merged commit 75e6263 into almende:develop Sep 29, 2017

wimrijnders deleted the fix_modFonts branch September 29, 2017 17:23

wimrijnders mentioned this pull request Oct 1, 2017

Network: Add extra check on null value during label handling #3511

Merged

justinharrell mentioned this pull request Oct 2, 2017

widthConstraint no longer has any effect #3517

Closed

wimrijnders mentioned this pull request Oct 4, 2017

Network: Retain constraint values in label font handling #3520

Merged

wimrijnders mentioned this pull request Oct 19, 2017

v4.21.0 : broken highlight code ? #3589

Open

		var Node = require("./components/Node").default;
		var Label = require("./components/shared/Label").default;

Network: Fix handling of multi-fonts #3486

Network: Fix handling of multi-fonts #3486

Conversation

wimrijnders commented Sep 26, 2017 • edited Loading

Changes

Further changes

Final Comment

yotamberk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders commented Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders commented Sep 27, 2017

wimrijnders commented Sep 30, 2017

justinharrell commented Oct 2, 2017

wimrijnders commented Oct 2, 2017

wimrijnders commented Sep 26, 2017 •

edited

Loading

wimrijnders Sep 26, 2017 •

edited

Loading

wimrijnders Sep 27, 2017 •

edited

Loading

wimrijnders Sep 27, 2017 •

edited

Loading

wimrijnders Sep 27, 2017 •

edited

Loading

wimrijnders commented Sep 27, 2017 •

edited

Loading