Individual answers Feedback not shown in Cloze questions

This was working for me at the point when I thought I had finished the the Cloze questiontype, so I don't know what might have gone wrong.

The feedbacktext has been forgotten

    protected function feedback_popup(question_graded_automatically $subq,
            $fraction, $feedbacktext, $rightanswer, question_display_options $options) {

        if (!$options->feedback) {
            return '';
        }

        $feedback = array();
        if ($options->correctness) {
            if (is_null($fraction)) {
                $state = question_state::$gaveup;
            } else {
                $state = question_state::graded_state_for_fraction($fraction);
            }
            $feedback[] = $state->default_string(true);
        if ($options->rightanswer) {
            $feedback[] = get_string('correctansweris', 'qtype_shortanswer', $rightanswer);
        }

        $subfraction = '';
        if ($options->marks >= question_display_options::MARK_AND_MAX && $subq->maxmark > 0) {
            $a = new stdClass();
            $a->mark = format_float($fraction * $subq->maxmark, $options->markdp);
            $a->max =  format_float($subq->maxmark, $options->markdp);
            $feedback[] = get_string('markoutofmax', 'question', $a);
        }

        return html_writer::tag('span', implode('<br />', $feedback),
                array('class' => 'feedbackspan accesshide'));

Thanks, Pierre.
If we insert the missing feedbacktext here, it works fine:

$feedback = array();
        $feedback[] = $feedbacktext;

Pierre, Tim, any progress on this bug?

Any update on this one-month old bug with a very easy fix?
Actually, it would be better to check for existence of a feedback text, so I suggest this fix:

$feedback = array();
if ($feedbacktext) {
    $feedback[] = $feedbacktext;
}

Tim return next week from his holidays.
However if you can prepare a commit and tests results (even for such a little code change), this should help.

Now that Tim is back, I hope he will find the time to commit this very easy fix. It is really needed for the Cloze question type to display feedback.

A proposal can be found at
https://github.com/ppichet/moodle/commit/eff62bb3ecec297521db262ef60dfa88a96727b7

Seems OK tested on the three variants.
However the feedback display although similar to new engine regular question types does not clearly identify the feedback part as in preceeding versions which is apparently the new engine standards.

If the $feedbacktext != '' test is not done, there is an empty line when the feedback is null.

Thank you Pierre, Actually this question of displaying the specific feedback in the Cloze sub-questions (in Moodle 2.1) is more tricky than I thought at first. I am conducting exhaustive tests and will report in this discussion as soon as possible.

Note: tested in teacher Preview question window. With Pierre's patch applied:

if ($feedbacktext != ('')) {
    $feedback[] = $feedbacktext;
}

1.- displaying individual Feedback

case : sub-question answer does not contain a feedback message
settings: Behaviour: any;
Whether correct: Not shown; Marks: Not shown; Specific feedback: Shown; Right answer: Not shown;
bug: hovering shows small empty tooltip rather than nothing at all.
suggested fix: check that the feedback() array is not empty before returning function data:
line 150 add

if (empty($feedback)) {
            return "";
        }
        return html_writer::tag('span', implode('<br />', $feedback),
                array('class' => 'feedbackspan accesshide'));

2.- display bug in MCQ sub-questions only

a. Behaviour: Adaptive

Whether correct: shown;
action: Check button
tootlip feedback is displayed when hovering over dropdown list input field: OK (i.e. hovering over the student's actual answer text, as in all other question types)

b. Behaviour: any

Whether correct: shown;
action: Submit and finish button
tootlip feedback is NOT displayed when hovering over dropdown list input field but hovering over the green (correct) or red (incorrect) icon.This behaviour is disconcerting.

c. Behaviour: any

action: Submit and finish button
Whether correct: Not shown; Marks: Not shown; Specific feedback: Shown; Right answer: Not shown;
action: Submit and finish button
tootlip feedback can NOT be displayed at all for the MCQ sub-questions, since the green (correct) or red (incorrect) icons are not being displayed in this scenario.

I do not know well enough the code of the Cloze question in M 2.1 to suggest a fix for this display bug.

I'll try to look at the fix this week.

Thanks Joseph for the more complete tests that I have done.
Should find time to work on it tonight.

When the question is finished the select element is disabled

<select id="q10:1_sub2_answer" disabled="disabled" class="select incorrect" name="q10:1_sub2_answer">

The hovering is not working in this case.

P.S. the () are not necessary in $feedbacktext != (''). they where just a leftover from a previous code using is_null() . There are some inconvenients to work to late

As Joseph noted, the test for empty line 150 should be added.

@ Pierre
1.- I've just noticed that this problem is navigator-dependent. Firefox and Chrome will refuse to display anything pertaining to an input field which is disabled, whereas "good old" Internet Explorer will accept to do it!

2.- With or without the parenteses, this condition alone does not solve the problem, Only the empty array test I suggest will work.

Joseph

see http://stackoverflow.com/questions/2034820/firefox-does-not-show-tooltips-on-disabled-input-fields

"Only the empty array test I suggest will work."
I agree with you that your final test (line 150) is necessary if nothing has been added to $feedback at this point.
The first test (line 125) is necessary to not have an empty line if there is no feedback defined.

Should we or not disabled the select element in Cloze, the corresponding shortanswer and numerical are not disabled as far as I know on Firefox.

The text input elements (numerical or shortanswer) are "readonly" when the question is finished.
The tool-tip remains useable.

Select element in Cloze must stay in-line with the text which limits the possibilities.

If we put before and after the select element a small transparent image, this allows the display of the tooltip message when the checkmark is not shown.

More simply just put a space before and after the select element can do the job.

        $output = '';
        $output .= html_writer::start_tag('label', array('class' => 'subq'));
        $output .=' ';
        $output .= $select;
        $output .= $feedbackimg.' ';
        $output .= $feedbackpopup;

Although there will be a space between the text it will stay inline.
This could change a little the display when the select element is used to complete a word or specify the punctuation.
So perhaps we should not add it before the select.
The space after could be used only if $feedbackimg is null.
In this case a transparent feedback image can be used.

Pierre's fix is almost right, but it misses out a check for $options->feedback. I have done a revised fix.

By the way, the 'disabled select menus in Firefox don't show any feedback' problem is a separate bug. I think someone may already have reported it. Anyway, please can you move discussion of that issue there. Thanks.

Tim "Pierre's fix is almost right, but it misses out a check for $options->feedback. I have done a revised fix."
1.- But that test is already done line 122:

if (!$options->feedback) {
            return '';
        }

2.- My remark 13/Sep/11 7:30 PM is still valid:
.- displaying individual Feedback

case : sub-question answer does not contain a feedback message
settings: Behaviour: any;
Whether correct: Not shown; Marks: Not shown; Specific feedback: Shown; Right answer: Not shown;
bug: hovering shows small empty tooltip rather than nothing at all.
and my suggested fix should be added to avoid display of small empty tooltip.
add line 150

if (empty($feedback)) {
            return "";
        }
        return html_writer::tag('span', implode('<br />', $feedback),
                array('class' => 'feedbackspan accesshide'));

Joseph

Doh! Thanks Joseph. I was clearly too tired and careless to be reviewing code. I'll fix it tomorrow.

I reworked my proposal YESTERDAY but did not add the new references here.

@@ -123,7 +123,8 @@ abstract class qtype_multianswer_subq_renderer_base extends qtype_renderer {
123	123	
         }
124	124	
 
125	125	
         $feedback = array();
126	 	
-        if ($feedbacktext != ('')) {
 	126	
+
 	127	
+        if ($feedbacktext != '') {
127	128	
             $feedback[] = $feedbacktext;
128	129	
         }
129	130	
 
...	...	
@@ -147,7 +148,10 @@ abstract class qtype_multianswer_subq_renderer_base extends qtype_renderer {
147	148	
             $a->max =  format_float($subq->maxmark, $options->markdp);
148	149	
             $feedback[] = get_string('markoutofmax', 'question', $a);
149	150	
         }
150	 	
-
 	151	
+        
 	152	
+        if (empty($feedback)) {
 	153	
+            return "";
 	154	
+        }
151	155	
         return html_writer::tag('span', implode('<br />', $feedback),
152	156	
                 array('class' => 'feedbackspan accesshide'));
153	157	
     }

So we all agree for almost the same thing
with the exception that there is no necessary test for feedback line 135

if ($options->feedback && $feedbacktext != ('')) {

as this is already tested line 121
We need however to test $feedback line 150 as $feedback could be null and if nothing else is displayed then there is a small empty square that will remain.

Should we put the feedback before or after the correctness is not clear for me given that the $feedbackimg is already displayed before the user move his mouse to see the details.

P.S.
the reference is

https://github.com/ppichet/moodle/blob/9e6fbeb601936002854fa6e9b784ea448040d04e/question/type/multianswer/renderer.php

As this was my first merge, perhaps things are not so clear ...

OK. I revised my commit. Thank you both for your peer-review.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Thanks everyone this has been integrated now

test passed.
Thanks

And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

Ciao