Network: Retain constraint values in label font handling #3520

wimrijnders · 2017-10-04T07:17:26Z

Due to changed logic in the label font handling, the option values for widthConstraint and heightConstraint were overwritten.

The fix is in effect a reversal of two code lines: parsing constraint options should come after parsing (multi)font options.

Further changes:

Additional 1-liner fix: constraint values were not copied for edge-instance specific options.
Small refactoring of Label#constrain() in order to separate concerns
Added unit test for regression testing of this issue.

All of which entices me to make the curious observation that two changed lines of source code has led to a +-150 line regression test.

Attentive observers may note that the automatic line breaks are now positioned differently, e.g. in the widthHeight example.

This is due to a fix in the line width calculation in #3486: current stable does not take the spaces into account when calculating width.

Fixes almende#3517. Due to changed logic in the label font handling, the option values for `widthConstraint` and `heightConstraint` were overwritten. The fix is in effect a reversal of two code lines: parsing constraint options should come *after* parsing (multi)font options. Further changes: - Additional 1-liner fix: constraint values were not copied for edge-instance specific options. - Small refactoriing of `Label#constrain()` in order to separate concerns - Added unit test for regression testing of this issue. This leads to the curious observation that, while the actual change is two lines of source code, this resulted in a +-150 line regression test.

mbroad

Not necessarily as part of the ticket, but it seems like we have a lot of work to do around documenting and testing the order and application of options.

mbroad · 2017-10-04T11:45:18Z

test/Label.test.js

