From e73888a10772a0e049d752a23471ea2ae0cb4e45 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 23 Feb 2024 16:32:40 +0000 Subject: [PATCH 1/2] MDL-81039 question: Improve some comments in question_display_options --- question/engine/lib.php | 70 +++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/question/engine/lib.php b/question/engine/lib.php index 213838a878d..a181a51b90b 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -477,7 +477,7 @@ abstract class question_engine { */ class question_display_options { /**#@+ - * @var integer named constants for the values that most of the options take. + * @var int named constants for the values that most of the options take. */ const SHOW_ALL = -1; const HIDDEN = 0; @@ -485,13 +485,13 @@ class question_display_options { const EDITABLE = 2; /**#@-*/ - /**#@+ @var integer named constants for the {@link $marks} option. */ + /**#@+ @var int named constants for the {@see $marks} option. */ const MAX_ONLY = 1; const MARK_AND_MAX = 2; /**#@-*/ /** - * @var integer maximum value for the {@link $markpd} option. This is + * @var int maximum value for the {@see $markpd} option. This is * effectively set by the database structure, which uses NUMBER(12,7) columns * for question marks/fractions. */ @@ -514,33 +514,40 @@ class question_display_options { * This includes the green/red hilighting of the bits of their response, * whether the one-line summary of the current state of the question says * correct/incorrect or just answered. - * @var integer {@link question_display_options::HIDDEN} or - * {@link question_display_options::VISIBLE} + * @var int {@see question_display_options::HIDDEN} or + * {@see question_display_options::VISIBLE} */ public $correctness = self::VISIBLE; /** * The the mark and/or the maximum available mark for this question be visible? - * @var integer {@link question_display_options::HIDDEN}, - * {@link question_display_options::MAX_ONLY} or {@link question_display_options::MARK_AND_MAX} + * @var int {@see question_display_options::HIDDEN}, + * {@see question_display_options::MAX_ONLY} or {@see question_display_options::MARK_AND_MAX} */ public $marks = self::MARK_AND_MAX; - /** @var number of decimal places to use when formatting marks for output. */ + /** @var int of decimal places to use when formatting marks for output. */ public $markdp = 2; /** * Should the flag this question UI element be visible, and if so, should the - * flag state be changable? - * @var integer {@link question_display_options::HIDDEN}, - * {@link question_display_options::VISIBLE} or {@link question_display_options::EDITABLE} + * flag state be changeable? + * + * @var int {@see question_display_options::HIDDEN}, + * {@see question_display_options::VISIBLE} or {@see question_display_options::EDITABLE} */ public $flags = self::VISIBLE; /** * Should the specific feedback be visible. - * @var integer {@link question_display_options::HIDDEN} or - * {@link question_display_options::VISIBLE} + * + * Specific feedback is typically the part of the feedback that changes based on the + * answer that the student gave. For example the feedback shown if a particular choice + * has been chosen in a multi-choice question. It also includes the combined feedback + * that a lost of question types have (e.g. feedback for any correct/incorrect response.) + * + * @var int {@see question_display_options::HIDDEN} or + * {@see question_display_options::VISIBLE} */ public $feedback = self::VISIBLE; @@ -548,31 +555,35 @@ class question_display_options { * For questions with a number of sub-parts (like matching, or * multiple-choice, multiple-reponse) display the number of sub-parts that * were correct. - * @var integer {@link question_display_options::HIDDEN} or - * {@link question_display_options::VISIBLE} + * @var int {@see question_display_options::HIDDEN} or + * {@see question_display_options::VISIBLE} */ public $numpartscorrect = self::VISIBLE; /** * Should the general feedback be visible? - * @var integer {@link question_display_options::HIDDEN} or - * {@link question_display_options::VISIBLE} + * + * This is typically feedback shown to all students after the question + * is finished, irrespective of which answer they gave. + * + * @var int {@see question_display_options::HIDDEN} or + * {@see question_display_options::VISIBLE} */ public $generalfeedback = self::VISIBLE; /** - * Should the automatically generated display of what the correct answer is - * be visible? - * @var integer {@link question_display_options::HIDDEN} or - * {@link question_display_options::VISIBLE} + * Should the automatically generated display of what the correct answer be visible? + * + * @var int {@see question_display_options::HIDDEN} or + * {@see question_display_options::VISIBLE} */ public $rightanswer = self::VISIBLE; /** * Should the manually added marker's comment be visible. Should the link for * adding/editing the comment be there. - * @var integer {@link question_display_options::HIDDEN}, - * {@link question_display_options::VISIBLE}, or {@link question_display_options::EDITABLE}. + * @var int {@see question_display_options::HIDDEN}, + * {@see question_display_options::VISIBLE}, or {@see question_display_options::EDITABLE}. * Editable means that form fields are displayed inline. */ public $manualcomment = self::VISIBLE; @@ -593,8 +604,8 @@ class question_display_options { /** * Should the history of previous question states table be visible? - * @var integer {@link question_display_options::HIDDEN} or - * {@link question_display_options::VISIBLE} + * @var int {@see question_display_options::HIDDEN} or + * {@see question_display_options::VISIBLE} */ public $history = self::HIDDEN; @@ -659,9 +670,8 @@ class question_display_options { public ?bool $versioninfo = null; /** - * Set all the feedback-related fields {@link $feedback}, {@link generalfeedback}, - * {@link rightanswer} and {@link manualcomment} to - * {@link question_display_options::HIDDEN}. + * Set all the feedback-related fields, feedback, numpartscorrect, generalfeedback, + * rightanswer, manualcomment} and correctness to {@see question_display_options::HIDDEN}. */ public function hide_all_feedback() { $this->feedback = self::HIDDEN; @@ -676,10 +686,10 @@ class question_display_options { * Returns the valid choices for the number of decimal places for showing * question marks. For use in the user interface. * - * Calling code should probably use {@link question_engine::get_dp_options()} + * Calling code should probably use {@see question_engine::get_dp_options()} * rather than calling this method directly. * - * @return array suitable for passing to {@link html_writer::select()} or similar. + * @return array suitable for passing to {@see html_writer::select()} or similar. */ public static function get_dp_options() { $options = array(); From 02de9a1e3ea1ef95e1135ca988278daa2b99cf03 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 23 Feb 2024 21:17:26 +0000 Subject: [PATCH 2/2] MDL-81039 qtype_multichoice: better way to control feedback display --- question/type/multichoice/question.php | 7 +++--- question/type/multichoice/questiontype.php | 29 ++++++++++++++++++++++ question/type/multichoice/renderer.php | 8 +++--- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/question/type/multichoice/question.php b/question/type/multichoice/question.php index bf9b43c6657..fffe0be1c41 100644 --- a/question/type/multichoice/question.php +++ b/question/type/multichoice/question.php @@ -162,10 +162,9 @@ abstract class qtype_multichoice_base extends question_graded_automatically { break; } } - // Param $options->suppresschoicefeedback is a hack specific to the - // oumultiresponse question type. It would be good to refactor to - // avoid refering to it here. - return $options->feedback && empty($options->suppresschoicefeedback) && + qtype_multichoice::support_legacy_review_options_hack($options); + return $options->feedback && + $options->feedback !== qtype_multichoice::COMBINED_BUT_NOT_CHOICE_FEEDBACK && $isselected; } else if ($component == 'question' && $filearea == 'hint') { diff --git a/question/type/multichoice/questiontype.php b/question/type/multichoice/questiontype.php index 08b743444cf..b1d1b4ff0cf 100644 --- a/question/type/multichoice/questiontype.php +++ b/question/type/multichoice/questiontype.php @@ -37,6 +37,35 @@ require_once($CFG->libdir . '/questionlib.php'); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qtype_multichoice extends question_type { + /** + * @var int a special value that can be set for {@see question_display_options::$feedback}. + * + * This is not used by the core question type, but is used by some variants of this question + * types in the plugins database, including qtype_oumultiresponse and qtype_answersselect. + * + * If ->feedback is set to this value, then the renderer will display the combined feebdack, + * but not the feedback for each specific choice. + */ + const COMBINED_BUT_NOT_CHOICE_FEEDBACK = 0x100; + + /** + * Helper to catch and update if a plugin is using the old version of the COMBINED_BUT_NOT_CHOICE_FEEDBACK thing. + * + * @param question_display_options $options to be updated before being used. + */ + public static function support_legacy_review_options_hack(question_display_options $options): void { + if (empty($options->suppresschoicefeedback)) { + return; // Nothing to do. + } + + debugging('$options->suppresschoicefeedback should no longer be used. To get a similar effect, ' . + 'instead set $options->feedback = $options->feedback && qtype_multichoice::COMBINED_BUT_NOT_CHOICE_FEEDBACK.'); + if ($options->feedback) { + $options->feedback = self::COMBINED_BUT_NOT_CHOICE_FEEDBACK; + } + unset($options->suppresschoicefeedback); + } + public function get_question_options($question) { global $DB, $OUTPUT; diff --git a/question/type/multichoice/renderer.php b/question/type/multichoice/renderer.php index 287ca32162d..047af849ec4 100644 --- a/question/type/multichoice/renderer.php +++ b/question/type/multichoice/renderer.php @@ -57,7 +57,7 @@ abstract class qtype_multichoice_renderer_base extends qtype_with_combined_feedb /** * Whether a choice should be considered right, wrong or partially right. * @param question_answer $ans representing one of the choices. - * @return fload 1.0, 0.0 or something in between, respectively. + * @return float 1.0, 0.0 or something in between, respectively. */ protected abstract function is_right(question_answer $ans); @@ -118,10 +118,8 @@ abstract class qtype_multichoice_renderer_base extends qtype_with_combined_feedb 'data-region' => 'answer-label', ]); - // Param $options->suppresschoicefeedback is a hack specific to the - // oumultiresponse question type. It would be good to refactor to - // avoid refering to it here. - if ($options->feedback && empty($options->suppresschoicefeedback) && + qtype_multichoice::support_legacy_review_options_hack($options); + if ($options->feedback && $options->feedback !== qtype_multichoice::COMBINED_BUT_NOT_CHOICE_FEEDBACK && $isselected && trim($ans->feedback)) { $feedback[] = html_writer::tag('div', $question->make_html_inline($question->format_text(