Moodle
  1. Moodle
  2. MDL-17064

Allow user to enter the required quantity of repeated elements in one page reload

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.9.2
    • Fix Version/s: None
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Rank:
      4775

      Description

      Button 'Add 3 blank choices' isn't terribly helpful when you need a ten or more choices. Using field to enter the required number of choices we can ajust their number in one page reload.

      1. numberofblanks.beta.patch
        10 kB
        Oleg Sychev
      2. numberofblanks.beta2.patch
        13 kB
        Oleg Sychev
      3. numberofblanks.beta2.patch
        10 kB
        Oleg Sychev

        Issue Links

          Activity

          Hide
          Oleg Sychev added a comment -

          Analyzing the files I found that code, that need to be modified for this is located not in question editing class, but in moodleform class (repeat_elements).

          Do you want me to override this function in question_edit_form or write new function there? Other parts of Moodle potentially can benefit from this too.

          Show
          Oleg Sychev added a comment - Analyzing the files I found that code, that need to be modified for this is located not in question editing class, but in moodleform class (repeat_elements). Do you want me to override this function in question_edit_form or write new function there? Other parts of Moodle potentially can benefit from this too.
          Hide
          Oleg Sychev added a comment -

          want you opinion on some aspects before I start working on actual patch:

          1) I try to keep this task separated from another (htmleditors) as hard as I can, but actually had to believe that their user (but not necessary programmer - see $elementobjsafter in 2) interface must be united: a fieldset 'Editing options' with

          • a) text field for number of choices/answers (calculated, match, missingtype,multichoice, numerical, shortanswer and, probably, truefalse)
          • b) check box for using htmleditor on choices (multichoice and match)
          • c) check box for using htmleditor on feedback to individual choices/answers (calculated, missingtype,multichoice, numerical, shortanswer and, probably, truefalse)
          • d) check box for htmleditor on feedback to any correct/partial correct/incorrect answers (multichoice - probably such comments must also be added to match question type, because it support only general feedback right now)
          • e) button for applying of changes in options above.
            I think that one 'apply' button is better solution because it'll reduce the number of page reloads needed to customise the question editing page to you liking (and with htmleditors reload is slow). If formslib allows this, enter in a) field must probably activate 'apply' button for better usability.
            Also, I think that new interface must be place before repeated elements, not after them as in 'Add xxx' button case.

          2) To implement this subtask we need a replacement for repeat_elements function from formslib.php. I suggest following parameters (my changes to repeat_elements comments marked with !!)
          repeat_elements_ex

          • @param array $elementobjs Array of elements or groups of elements that are to be repeated
          • @param integer $repeats no of times to repeat elements initially
          • @param array $options Array of options to apply to elements. Array keys are element names.
          • This is an array of arrays. The second sets of keys are the option types
          • for the elements :
          • 'default' - default value is value
          • 'type' - PARAM_* constant is value
          • 'helpbutton' - helpbutton params array is value
          • 'disabledif' - last three moodleform::disabledIf()
          • params are value as an array
          • !!EDITED @param string $repeatname name for text element storing no of repeats
          • !! ADDED @param string $repeatlabel label to field with number of repeated elements
          • @param string $addfieldsname !!EDITED - name for button to apply options
          • !! OBSOLETE - must be deleted @param int $addfieldsno how many fields to add at a time
          • @param string $addstring !!EDITED user visible name of apply button, defaults 'Apply'
          • @param boolean $addbuttoninside if true don't call closeHeaderBefore($addfieldsname). Default false.
          • !! ADDED @param array $elementobjsafter - array of elements to insert after field with number of repeats (for MDL-17062)
          • !! ADDED @param array $optionsafter - options to elements in previous parameter
          • @return int no of repeats of element in this page

          the questions are:
          a) should we allow the caller to insert elements before as well as after field with number of repeats? This will increase universality at the cost of some additional code.
          b) the caller needs to supply a function to process options in $elementsafter on applying - should it be a member of the class with fixed name, or should the caller supply a name of the function as parameter?
          c) repeat_elements is located in moodleform class, where should I place repeat_elements_ex - in moodleform or in question_edit_form?
          d) to reduce the patch size I can edit repeat_elements instead of writing a new function (they'll have many common code) - but this will break interface of a basic system class. Thought scanning moodle code I found that this function used only in questions editing and in mod_form.php for quiz and choice modules, so it won't be too hard to me to upgrade them all if you want it.

          3) There is a good deal of code duplication related to the repeated elements in questions. Should I refactor it to reduce the amount of duplicated code?

          I would like to hear you opinion about this before writing a patch to avoid unnecessary rewriting.

          Show
          Oleg Sychev added a comment - want you opinion on some aspects before I start working on actual patch: 1) I try to keep this task separated from another (htmleditors) as hard as I can, but actually had to believe that their user (but not necessary programmer - see $elementobjsafter in 2) interface must be united: a fieldset 'Editing options' with a) text field for number of choices/answers (calculated, match, missingtype,multichoice, numerical, shortanswer and, probably, truefalse) b) check box for using htmleditor on choices (multichoice and match) c) check box for using htmleditor on feedback to individual choices/answers (calculated, missingtype,multichoice, numerical, shortanswer and, probably, truefalse) d) check box for htmleditor on feedback to any correct/partial correct/incorrect answers (multichoice - probably such comments must also be added to match question type, because it support only general feedback right now) e) button for applying of changes in options above. I think that one 'apply' button is better solution because it'll reduce the number of page reloads needed to customise the question editing page to you liking (and with htmleditors reload is slow). If formslib allows this, enter in a) field must probably activate 'apply' button for better usability. Also, I think that new interface must be place before repeated elements, not after them as in 'Add xxx' button case. 2) To implement this subtask we need a replacement for repeat_elements function from formslib.php. I suggest following parameters (my changes to repeat_elements comments marked with !!) repeat_elements_ex @param array $elementobjs Array of elements or groups of elements that are to be repeated @param integer $repeats no of times to repeat elements initially @param array $options Array of options to apply to elements. Array keys are element names. This is an array of arrays. The second sets of keys are the option types for the elements : 'default' - default value is value 'type' - PARAM_* constant is value 'helpbutton' - helpbutton params array is value 'disabledif' - last three moodleform::disabledIf() params are value as an array !!EDITED @param string $repeatname name for text element storing no of repeats !! ADDED @param string $repeatlabel label to field with number of repeated elements @param string $addfieldsname !!EDITED - name for button to apply options !! OBSOLETE - must be deleted @param int $addfieldsno how many fields to add at a time @param string $addstring !!EDITED user visible name of apply button, defaults 'Apply' @param boolean $addbuttoninside if true don't call closeHeaderBefore($addfieldsname). Default false. !! ADDED @param array $elementobjsafter - array of elements to insert after field with number of repeats (for MDL-17062 ) !! ADDED @param array $optionsafter - options to elements in previous parameter @return int no of repeats of element in this page the questions are: a) should we allow the caller to insert elements before as well as after field with number of repeats? This will increase universality at the cost of some additional code. b) the caller needs to supply a function to process options in $elementsafter on applying - should it be a member of the class with fixed name, or should the caller supply a name of the function as parameter? c) repeat_elements is located in moodleform class, where should I place repeat_elements_ex - in moodleform or in question_edit_form? d) to reduce the patch size I can edit repeat_elements instead of writing a new function (they'll have many common code) - but this will break interface of a basic system class. Thought scanning moodle code I found that this function used only in questions editing and in mod_form.php for quiz and choice modules, so it won't be too hard to me to upgrade them all if you want it. 3) There is a good deal of code duplication related to the repeated elements in questions. Should I refactor it to reduce the amount of duplicated code? I would like to hear you opinion about this before writing a patch to avoid unnecessary rewriting.
          Hide
          Tim Hunt added a comment -

          I disagree I think we should aim for a minimal change to the repeat elements feature. I think we should change

          [Add blanks for 3 more answers]

          to

          [Add] blanks for [3 v] more answers

          That is, add is a button, and 3 is a drop-down menu of numbers, defaulting to whatever number was used before.

          Or, perhaps if you think it would be better to use the number, rather than the increment, then do

          Make space for [5 v] answers [Update]

          In this second case, you could use get/set_user_preference to remember the user's preference for each form.

          By the way, only 10 forms in Moodle use repeats, so I think we can get away with changing this.

          I don't think we should combine the different settings. The right thing to do is to use user_preferences, so that people don't have to change these options very often.

          Show
          Tim Hunt added a comment - I disagree I think we should aim for a minimal change to the repeat elements feature. I think we should change [Add blanks for 3 more answers] to [Add] blanks for [3 v] more answers That is, add is a button, and 3 is a drop-down menu of numbers, defaulting to whatever number was used before. Or, perhaps if you think it would be better to use the number, rather than the increment, then do Make space for [5 v] answers [Update] In this second case, you could use get/set_user_preference to remember the user's preference for each form. By the way, only 10 forms in Moodle use repeats, so I think we can get away with changing this. I don't think we should combine the different settings. The right thing to do is to use user_preferences, so that people don't have to change these options very often.
          Hide
          Jamie Pratt added a comment -

          I think that Tim's suggestion for how to change repeat_elements sounds good. Either option sounds good, although if you allow a user to reduce the number of blanks instead of just to be able to add blanks then you will need to make sure that data that is dropped from the form is also appropriately dealt with in the processing of data from existing forms after submission.

          Show
          Jamie Pratt added a comment - I think that Tim's suggestion for how to change repeat_elements sounds good. Either option sounds good, although if you allow a user to reduce the number of blanks instead of just to be able to add blanks then you will need to make sure that data that is dropped from the form is also appropriately dealt with in the processing of data from existing forms after submission.
          Hide
          Pierre Pichet added a comment -

          "The right thing to do is to use user_preferences, so that people don't have to change these options very often"
          I agree with this as long as the user preferences are specific for the form.
          I think that a drop-down menu is better from the view point that the values are already filtered however for flexibility and accessibility to diseabled persons an input text field is better.

          Show
          Pierre Pichet added a comment - "The right thing to do is to use user_preferences, so that people don't have to change these options very often" I agree with this as long as the user preferences are specific for the form. I think that a drop-down menu is better from the view point that the values are already filtered however for flexibility and accessibility to diseabled persons an input text field is better.
          Hide
          Oleg Sychev added a comment -

          I must apologize for a long comment - it probably looked worse than it is. The actual interface will be very slim and the patch too.

          The actual changes proposed by me for repeat_elements is quite small:
          a) replace button 'Add [3] choices' with text field 'Enter the number of choices [ 3 ]' and button 'Apply' (as in Tim suggestion). Drop-down menu is cool but it will limit the number of blanks.
          b) place them before repeated elements (it has some sense to put "Add more blanks" button on the end of sequence - user will press it if he/she will run out of existing blanks; hovewer when user can see and correct the number of blanks it usually will be done before entering the actual values in blanks).
          c) let caller of repeat_elements add some elements to this fieldset to use with 'Apply' button if he/she want it - it hurt nothing and help speed up question editing a lot, since it will save us one page reload - and page reload with 5-12 htmleditors (or more) is a big deal. Also I'm not fond of having several buttons for applying different options on editing one question.

          I'm unsure about using user preferences as the using of htmleditors and required number of blanks may depend on the nature of questions, not just on preferences for this user, but this isn't a bug deal for me, so I can accept you suggestions.

          As for the matter of reducing the number of filled blanks, I think the best solution will be to ask additional question (Are you sure you want to delete ...?), but I'm unsure if this possible with formslib. If impossible, probably better to keep filled blanks (and show user an error to clear them manually before reducing the number of blanks) than risk unwanted deletion of useful data.

          Show
          Oleg Sychev added a comment - I must apologize for a long comment - it probably looked worse than it is. The actual interface will be very slim and the patch too. The actual changes proposed by me for repeat_elements is quite small: a) replace button 'Add [3] choices' with text field 'Enter the number of choices [ 3 ]' and button 'Apply' (as in Tim suggestion). Drop-down menu is cool but it will limit the number of blanks. b) place them before repeated elements (it has some sense to put "Add more blanks" button on the end of sequence - user will press it if he/she will run out of existing blanks; hovewer when user can see and correct the number of blanks it usually will be done before entering the actual values in blanks). c) let caller of repeat_elements add some elements to this fieldset to use with 'Apply' button if he/she want it - it hurt nothing and help speed up question editing a lot, since it will save us one page reload - and page reload with 5-12 htmleditors (or more) is a big deal. Also I'm not fond of having several buttons for applying different options on editing one question. I'm unsure about using user preferences as the using of htmleditors and required number of blanks may depend on the nature of questions, not just on preferences for this user, but this isn't a bug deal for me, so I can accept you suggestions. As for the matter of reducing the number of filled blanks, I think the best solution will be to ask additional question (Are you sure you want to delete ...?), but I'm unsure if this possible with formslib. If impossible, probably better to keep filled blanks (and show user an error to clear them manually before reducing the number of blanks) than risk unwanted deletion of useful data.
          Hide
          Tim Hunt added a comment -

          Choice of number of blanks before the repeated element. Yes. That is good.

          Using a dropdown does not stop people typing the number if they want. It does put a finite upper limit on, but it can be very large. For example 100. And then if the forum is currently showing more than, say, 50 repeats, extend the dropdown to show 200 choices.

          With user preferences, I was thinking of a different preference for each type of form, so there is one setting for multiple choices, and a separate one for shortanswer. That is, make the name of the preference a combination of the forum name and the repeated item name.

          I don't think people will need to change the HTML editor settings very often if we are using user preferences, which I why I think we should keep them speparate, for the reason of simplicity.

          Actually, in Moodle 2.0, there is already a button next to each HTML editor to switch from the editor to plain textarea and back without a reload at all. So actually, that feature is not required in core in the long term.

          So I suggest you do the repeat stuff as a patch for me to apply to core, and you do the other changes just as a local customisation for your site, which lets you do exactly what you want.

          Show
          Tim Hunt added a comment - Choice of number of blanks before the repeated element. Yes. That is good. Using a dropdown does not stop people typing the number if they want. It does put a finite upper limit on, but it can be very large. For example 100. And then if the forum is currently showing more than, say, 50 repeats, extend the dropdown to show 200 choices. With user preferences, I was thinking of a different preference for each type of form, so there is one setting for multiple choices, and a separate one for shortanswer. That is, make the name of the preference a combination of the forum name and the repeated item name. I don't think people will need to change the HTML editor settings very often if we are using user preferences, which I why I think we should keep them speparate, for the reason of simplicity. Actually, in Moodle 2.0, there is already a button next to each HTML editor to switch from the editor to plain textarea and back without a reload at all. So actually, that feature is not required in core in the long term. So I suggest you do the repeat stuff as a patch for me to apply to core, and you do the other changes just as a local customisation for your site, which lets you do exactly what you want.
          Hide
          Oleg Sychev added a comment -

          Hi, Tim!

          "Using a dropdown does not stop people typing the number if they want. It does put a finite upper limit on, but it can be very large. " I'm unsure - as an experienced user i'm agree with you, after dealing with complaints of unexperienced users, that don't know that, I'm disagree. On the other hand, typical user usually don't enter text in the field where he/she already have seen a number. Probably I'm not quite understand, what wrong with text field with PARAM_INT to see you point there. Also, using text field will simplify the patch.

          "With user preferences, I was thinking of a different preference for each type of form, so there is one setting for multiple choices, and a separate one for shortanswer. That is, make the name of the preference a combination of the forum name and the repeated item name. " - good idea.

          " don't think people will need to change the HTML editor settings very often if we are using user preferences, which I why I think we should keep them speparate, for the reason of simplicity." Having two different fieldsets and 'Apply' button for different editing options isn't a simplicity for me, i'll rather have them in one group (or you mean simplicity for the programmer?). Anyway, see suggestions below on it.

          "Actually, in Moodle 2.0, there is already a button next to each HTML editor to switch from the editor to plain textarea and back without a reload at all. So actually, that feature is not required in core in the long term. " Having such button is good, but using it on question editing page requires additional clarification. There are two issues caused by large number of HTMLEditors there:
          a) slow page load
          b) browser malfunction (when the number is great)

          To work correctly with this we must address two issues:
          I) defaul value for repeated elements must be textarea (not htmleditor), otherwise we get issue a) by default and if we get b), than we can't use htmleditors at all, even where we want them (for example on the question text)
          II) there must be a way to control htmleditors in similar field in all repeats: having to press a button on each of 5-15 blanks isn't terribly usable (there is also an issue about what type wil have the added blanks). So now I think, that repeat_elements must create separate buttons to control all textarea/htmleditor option on each repeated field that allows it (this doesn't forbid individual switching, but mass switching is far more common). Also this can be saved in user preferences too.

          So, I suggest the best solution is o have two patches to the repeat_elements function:
          1) choice the number of blanks - targeted for both 1.9 and 2.0
          2) buttons to handle using htmleditors on repeated elements at once - targeted for 2.0 only. Handling htmleditors for 1.9 should be done on local customisation basis.

          Show
          Oleg Sychev added a comment - Hi, Tim! "Using a dropdown does not stop people typing the number if they want. It does put a finite upper limit on, but it can be very large. " I'm unsure - as an experienced user i'm agree with you, after dealing with complaints of unexperienced users, that don't know that, I'm disagree. On the other hand, typical user usually don't enter text in the field where he/she already have seen a number. Probably I'm not quite understand, what wrong with text field with PARAM_INT to see you point there. Also, using text field will simplify the patch. "With user preferences, I was thinking of a different preference for each type of form, so there is one setting for multiple choices, and a separate one for shortanswer. That is, make the name of the preference a combination of the forum name and the repeated item name. " - good idea. " don't think people will need to change the HTML editor settings very often if we are using user preferences, which I why I think we should keep them speparate, for the reason of simplicity." Having two different fieldsets and 'Apply' button for different editing options isn't a simplicity for me, i'll rather have them in one group (or you mean simplicity for the programmer?). Anyway, see suggestions below on it. "Actually, in Moodle 2.0, there is already a button next to each HTML editor to switch from the editor to plain textarea and back without a reload at all. So actually, that feature is not required in core in the long term. " Having such button is good, but using it on question editing page requires additional clarification. There are two issues caused by large number of HTMLEditors there: a) slow page load b) browser malfunction (when the number is great) To work correctly with this we must address two issues: I) defaul value for repeated elements must be textarea (not htmleditor), otherwise we get issue a) by default and if we get b), than we can't use htmleditors at all, even where we want them (for example on the question text) II) there must be a way to control htmleditors in similar field in all repeats: having to press a button on each of 5-15 blanks isn't terribly usable (there is also an issue about what type wil have the added blanks). So now I think, that repeat_elements must create separate buttons to control all textarea/htmleditor option on each repeated field that allows it (this doesn't forbid individual switching, but mass switching is far more common). Also this can be saved in user preferences too. So, I suggest the best solution is o have two patches to the repeat_elements function: 1) choice the number of blanks - targeted for both 1.9 and 2.0 2) buttons to handle using htmleditors on repeated elements at once - targeted for 2.0 only. Handling htmleditors for 1.9 should be done on local customisation basis.
          Hide
          Tim Hunt added a comment -

          I) In Moodle 2.0 we are using a completely different HTML editor (TinyMCE instead of HTML area). So hopefully your concerns do not arise.

          II) Yes, we should do this, but just by linking the existing toggle buttons on the corresponding fields, not by introducing a new UI element.

          Anyway, I think we are broadly in agreement, so you could just go ahead and make a patch. Then we can look at it for real and see how well it works. That will probably settle the argument.

          Show
          Tim Hunt added a comment - I) In Moodle 2.0 we are using a completely different HTML editor (TinyMCE instead of HTML area). So hopefully your concerns do not arise. II) Yes, we should do this, but just by linking the existing toggle buttons on the corresponding fields, not by introducing a new UI element. Anyway, I think we are broadly in agreement, so you could just go ahead and make a patch. Then we can look at it for real and see how well it works. That will probably settle the argument.
          Hide
          Oleg Sychev added a comment -

          This is a first, trial version of the patch to Moodle 1.9 to allow us dicuss things properly. It has several issues and intentional violations of Moodle rules:
          1) language strings are hardcoded to reduce the number of patched files, i'll move them to language packs in final version
          2) I would like to group controls (see commented code), but failed to find a way to create 'compare' rule for the elements in group
          3) string with compare rule is commented: it's uncommenting causes fatal error in getValidationScript function - probably compare rule doesn't work well with hidden elements
          4) should we now add an empty blanks when editing question? I leave this to be right now.
          5) there is a problem with reducing number of blanks: user can unitentionally delete useful data. There are several approaches to this problem:
          a) prohibit reducing number of blanks (the simplest way - implemented now)
          b) show an error if user want to delete filled blanks, deleting them if he confirm deletion (this requires a good deal of programming and no small patch because repeat_elements must analyze an arbitrary elements array to find filled/unfilled blanks)

          Show
          Oleg Sychev added a comment - This is a first, trial version of the patch to Moodle 1.9 to allow us dicuss things properly. It has several issues and intentional violations of Moodle rules: 1) language strings are hardcoded to reduce the number of patched files, i'll move them to language packs in final version 2) I would like to group controls (see commented code), but failed to find a way to create 'compare' rule for the elements in group 3) string with compare rule is commented: it's uncommenting causes fatal error in getValidationScript function - probably compare rule doesn't work well with hidden elements 4) should we now add an empty blanks when editing question? I leave this to be right now. 5) there is a problem with reducing number of blanks: user can unitentionally delete useful data. There are several approaches to this problem: a) prohibit reducing number of blanks (the simplest way - implemented now) b) show an error if user want to delete filled blanks, deleting them if he confirm deletion (this requires a good deal of programming and no small patch because repeat_elements must analyze an arbitrary elements array to find filled/unfilled blanks)
          Hide
          Oleg Sychev added a comment -

          The patch also lacked using user preferences beacause i'm unsure - if number of blanks as a preference must be related to question type only, or to question category too (it's customary to have similar questions in one category, but one user can enter a totally different questions of one type depending of his needs).

          Show
          Oleg Sychev added a comment - The patch also lacked using user preferences beacause i'm unsure - if number of blanks as a preference must be related to question type only, or to question category too (it's customary to have similar questions in one category, but one user can enter a totally different questions of one type depending of his needs).
          Hide
          Pierre Pichet added a comment -

          In the actual code unfilled answers are filtered and not stored either in the validation process of the edit form or in the QTYPE save_options code so b) is not a real problem although this could be improved later...

          Show
          Pierre Pichet added a comment - In the actual code unfilled answers are filtered and not stored either in the validation process of the edit form or in the QTYPE save_options code so b) is not a real problem although this could be improved later...
          Hide
          Oleg Sychev added a comment -

          Pierre, there is a big difference between actual code and b):
          in actual code unfilled blanks is the problem of the caller, who knows specifically what he/she wants and what the structure of blanks is;
          in b) this will be a problem of repeat_elements who must adapt to arbitrary blanks.

          Show
          Oleg Sychev added a comment - Pierre, there is a big difference between actual code and b): in actual code unfilled blanks is the problem of the caller, who knows specifically what he/she wants and what the structure of blanks is; in b) this will be a problem of repeat_elements who must adapt to arbitrary blanks.
          Hide
          Pierre Pichet added a comment -

          I did not understand that your code should adjust to a decrease of the number of repeated items.
          Perhaps you should work with a less restrictive challenge i.e. the repeat element start with the initial repeat elements increment and if the user chage this to a lower value these initial elements be maintained and the new added elements follow the "decreased" increment value rule .
          When the user come next to this questiontype, the "decreased" increment value will be the default.

          Show
          Pierre Pichet added a comment - I did not understand that your code should adjust to a decrease of the number of repeated items. Perhaps you should work with a less restrictive challenge i.e. the repeat element start with the initial repeat elements increment and if the user chage this to a lower value these initial elements be maintained and the new added elements follow the "decreased" increment value rule . When the user come next to this questiontype, the "decreased" increment value will be the default.
          Hide
          Oleg Sychev added a comment -

          Hi, Pierre

          Now I compose my thoughts and found this:
          a) there is no real point in decreasing the number of blanks on already loaded form - it costs a page reload and helps nothing except lessing work to get to the bottom of the page - rather minor problem - so there is safe to prohibit this
          b) we must save the number of blanks as user's preferences - there will be a two preferences - global (from name plus repeated element name) and local (adding question category name)
          c) on adding new question we use local user preference, than global if local absent, than default value for this questiontype
          d) on editing question we add 1-3 empty blanks to the current question just in case user want to add a choice, old user preferences is ignored (but probably updated, at least local)

          I'm agree that default value MUST be decreased if we use this scheme. The problem is how to tell user that decreased number will be saved:
          1) if we use client side validation to not allow to decrease the number of blanks, than server script simply doesn't know about this, so it can't decrease number (and there is a bug in formslib probably related to 'compare' type rules)
          2) if we compare the numbers on server, the user can't see error messages because they isn't displayed with noSubmitButton; also this requires page reloads - this can be saved by reporting errors even with noSubmitButton, if Jamie would be so kind to improve formslib or at least tell me, where to see and what to do to allow this.

          Show
          Oleg Sychev added a comment - Hi, Pierre Now I compose my thoughts and found this: a) there is no real point in decreasing the number of blanks on already loaded form - it costs a page reload and helps nothing except lessing work to get to the bottom of the page - rather minor problem - so there is safe to prohibit this b) we must save the number of blanks as user's preferences - there will be a two preferences - global (from name plus repeated element name) and local (adding question category name) c) on adding new question we use local user preference, than global if local absent, than default value for this questiontype d) on editing question we add 1-3 empty blanks to the current question just in case user want to add a choice, old user preferences is ignored (but probably updated, at least local) I'm agree that default value MUST be decreased if we use this scheme. The problem is how to tell user that decreased number will be saved: 1) if we use client side validation to not allow to decrease the number of blanks, than server script simply doesn't know about this, so it can't decrease number (and there is a bug in formslib probably related to 'compare' type rules) 2) if we compare the numbers on server, the user can't see error messages because they isn't displayed with noSubmitButton; also this requires page reloads - this can be saved by reporting errors even with noSubmitButton, if Jamie would be so kind to improve formslib or at least tell me, where to see and what to do to allow this.
          Hide
          Pierre Pichet added a comment -

          "on adding new question we use local user preference, than global if local absent, than default value for this questiontype"

          Is this allows for the fact that the local user preferences are specific to the question type that is edited ?

          "The problem is how to tell user that decreased number will be saved"
          we don`'t tell him, just add this info to the docs.
          He will notice the next time.
          Say default is 3 new answers, that the user push to 10 new answers (in multiple choice for example.
          If he put 10 he will have 13 of them on this question the 23 if he continue.
          On return the default will be 10.
          If there is too many answers the validation process will eliminate all unfilled ones with the actual rules i.e. answer ans feedback == null and grade == 0 .

          Show
          Pierre Pichet added a comment - "on adding new question we use local user preference, than global if local absent, than default value for this questiontype" Is this allows for the fact that the local user preferences are specific to the question type that is edited ? "The problem is how to tell user that decreased number will be saved" we don`'t tell him, just add this info to the docs. He will notice the next time. Say default is 3 new answers, that the user push to 10 new answers (in multiple choice for example. If he put 10 he will have 13 of them on this question the 23 if he continue. On return the default will be 10. If there is too many answers the validation process will eliminate all unfilled ones with the actual rules i.e. answer ans feedback == null and grade == 0 .
          Hide
          Oleg Sychev added a comment -

          Yes, the preferences would be specific to the questiontype. They can be more specific based on the question category, but it's probably not worth the trouble, since getting category name in question editing form is somewhat tricky (actually number of choices/answers is more a matter of a question category than users preferences - imagine several staff memebers supporting a common set of categories with different pedagogical purposes for categories).

          There is no need to inform user now - it's better to set preferences based on the number of really filled subquestions, not on the number that user types in the 'Number of blanks' field (or there would be no way to reduce it at all thanks to the way moodleforms working). This means that user preferences on the number of repeats must be handled outside repeat_elements function.

          Show
          Oleg Sychev added a comment - Yes, the preferences would be specific to the questiontype. They can be more specific based on the question category, but it's probably not worth the trouble, since getting category name in question editing form is somewhat tricky (actually number of choices/answers is more a matter of a question category than users preferences - imagine several staff memebers supporting a common set of categories with different pedagogical purposes for categories). There is no need to inform user now - it's better to set preferences based on the number of really filled subquestions, not on the number that user types in the 'Number of blanks' field (or there would be no way to reduce it at all thanks to the way moodleforms working). This means that user preferences on the number of repeats must be handled outside repeat_elements function.
          Hide
          Oleg Sychev added a comment -

          This is more mature version, tested of patch (forsmlib.php and match question files to test it) featuring several improvements and issues:
          1) user preferences is working now. User can't reduce the number of blanks on the current page (see discussion above), but actual number of entered subquestions is saved as user preference to be used on the next new question of the same type; on editing existing questions we just add QUESTION_NUMANS_ADD (which probably need to be lowered to 1-2) blanks to the current number of subquestions.
          2) we still need for MDL-17111 to be fixed if we want to display error message to the user on the matter of decreasing number of blanks.
          3) string still hardcoded - I'll move them after you approval of the patch

          Show
          Oleg Sychev added a comment - This is more mature version, tested of patch (forsmlib.php and match question files to test it) featuring several improvements and issues: 1) user preferences is working now. User can't reduce the number of blanks on the current page (see discussion above), but actual number of entered subquestions is saved as user preference to be used on the next new question of the same type; on editing existing questions we just add QUESTION_NUMANS_ADD (which probably need to be lowered to 1-2) blanks to the current number of subquestions. 2) we still need for MDL-17111 to be fixed if we want to display error message to the user on the matter of decreasing number of blanks. 3) string still hardcoded - I'll move them after you approval of the patch
          Hide
          Oleg Sychev added a comment -

          Tim, you wrote "Using a dropdown does not stop people typing the number if they want". I'm considering using drop down menu to avoid MDL-17111, but system of typing a number in dropdown menu puzzles me: at least on widely used Internet Explorer it's very strange - you can't just type number (especially in two digits), you need to press a lot of buttons to get to twenty or more numbers.

          Show
          Oleg Sychev added a comment - Tim, you wrote "Using a dropdown does not stop people typing the number if they want". I'm considering using drop down menu to avoid MDL-17111 , but system of typing a number in dropdown menu puzzles me: at least on widely used Internet Explorer it's very strange - you can't just type number (especially in two digits), you need to press a lot of buttons to get to twenty or more numbers.
          Hide
          Oleg Sychev added a comment -

          Hi, Tim. The patch now requires you attention!

          This update is final beta version of patch (use with patches to Match questiontype: edit_match_form.php.patch2 and questiontype.php.patch2 for testing - be careful, as it's breaks compatibility with other forms using repeat_elements). The patch features:
          1) rough fix for MDL-17111 that allows us to work without errors there
          2) code cleanup in repeat_elements

          Now I wait for Tim comments and approval (other comments are welcome as well). After you approval I'll move strings to language packs, edit out temporary comments about changes in interface of repeat_elements and create patches for all other question types using repeat_elements (other forms as well if necessary).

          Show
          Oleg Sychev added a comment - Hi, Tim. The patch now requires you attention! This update is final beta version of patch (use with patches to Match questiontype: edit_match_form.php.patch2 and questiontype.php.patch2 for testing - be careful, as it's breaks compatibility with other forms using repeat_elements). The patch features: 1) rough fix for MDL-17111 that allows us to work without errors there 2) code cleanup in repeat_elements Now I wait for Tim comments and approval (other comments are welcome as well). After you approval I'll move strings to language packs, edit out temporary comments about changes in interface of repeat_elements and create patches for all other question types using repeat_elements (other forms as well if necessary).
          Hide
          Pierre Pichet added a comment -

          which means in 1.9
          Function and Method Cross Reference
          repeat_elements()
          Defined at:

          • /lib/formslib.php -> line 519

          Referenced 11 times:

          • /question/type/calculated/edit_calculated_form.php -> line 76
          • /question/type/calculated/edit_calculated_form.php -> line 97
          • /question/type/missingtype/edit_missingtype_form.php -> line 45
          • /question/type/multichoice/edit_multichoice_form.php -> line 65
          • /question/type/shortanswer/edit_shortanswer_form.php -> line 51
          • /question/type/numerical/edit_numerical_form.php -> line 55
          • /question/type/numerical/edit_numerical_form.php -> line 78
          • /mod/quiz/mod_form.php -> line 239
          • /question/type/match/edit_match_form.php -> line 48
          • /mod/choice/mod_form.php -> line 56
          Show
          Pierre Pichet added a comment - which means in 1.9 Function and Method Cross Reference repeat_elements() Defined at: /lib/formslib.php -> line 519 Referenced 11 times: /question/type/calculated/edit_calculated_form.php -> line 76 /question/type/calculated/edit_calculated_form.php -> line 97 /question/type/missingtype/edit_missingtype_form.php -> line 45 /question/type/multichoice/edit_multichoice_form.php -> line 65 /question/type/shortanswer/edit_shortanswer_form.php -> line 51 /question/type/numerical/edit_numerical_form.php -> line 55 /question/type/numerical/edit_numerical_form.php -> line 78 /mod/quiz/mod_form.php -> line 239 /question/type/match/edit_match_form.php -> line 48 /mod/choice/mod_form.php -> line 56
          Hide
          Oleg Sychev added a comment -

          11 calls is no problem now, while I have some free time (it can change in time). The changes are quite cosmetic.

          The main thing we need now is Tim's review and decision - I'm certainly isn't going to do any mass changing before his approval.

          Show
          Oleg Sychev added a comment - 11 calls is no problem now, while I have some free time (it can change in time). The changes are quite cosmetic. The main thing we need now is Tim's review and decision - I'm certainly isn't going to do any mass changing before his approval.
          Hide
          Pierre Pichet added a comment -

          Hi Oleg,
          Pressing the enter key can be similar to submitting the form.
          Could you test on your actual version how pressing the enter key in the text element ( $mform->addElement('text',$repeatname,$repeatstring,array('size'=>3));
          is interpreted?

          Show
          Pierre Pichet added a comment - Hi Oleg, Pressing the enter key can be similar to submitting the form. Could you test on your actual version how pressing the enter key in the text element ( $mform->addElement('text',$repeatname,$repeatstring,array('size'=>3)); is interpreted?
          Hide
          Oleg Sychev added a comment -

          Hi Pierre,
          On my version pressing enter key in the 'number of blanks' field doesn't submit form, it act as if 'Apply' button is pressed (which is intended). I guess formslib take care of this somehow.

          Show
          Oleg Sychev added a comment - Hi Pierre, On my version pressing enter key in the 'number of blanks' field doesn't submit form, it act as if 'Apply' button is pressed (which is intended). I guess formslib take care of this somehow.
          Hide
          Pierre Pichet added a comment -

          "as if 'Apply' button is pressed (which is intended)"
          Does it generates new answers i.e. the same effect that using the $mform->addElement('submit',$addfieldsname,$addstring);
          button?.

          Show
          Pierre Pichet added a comment - "as if 'Apply' button is pressed (which is intended)" Does it generates new answers i.e. the same effect that using the $mform->addElement('submit',$addfieldsname,$addstring); button?.
          Hide
          Oleg Sychev added a comment -

          Well, I'm a sort of managed to check out a CVS code for Moodle 1.9 and managed to create a CVS patch for this. Since all patch is now in one file I moved strings to their appropriate place.

          It would be good if the patch will get reviewing before I forget details about this all.

          Show
          Oleg Sychev added a comment - Well, I'm a sort of managed to check out a CVS code for Moodle 1.9 and managed to create a CVS patch for this. Since all patch is now in one file I moved strings to their appropriate place. It would be good if the patch will get reviewing before I forget details about this all.
          Hide
          Oleg Sychev added a comment -

          Yes, Pierre, pressing enter key in that field generates new answers.

          Show
          Oleg Sychev added a comment - Yes, Pierre, pressing enter key in that field generates new answers.
          Hide
          Tim Hunt added a comment -

          Nice work with the CVS patch. Makes it really easy for me to review. Thanks.

          (For anyone wondering, numberofblanks.beta.patch (10 kb) is the latest patch).

          Nice to see that it basically works. I like the outcome, but I don't like the way you have implemented it. It is not yet clean enough to go into core.

          1. I don't think it is a good idea to set the user preference in questiontype.php based on the number of answers that were entered. For example, if you are creating a lot of matching questions with 5 or 6 pairs of answers, then you would like to set the number of blanks to 6 at the start of the editing session and have it stay like that. It would be annoying if it changed every time you created a question with 5 answers.

          2. Similarly, I think there should be no mention of the user preference in edit_match_form. The form should just be responsible for determining how many blanks this question acutally uses - that is the code that was there before, which would be better re-written as

          $repeatsatstart = max(QUESTION_NUMANS_START, $countsubquestions + QUESTION_NUMANS_ADD);

          3. That is, I think the user_preferences stuff should be entirely contained in formslib. The calling code in the matching question type should not even know about it. That is, the specific form should pass in a parameter like $repeatsatstart, which is the minium number of blanks that the thing edited need. Then inside repeat_elements it should do $actualnumrepeats = max($repeatsatstart, $userpref);

          4. I don't really like the way that your code adds a new section to the form. Some people won't like that. For example, look at the 'Overall feedback' section on the quiz settings form. I think that 'Provide space for XXX things' should be one very plain line of content, that can appear anywhere in the form.

          5. repeat_elements now has a scary number of arguments. A function with 9 arguments should not be allowed. Can we do anything about that?

          Show
          Tim Hunt added a comment - Nice work with the CVS patch. Makes it really easy for me to review. Thanks. (For anyone wondering, numberofblanks.beta.patch (10 kb) is the latest patch). Nice to see that it basically works. I like the outcome, but I don't like the way you have implemented it. It is not yet clean enough to go into core. 1. I don't think it is a good idea to set the user preference in questiontype.php based on the number of answers that were entered. For example, if you are creating a lot of matching questions with 5 or 6 pairs of answers, then you would like to set the number of blanks to 6 at the start of the editing session and have it stay like that. It would be annoying if it changed every time you created a question with 5 answers. 2. Similarly, I think there should be no mention of the user preference in edit_match_form. The form should just be responsible for determining how many blanks this question acutally uses - that is the code that was there before, which would be better re-written as $repeatsatstart = max(QUESTION_NUMANS_START, $countsubquestions + QUESTION_NUMANS_ADD); 3. That is, I think the user_preferences stuff should be entirely contained in formslib. The calling code in the matching question type should not even know about it. That is, the specific form should pass in a parameter like $repeatsatstart, which is the minium number of blanks that the thing edited need. Then inside repeat_elements it should do $actualnumrepeats = max($repeatsatstart, $userpref); 4. I don't really like the way that your code adds a new section to the form. Some people won't like that. For example, look at the 'Overall feedback' section on the quiz settings form. I think that 'Provide space for XXX things' should be one very plain line of content, that can appear anywhere in the form. 5. repeat_elements now has a scary number of arguments. A function with 9 arguments should not be allowed. Can we do anything about that?
          Hide
          Oleg Sychev added a comment -

          Hi, Tim. Glad to see you there!

          1&2&3 First of all I'm not completely satisfacted by this solution myself, but was somewhat convinvced that there is no better way to handle this.

          The core of the problem is deleting unused blanks. We can't allow user just decrease the number of blanks, as he/she can accidentally delete useful data. Right now repeat_elements just don't allow to do it.
          "The form should just be responsible for determining how many blanks this question acutally uses - that is the code that was there before " - the problem is that code before determined the number of blanks only in validate and save_options functions, that is only when start editing and after submit button is pressed. When the user pressed 'Add 3 blanks' button the number of blanks wasn't handled by form - it was handled by repeat_element instead:
          if (!empty($addfields))

          { $repeats += $addfieldsno; }

          (you can see it in patch, deleted). Form before calling repeat_elements use $this->question to determine the number of subquestions - and it isn't updated when user reloads form pressing 'Apply'.

          "That is, the specific form should pass in a parameter like $repeatsatstart, which is the minium number of blanks that the thing edited need. Then inside repeat_elements it should do $actualnumrepeats = max($repeatsatstart, $userpref); " - not so easy, as the user can't decrease the number of blanks on the current form right now (because repeat_elements can't get the number of filled blanks from the caller on reloading page) - so with you solution he can't decrease the number of blanks for this questiontype at all once he raised it! That leads me to the conclusion, that userpref must be based on the number of actual filled blanks, so there will be some way to decrease the number of blanks.

          I can thought of only one other way to implement it: the caller of repeat_elements must supply a function, which returns the index of last filled blank. As some forms use repeat_elements several times, we either must use a callback function, or pass some of the repeat_elements arguments to this function so it can determine, about what set of blanks we are asking (it probably must be a prefix, see 5 below).

          Also, if I understand you code rightly, question editing form sometimes used without an actual editing question ($this->question->formoptions->repeatelements=false). Setting numofrepeats to max($repeatsatstart, $userpref) in that case will create strange results. The interface to ajust the number of blanks probably should be hidden in that case too. Can we really rely on assumption that blanks can be added in any case without harm to anything?

          So, what you would like to see implemented there? Additional form function and user preferences in formslib or leave userprefs to the caller? Or can you see a better way to implement this?

          "For example, if you are creating a lot of matching questions with 5 or 6 pairs of answers" - we can easily handle this by adding QUESTION_NUMANS_ADD to user preferences when creating new question. Fixed. I think that QUESTION_NUMANS_ADD should be lowered to 2 in it's new function.

          4. I don't like it too, but somewhat short of options. To place text and button on one line we must create a group. The way PEAR handled compare rule is strange: you must supply an array of elements to it. Since adding group rule you must supply an element name as an index, you can't use compare rule in such way - array can't be an index. There are two other options:
          a) validate it on server in repeat_elements (without rules at all) - this will costs additional page reloads; using rule would also be the only way to display an error message to describe what is happening, if Jamie would fix MDL-17111 since definition function can't show any errors. However this would be only way with additional function.
          b) use a dropdown - but at least on widely used Internet Explorer the way it hanldes trys to enter the number is quite strange. My opinion is that having to type much (or browse large dropdown) is worse than get one more line (on already big page, where no one will notice it).

          I beg you to at least consider these backsides of grouping (additional page reloads and no error messages or inconviniences to get required number) before insisting of grouping. We can group without better options if we will use caller-supplied function to determine the number of blanks thought.

          I can easily make an option to not use separate fieldset thought. Is one or two lines really matters? It is fieldsets that actually takes a lot of place (especially fieldsets for blanks). Pity we can't use some sort of horizontal line to separate blanks instead - this will realy makes a difference for question editing.

          5. Actually yes. repeat_elements can recieve a prefix for additional elements names ($repeathiddenname, $repeatname, $groupname, $headername, $addfieldsname) instead of getting actual names.

          So, I said my reasons. Whats now you decision on:
          A) Should we use an additional function, supplied by form, to determine the number of filled blanks, or just leave preferences to the caller, or use some better way I just can't see?
          B) How should we treat case with $this->question->formoptions->repeatelements=false? Elements to handle the number of blanks probably should be hidden at all in that case, and userprefs ignored as well. Probably there must be an option to do so in repeat_elements.
          C) What is a lesser evil: page reloading (group with text field), inconvinience entering the number of blanks (group with dropdown) or placing 'Aplly' button on separate line? Should there be an option to not use separate fieldset for controls?
          D) Should we pass a prefix instead of element's names? We can win 4 arguments (but 2 can be lost for new options mentioned above).
          I'll make a new patch according to you decision.

          Show
          Oleg Sychev added a comment - Hi, Tim. Glad to see you there! 1&2&3 First of all I'm not completely satisfacted by this solution myself, but was somewhat convinvced that there is no better way to handle this. The core of the problem is deleting unused blanks. We can't allow user just decrease the number of blanks, as he/she can accidentally delete useful data. Right now repeat_elements just don't allow to do it. "The form should just be responsible for determining how many blanks this question acutally uses - that is the code that was there before " - the problem is that code before determined the number of blanks only in validate and save_options functions, that is only when start editing and after submit button is pressed. When the user pressed 'Add 3 blanks' button the number of blanks wasn't handled by form - it was handled by repeat_element instead: if (!empty($addfields)) { $repeats += $addfieldsno; } (you can see it in patch, deleted). Form before calling repeat_elements use $this->question to determine the number of subquestions - and it isn't updated when user reloads form pressing 'Apply'. "That is, the specific form should pass in a parameter like $repeatsatstart, which is the minium number of blanks that the thing edited need. Then inside repeat_elements it should do $actualnumrepeats = max($repeatsatstart, $userpref); " - not so easy, as the user can't decrease the number of blanks on the current form right now (because repeat_elements can't get the number of filled blanks from the caller on reloading page) - so with you solution he can't decrease the number of blanks for this questiontype at all once he raised it! That leads me to the conclusion, that userpref must be based on the number of actual filled blanks, so there will be some way to decrease the number of blanks. I can thought of only one other way to implement it: the caller of repeat_elements must supply a function, which returns the index of last filled blank. As some forms use repeat_elements several times, we either must use a callback function, or pass some of the repeat_elements arguments to this function so it can determine, about what set of blanks we are asking (it probably must be a prefix, see 5 below). Also, if I understand you code rightly, question editing form sometimes used without an actual editing question ($this->question->formoptions->repeatelements=false). Setting numofrepeats to max($repeatsatstart, $userpref) in that case will create strange results. The interface to ajust the number of blanks probably should be hidden in that case too. Can we really rely on assumption that blanks can be added in any case without harm to anything? So, what you would like to see implemented there? Additional form function and user preferences in formslib or leave userprefs to the caller? Or can you see a better way to implement this? "For example, if you are creating a lot of matching questions with 5 or 6 pairs of answers" - we can easily handle this by adding QUESTION_NUMANS_ADD to user preferences when creating new question. Fixed. I think that QUESTION_NUMANS_ADD should be lowered to 2 in it's new function. 4. I don't like it too, but somewhat short of options. To place text and button on one line we must create a group. The way PEAR handled compare rule is strange: you must supply an array of elements to it. Since adding group rule you must supply an element name as an index, you can't use compare rule in such way - array can't be an index. There are two other options: a) validate it on server in repeat_elements (without rules at all) - this will costs additional page reloads; using rule would also be the only way to display an error message to describe what is happening, if Jamie would fix MDL-17111 since definition function can't show any errors. However this would be only way with additional function. b) use a dropdown - but at least on widely used Internet Explorer the way it hanldes trys to enter the number is quite strange. My opinion is that having to type much (or browse large dropdown) is worse than get one more line (on already big page, where no one will notice it). I beg you to at least consider these backsides of grouping (additional page reloads and no error messages or inconviniences to get required number) before insisting of grouping. We can group without better options if we will use caller-supplied function to determine the number of blanks thought. I can easily make an option to not use separate fieldset thought. Is one or two lines really matters? It is fieldsets that actually takes a lot of place (especially fieldsets for blanks). Pity we can't use some sort of horizontal line to separate blanks instead - this will realy makes a difference for question editing. 5. Actually yes. repeat_elements can recieve a prefix for additional elements names ($repeathiddenname, $repeatname, $groupname, $headername, $addfieldsname) instead of getting actual names. So, I said my reasons. Whats now you decision on: A) Should we use an additional function, supplied by form, to determine the number of filled blanks, or just leave preferences to the caller, or use some better way I just can't see? B) How should we treat case with $this->question->formoptions->repeatelements=false? Elements to handle the number of blanks probably should be hidden at all in that case, and userprefs ignored as well. Probably there must be an option to do so in repeat_elements. C) What is a lesser evil: page reloading (group with text field), inconvinience entering the number of blanks (group with dropdown) or placing 'Aplly' button on separate line? Should there be an option to not use separate fieldset for controls? D) Should we pass a prefix instead of element's names? We can win 4 arguments (but 2 can be lost for new options mentioned above). I'll make a new patch according to you decision.
          Hide
          Oleg Sychev added a comment -

          This is the second beta version of patch. It's features:
          1) handling user preferences by repeat_elements
          2) allows user to reduce number of blanks (deleting only unused blanks)
          3) the controls are grouped and the separate fieldset for them is the problem of the caller (if he want it)
          4) we can hide controls at all if needed (also ignoring userprefs in the case)
          5) the number of arguments is 7 now
          6) questiontype.php can remain unchanged now

          Known problems:
          a) the code in the caller become bigger, but also less sparse
          b) should we update userprefs on forms where the controls are hidden? Probably no.
          c) somewhat hacked way to set up the number of blanks properly, but no other way seems to work and this is just how QuickForms are supposed to work
          d) there is no way to display error to the user if he/she trying to delete used blanks - definition function can't shows errors in current formslib implementation (should we ask Jamie to do so?).

          Tim, please review it and say if it's OK now.

          Show
          Oleg Sychev added a comment - This is the second beta version of patch. It's features: 1) handling user preferences by repeat_elements 2) allows user to reduce number of blanks (deleting only unused blanks) 3) the controls are grouped and the separate fieldset for them is the problem of the caller (if he want it) 4) we can hide controls at all if needed (also ignoring userprefs in the case) 5) the number of arguments is 7 now 6) questiontype.php can remain unchanged now Known problems: a) the code in the caller become bigger, but also less sparse b) should we update userprefs on forms where the controls are hidden? Probably no. c) somewhat hacked way to set up the number of blanks properly, but no other way seems to work and this is just how QuickForms are supposed to work d) there is no way to display error to the user if he/she trying to delete used blanks - definition function can't shows errors in current formslib implementation (should we ask Jamie to do so?). Tim, please review it and say if it's OK now.
          Hide
          Oleg Sychev added a comment -

          Tim
          "repeat_elements now has a scary number of arguments. A function with 9 arguments should not be allowed" - actually it had 8 arguments before reworking, 5 of them was required - not there are 7 arguments and only 4 is required.

          As for placing controls in separate fieldset I think that overall strategy can be place them in fieldset if each blank is in fieldset, otherwise no.

          It would be good if you can review this patch in the near future, as I have to rework other pages in Moodle using this function and I may be busy later on.

          Show
          Oleg Sychev added a comment - Tim "repeat_elements now has a scary number of arguments. A function with 9 arguments should not be allowed" - actually it had 8 arguments before reworking, 5 of them was required - not there are 7 arguments and only 4 is required. As for placing controls in separate fieldset I think that overall strategy can be place them in fieldset if each blank is in fieldset, otherwise no. It would be good if you can review this patch in the near future, as I have to rework other pages in Moodle using this function and I may be busy later on.
          Hide
          Tim Hunt added a comment -

          Oleg, I still think you should post in the general developer forum about this, to get more people's opinions.

          Show
          Tim Hunt added a comment - Oleg, I still think you should post in the general developer forum about this, to get more people's opinions.
          Hide
          Oleg Sychev added a comment -

          You don't mention it before. Here it is http://moodle.org/mod/forum/discuss.php?d=112251 if it's really interested someone.

          Actually, many peoples seems to don't know about repeat_elements, it's used in quiz/choice/questions only right now. One example is upload plugin for assugnment that can use it to get several files in one submit.

          Show
          Oleg Sychev added a comment - You don't mention it before. Here it is http://moodle.org/mod/forum/discuss.php?d=112251 if it's really interested someone. Actually, many peoples seems to don't know about repeat_elements, it's used in quiz/choice/questions only right now. One example is upload plugin for assugnment that can use it to get several files in one submit.
          Hide
          Oleg Sychev added a comment -

          Better version of previous patch. Features:
          1) caller can limit the number of blanks (useful when using to accept several files from student for example)
          2) default implementation of get_last_used_blank allows to work somehow even if the caller doesn't provide function (or doesn't know about changes at all, the only downside will be wrong button string in many cases)
          3) QUESTION_NUMANS_ADD lowered to 2 following change in it's meaning

          Show
          Oleg Sychev added a comment - Better version of previous patch. Features: 1) caller can limit the number of blanks (useful when using to accept several files from student for example) 2) default implementation of get_last_used_blank allows to work somehow even if the caller doesn't provide function (or doesn't know about changes at all, the only downside will be wrong button string in many cases) 3) QUESTION_NUMANS_ADD lowered to 2 following change in it's meaning
          Hide
          Oleg Sychev added a comment -

          Tim.

          I wonder, should we make this as a change in repeat_elements or create a new function? The function interface will be changed (thought it remain workable in very basic sense with old calls, but the number of choices would be uneditable), I convert all Moodle uses, but this may cause problems for 3d-party modules (for some questions for sure). On the other side, this is much needed interface improvment so waiting for 2.0 is quite a pain. But such interface change must be tied up at least with minor version change, so that any 3d-party plugin using it can have clear version distinctions for their versions.

          Or, alternatively, we can declare repeat_elements deprecated and create a new similar function, dropping support of old function in 2.0 while having 2 functions in 1.9.

          On the forum we get one vote for text field and not much else. I still wait you opinion on last patch before I start reworking all calls to the repeat_elements in Moodle.

          Show
          Oleg Sychev added a comment - Tim. I wonder, should we make this as a change in repeat_elements or create a new function? The function interface will be changed (thought it remain workable in very basic sense with old calls, but the number of choices would be uneditable), I convert all Moodle uses, but this may cause problems for 3d-party modules (for some questions for sure). On the other side, this is much needed interface improvment so waiting for 2.0 is quite a pain. But such interface change must be tied up at least with minor version change, so that any 3d-party plugin using it can have clear version distinctions for their versions. Or, alternatively, we can declare repeat_elements deprecated and create a new similar function, dropping support of old function in 2.0 while having 2 functions in 1.9. On the forum we get one vote for text field and not much else. I still wait you opinion on last patch before I start reworking all calls to the repeat_elements in Moodle.
          Hide
          Oleg Sychev added a comment -

          Tim, before resorting to have this as 2.0 only I have one last proposition: make a new function (repeat_fields or whatever) with new interface, instead of editing repeat_elements. It saves us from compatibility breaks. repeat_elements would be deleted in 2.0

          Also I'm still waiting you opinion on last patch (it isn't directly testable now). Are you content with this structure or something in it still bother you?

          Show
          Oleg Sychev added a comment - Tim, before resorting to have this as 2.0 only I have one last proposition: make a new function (repeat_fields or whatever) with new interface, instead of editing repeat_elements. It saves us from compatibility breaks. repeat_elements would be deleted in 2.0 Also I'm still waiting you opinion on last patch (it isn't directly testable now). Are you content with this structure or something in it still bother you?
          Hide
          Tim Hunt added a comment -

          I am not going to have any time to work on this any time soon, so if anyone else can take this, please do.

          Show
          Tim Hunt added a comment - I am not going to have any time to work on this any time soon, so if anyone else can take this, please do.
          Hide
          Oleg Sychev added a comment -

          Converting this to issue and creating subtasks to broke this issue in small, managable subtasks and leave aside API changing problems

          Show
          Oleg Sychev added a comment - Converting this to issue and creating subtasks to broke this issue in small, managable subtasks and leave aside API changing problems
          Hide
          Oleg Sychev added a comment -

          Actually this issue could be resolved on a different basis: make "Add xxx" button working using javascript, adding blanks without page reloads at all.

          Show
          Oleg Sychev added a comment - Actually this issue could be resolved on a different basis: make "Add xxx" button working using javascript, adding blanks without page reloads at all.
          Hide
          Tim Hunt added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Tim Hunt added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
          Hide
          Dan Marsden added a comment -

          HI Oleg - if you think this is still an issue with the latest stable releases you should update the affects version above as well.

          Show
          Dan Marsden added a comment - HI Oleg - if you think this is still an issue with the latest stable releases you should update the affects version above as well.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated: