Document test failure reasons

[IanMayo]

We wish to let users learn more about the applicable elements for operations.

This will show them why the current selection isn't applicable to a particular operation. It will also teach them what kind of elements that an operation will apply to.

In my original spec, I did hope to include the text in the applicable tests:
debrief/limpet#65 (comment)

But, I appreciate that if they aren't available in the parent schema, then we can't use them.

It would be great, however, if we could add these string attributes to the applicable tests - then our logic code can display text to a user regarding the specific test that failed.

So, these appear to be the options (in descending preference order, IMHO):

1. Is it worth considering extending the Core Expressions Framework to allow these optional parameters?
2. Should we come up with a library of error messages, so that when an expression fails, we can lookup the human-readable explanation of why that test failed. But, the library text may not always be correct for the specific context of the test.
3. Alternatively, we can provide an additional "Failed" documentation element that explains why selection matching may have failed. But, it is a maintenance burden to keep this aligned with the actual tests.

Would you mind considering/investigating these?

[dinkoivanov]

Regarding option 1:
It's not possible to "extend" an extension point - http://stackoverflow.com/questions/28450349/is-it-possible-to-extend-or-re-use-an-eclipse-extension-point. What we can do to "extend" it is we can copy the exsd files definitions from org.eclipse.core.expressions (definitions.exsd and expressionLanguage.exsd) and modify them to suit our needs (add 'fail' attribute). We'll need however to also modify the code in the o.e.c.expressions plugin that's responsible for parsing and evaluating the expression, so that we provide the right 'fail' message during expression evaluation.

That's a serious bit of work, compared to option 2 - building a library of messages. If I understand you correctly for example for "sampleModel.allNumeric" expression definition in the sampleModel/plugin.xml we shall define a human readable display message, such as "Selection should only contain numeric values"? You mentioned that

the library text may not always be correct for the specific context of the test

Could you explain or give an example? I imagine that ideally with a well structured reusable expression definitions, context won't matter that much.

[IanMayo]

Hmm, something like "A frequency dataset is required".

Or, whilst the test may be an "AND" for "has location", "has course", "has speed", our user-oriented message for the AND failure could be "Requires a composite track".

[IanMayo]

Hmm, I've had a play.

