Runtime validation and error reporting #46

LandonJerre · 2017-01-20T17:22:20Z

Extension to the library to provide a way to handle internal node errors in a more visible way. Nodes now have a chance to implement a kind of self validation mechanism, and report to the scene if the operation they representing is not possible on the data they contain. (Example in the calculator sample, if the division node gets a 0 value on the divisor port, that now results in a validation error.) In case of an error, the reported error message if displayed on the invalid node.

Additionally there's a minor modification to create an easy way to iterate through all the existing nodes in the scene.

This change is

…ut data from all the nodes, makes code generation based on the graphs easier.)

russelltg

Looks good, a few minor comments

Really cool feature, can't wait to use it on my own project.

russelltg · 2017-01-20T21:44:07Z

examples/calculator/DivisionModel.hpp

@@ -45,7 +48,7 @@ class DivisionModel : public MathOperationDataModel
      default:
        break;
    }
-	return QString("");
+	  return QString("");


2 for indentation, not tab+2 spaces

russelltg · 2017-01-20T21:44:32Z

examples/calculator/DivisionModel.hpp

@@ -56,6 +59,14 @@ class DivisionModel : public MathOperationDataModel
  clone() const override
  { return std::make_unique<DivisionModel>(); }

+  virtual
+  bool
+  isValid() const { return isModelValid; }


russelltg · 2017-01-20T21:44:39Z

examples/calculator/DivisionModel.hpp

+
+  virtual
+  QString
+  errorMessage() const { return modelValidationError; }


again override

russelltg · 2017-01-20T21:46:16Z

examples/calculator/DivisionModel.hpp

@@ -65,7 +76,13 @@ class DivisionModel : public MathOperationDataModel
  }

 private:
-
+  void
+  setValidationState(bool isValid, const QString &msg)


Is this function needed? It just manipulates private member variables and doesn't do anything special....

russelltg · 2017-01-20T21:48:07Z

examples/calculator/DivisionModel.hpp

-      _result.reset();
+      setValidationState(true,
+        QString(""));
+      if (n1 && n2 && (n2->number() != 0.0))


The last condition will never be true because it's checked on L94

@russelltg

…minor styling and logic fixes in the example, based on the comments of @russelltg

LandonJerre · 2017-01-20T22:49:16Z

Thanks for the feedback, I fixed the things you mentioned (I probably forgot to double check this file before I sent the pull request). On top of this there was a minor bug in the drawing logic that now got fixed: if you check the calculator example, the result node doesn't resize correctly if its value changes from empty to some number. (If you pull the mouse over it, that forces a refresh.) I already implemented a solution that handled this kind of size change, but it only ran if the validation state changed, so I removed the condition check from it to handle all kinds of data dependent size change. Theoretically recalculation could be a teeny bit slower because of this, but I think the size of the graph becomes unmanagable for the endusers much before this slowdown could be significant. (Although a check could be implemented, so that we only force an update when the size really changed.)

russelltg · 2017-01-20T23:33:01Z

👍 LGTM

russelltg · 2017-01-24T00:41:45Z

I actually just had an idea. If isValid returns true and the error message is set, that could be a warning, displayed in yellow. That would be cool and useful (at least for me...)

LandonJerre · 2017-01-24T11:26:25Z

Good idea, but instead of checking the message for content, I've changed isValid into returning an enum value. It's clearer, and easily extensible to handle additional validation states. (Multiple different warning levels for example.) I'm in the process of refactoring the entire branch to use this consistently, so a second full review after the commit would probably be a good idea.

…; Error display colors now come from the style system; Additional bugfixes

LandonJerre · 2017-01-24T13:31:00Z

The end result visually:

The travis build magically failed, looking at the log it looks like it's not a compilation error.

LandonJerre · 2017-01-24T14:35:40Z

Review status: 2 of 19 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

examples/calculator/DivisionModel.hpp, line 51 at r1 (raw file):