+        assert.equal(fontOptions.minWdt, -1);
+        assert.equal(fontOptions.maxWdt, expectedMaxWidthDefault);
+      } else {
+        let constraint = item.widthConstraint;


Shouldn't the test run be deterministic, so ifs and elses can be avoided?

Perhaps that's what you mean: In the unit test, since you know exactly what value you are expecting, so can do a direct test on input and output. So, for e.g. node 100, following should suffice:

assert.equal(fontOptions.minWdt, -1); assert.equal(fontOptions.maxWdt, 200); assert.equal(fontOptions.minHgtt, -1); assert.equal(fontOptions.valign, 'middle');

So trading in the conditionals for repetitive but flat code. Is that the clue? I'm agreeing to that. It's not that I don't know how to do that, e.g. it('respects the font option precedence').

In fact, I'm not even going to wait for your confirmation that that is what you mean to change to this. I've been following the source code logic too much here (notably Label#constrain()).

wimrijnders · 2017-10-05T02:28:45Z

Not necessarily as part of the ticket, but it seems like we have a lot of work to do around documenting and testing the order and application of options.

Oh man. Surely you couldn't have missed me lamenting about the state of network option handling in the lobby. The direction I'd like to go, is to simplify option handling as much as possible so that the actual intent becomes obvious. 'We're not there yet' is an euphemism in this context.

wimrijnders · 2017-10-05T03:44:50Z

The consequence of making it more linear is that it's now obvious that there is too much checking of the same thing.

I'm going let the idea of using the example go and make an 'enhanced subset' of it, so to speak, which also addresses the TODO's in the header comment.

Also, I notice that internal font option fields constrainWidth and constrainHeight are not tested, will add these as well.

Update: Well, crap. It appears that constrainWidth and constrainHeight are never set. @mbroad, would you mind verifying this?

This implies that Label#adjustSizes() is useless. Do we need it in working condition, if at all???

I'm going to delve back in previous commits to see if this is a recent thing.
Update: Nope. Last commit before I started mangling Label.js doesn't set them either (Jan 29 2017). For once, it's not my fault.

Update 2: widthConstraint: false doesn't work as advertized. From the docs:

If false, no widthConstraint is applied

This is not true, the boolean value is plain ignored. Same goes for heightConstraint: false. Shall we just remove it from the docs? Removing a constraint can be achieved by setting the value to null, e.g. widthConstraint: null.
Update: This also means that boolean can/should be removed from the option definitions. This would be a breaking API change, though the least malevolent I can imagine. Users in their right mind wouldn't be using the value anyway, and worst of all, it does nothing.

mbroad · 2017-10-08T04:00:28Z

This also means that boolean can/should be removed from the option definitions. This would be a breaking API change, though the least malevolent I can imagine. Users in their right mind wouldn't be using the value anyway, and worst of all, it does nothing.

Should it really be removed, or just actually set?

mbroad · 2017-10-08T03:56:50Z

test/Label.test.js

+    // checking incorrect settings or for future code changes.
+    //
+    // There is a lot of repetitiveness here. Perhaps using a direct copy of the
+    // example should be let go.


I agree wholeheartedly with this comment.

Test set ups should be made up of the minimal required code to satisfy preconditions for your tests.

OK. The thing is, I've already let go of the example and changed the test accordingly. I'll remove this comment and add some extra explanation.

mbroad · 2017-10-08T04:19:49Z

Well, crap. It appears that constrainWidth and constrainHeight are never set. @mbroad, would you mind verifying this?

Verified. They are both set to false, but nothing else modifies them.

wimrijnders · 2017-10-08T04:36:02Z

Verified. They are both set to false, but nothing else modifies them.

Thanks. I'll consider them artifacts and remove them and Label#adjustSize(), which is the only place were they are accessed and which thus does nothing.

Edit: if options widthConstraint and heightConstraint are retained, then these can not be removed. But I will rename them.

wimrijnders · 2017-10-08T04:47:30Z

Should it really be removed, or just actually set?

Had a think about this: I can image a scenario where the widthConstraint/heightConstraint gets overridden in the more specific options (e.g. defined in the global options, overridden in group or node instance).

So setting to false actually has potential use. So it needs to be handled as well. What to do with setting to true, however? That is useless......

Anyway, I'll add the handling in.

wimrijnders · 2017-10-08T07:30:46Z

Wrt. fully enabling width/heightConstraint: I'm digging myself into a hole here.

The consequence is that suddenly the margins are used again - option margin has not been active in a long time. So the changes are propagating.

Rather than fix this as well, I'll leave the margins as is, make a note and create an issue for it (#3536).

wimrijnders · 2017-10-09T02:54:52Z

Oh, merged already ☹️. I was hoping to get some changes in still. Never mind, I'll add them in #3536.

* Network: Retain constraint values in label font handling Fixes almende#3517. Due to changed logic in the label font handling, the option values for `widthConstraint` and `heightConstraint` were overwritten. The fix is in effect a reversal of two code lines: parsing constraint options should come *after* parsing (multi)font options. Further changes: - Additional 1-liner fix: constraint values were not copied for edge-instance specific options. - Small refactoriing of `Label#constrain()` in order to separate concerns - Added unit test for regression testing of this issue. This leads to the curious observation that, while the actual change is two lines of source code, this resulted in a +-150 line regression test. * Made unit test more linear, removed tabs * Made 'enhanced subset' of unit test * Removed TODO from comment * Culled redundant nodes from unit test

wimrijnders added Confirmed Bug Network labels Oct 4, 2017

Fixed conflicts

352048f

mbroad reviewed Oct 5, 2017

View reviewed changes

Made unit test more linear, removed tabs

481a556

wimrijnders added 3 commits October 5, 2017 07:18

Made 'enhanced subset' of unit test

f246978

Removed TODO from comment

175e741

Culled redundant nodes from unit test

4cd7286

mbroad reviewed Oct 8, 2017

View reviewed changes

mbroad approved these changes Oct 8, 2017

View reviewed changes

wimrijnders mentioned this pull request Oct 8, 2017

Network: margin options are not used #3536

Open

2 tasks

yotamberk approved these changes Oct 8, 2017

View reviewed changes

yotamberk merged commit 4a25c43 into almende:develop Oct 8, 2017

wimrijnders deleted the issue3517 branch October 9, 2017 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network: Retain constraint values in label font handling #3520

Network: Retain constraint values in label font handling #3520

wimrijnders commented Oct 4, 2017 •

edited

Loading

mbroad left a comment

mbroad Oct 4, 2017

wimrijnders Oct 5, 2017 •

edited

Loading

wimrijnders commented Oct 5, 2017

wimrijnders commented Oct 5, 2017 •

edited

Loading

mbroad commented Oct 8, 2017

mbroad Oct 8, 2017

wimrijnders Oct 8, 2017

mbroad commented Oct 8, 2017

wimrijnders commented Oct 8, 2017 •

edited

Loading

wimrijnders commented Oct 8, 2017 •

edited

Loading

wimrijnders commented Oct 8, 2017 •

edited

Loading

wimrijnders commented Oct 9, 2017

Network: Retain constraint values in label font handling #3520

Network: Retain constraint values in label font handling #3520

Conversation

wimrijnders commented Oct 4, 2017 • edited Loading

mbroad left a comment

Choose a reason for hiding this comment

mbroad Oct 4, 2017

Choose a reason for hiding this comment

wimrijnders Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

wimrijnders commented Oct 5, 2017

wimrijnders commented Oct 5, 2017 • edited Loading

mbroad commented Oct 8, 2017

mbroad Oct 8, 2017

Choose a reason for hiding this comment

wimrijnders Oct 8, 2017

Choose a reason for hiding this comment

mbroad commented Oct 8, 2017

wimrijnders commented Oct 8, 2017 • edited Loading

wimrijnders commented Oct 8, 2017 • edited Loading

wimrijnders commented Oct 8, 2017 • edited Loading

wimrijnders commented Oct 9, 2017

wimrijnders commented Oct 4, 2017 •

edited

Loading

wimrijnders Oct 5, 2017 •

edited

Loading

wimrijnders commented Oct 5, 2017 •

edited

Loading

wimrijnders commented Oct 8, 2017 •

edited

Loading

wimrijnders commented Oct 8, 2017 •

edited

Loading

wimrijnders commented Oct 8, 2017 •

edited

Loading