Whilst I can change the schema to include documentation elements, and the XML file still gets read in, the content is ignored by Eclipse (which I'm pretty sure is what you said).

This may offer an alternative strategy. What if we also read the XML code into a DOM object. Then, use XPath (or similar) to navigate to the comment/supporting content for a particular entry.

Is this worth further consideration?

[dinkoivanov]

I think with this we'll avoid trying to extend the EXSD schema. I had some doubts that the Eclipse Plugin editor might drop elements not contained in the schema when you do save the plugin.xml, but it's not the case - I also tested with custom "fail" documentation elements/attributes, and those are preserved.
The expression evaluation however does not provide information about the expression node that evaluated to false. We would still need to somehow extend/modify the framework to handle that.

Regarding the "Requires a composite track" user message for the AND(hasLocation, hasCourse, hasSpeed) expression. Could this expression have different user message in a different context? If not, we can follow approach 2 for it. This means it will be contained in a separate definition like "requiresCompositeTrack", and then we could have a property file containing the message
requiresCompositeTrack=Requires a composite track

[IanMayo]

On 28th Feb Dinko said:

I can do some more investigation on this option: "...What if we also read the
XML code into a DOM object. Then, use XPath (or similar) to navigate to the
comment/supporting content for a particular entry..." Reading and navigating the 
XML is easy. The challenge would be to determine which expression node failed
during evaluation. We can do that by wrapping the expression syntax tree in our
wrapper objects that delegate evaluation, but store local results (test failure), so 
that after the evaluation, we can query our nodes and see what failed.

That strategy sounds just like what I was hoping for. Having the user feedback in there with the actual content will make things much easier to maintain & understand.

@dinkoivanov - I'd appreciate you exploring this further.

[dinkoivanov]

How will those failure messages be presented to the user? Imagine the following applicability expression:

<and failMessage="Needs multiple composite tracks">
    <and failMessage="Requires a composite track">
        <hasLocation/>
        <hasCourse/>
        <hasSpeed/>
    </and>
    <and failMessage="Requires multiple elements">
        <sizeBiggerThanZero/>
    </and>
</and>

In case one of the inner ANDs fail, the outer will fail as well - what should be the message then? Shall we show only the outermost message? Showing all the fail messages will make the task harder, since we'll need to also convey the expression data somehow - whether it's an AND/OR node, etc.

[IanMayo]

Ok, we'll imagine that the track is missing a "course" attribute.

Somewhere, we'd declare that failing "hasCourse" is presented as:
"Needs a course field (an angle, in degrees or radians)".

This could be declared as a "failMessage" for , or somewhere centrally.

So the error message for a track that omits both course and speed would be:

Selection invalid, reasons as follows:

• Needs multiple composite tracks failed because of:
• Requires a composite track failed because of:
• Needs a course field (an angle, in degrees or radians)
• Needs a speed field (a speed, in m/sec or knots)

[dinkoivanov]

I did some exploratory work in a separate branch here:
https://github.com/debrief/limpet_ops/tree/issue3__document_test_failure_reason
To demonstrate this, launch the Limpet POC View, select some elements and right click - in the IDE console you'll see some logging - not applicable operations would dump some messages (failure messages if specified).
To see how failure messages are specified, open the plugin.xml of the sampleModel and check the XML. I've provided fail messages for some of the conditions.
There are now few Custom* classes in the samplemodel package that carry out loading the fail message from the XML and noting the failure during expression node evaluation. It's not quite clean, since the expression framework was not quite designed to be extensible, but I'd say it's not that bad (no reflection was needed for example).
In essence, we provide a CustomElementHandler that will wrap expression nodes in our CustomExpression (wrapper), which will take care of the fail message during evaluation.

[dinkoivanov]

Selection invalid, reasons as follows:

Needs multiple composite tracks failed because of:
Requires a composite track failed because of:
Needs a course field (an angle, in degrees or radians)
Needs a speed field (a speed, in m/sec or knots)

That's clear, however the "because of" relationship might not be clear. For example if sizeBiggerThanZero failed too, there'll be a "Requires multiple elements" message on bottom and it won't be clear whether "Needs multiple..." or "Requires a composite ..." failed due to sizeBiggerThanZero.
Perhaps we can use indentation to convey this?

[IanMayo]

Yes, maybe something like this (I've used characters to force the indent).

Selection invalid, reasons as follows:

• Needs multiple composite tracks failed because of:
• == Requires a composite track failed because of:
• == == Needs a course field (an angle, in degrees or radians)
• == == Needs a speed field (a speed, in m/sec or knots)
• == Requires multiple elements

[dinkoivanov]

The presentation is checked in. Failure reason related classes are moved to a separate package - samplemodel.failurelog.
The functionality can be tested with the "Concatenate Strings" operation defined in the sampleOperation1 plugin. The operation is only applicable if all selected elements are SampleModel instance and non-numeric.
Here is what is logged in the console, if a String and a Number are selected:

[Concatenate Strings] failed because of:
  [All should be instanceof sample model and not be numeric] failed because of:
    [Should not be numeric]

And here is the result when a String and a Date are selected:

[Concatenate Strings] failed because of:
  [All should be instanceof sample model and not be numeric] failed because of:
    [Should be instanceof sample model]

In essence there is an ExpressionLogger class, that is invoked during expression evaluation (In the CustomExpression class). It creates a hierarchical structure of LoggerNode instances, to represent the failure messages at specific parts of the expression.

[IanMayo]

Yes, I saw the screenshot of your code earlier on, it looks great (the indent-level code is clever).

I may not get chance to test this until later this week.