Moodle
  1. Moodle
  2. MDL-14771

Allow a "number" field to be displayed with a specified number of decimals (Backend code)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9, 1.9.1
    • Fix Version/s: 1.9.2
    • Labels:
      None
    • Database:
      Any
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      Currently, a field of type "number" is useless, since there is no advantage compared to a simple "text" field. We all know what Excel or OpenOffice's Calc offers for a number type field. To make the number field useful, there should at least be the possibility to display a number with a given number of decimal digits.

        Gliffy Diagrams

        1. MDL-14771_20080512.patch
          1 kB
          Robert Allerstorfer
        2. MDL-14771_20080530.patch
          1 kB
          Robert Allerstorfer

          Issue Links

            Activity

            Hide
            Robert Allerstorfer added a comment - - edited

            The attached patch allows specifying a number (integer >= 0) of decimal digits a number should be displayed with. The number of decimals can be specified within the "preset.xml" file (which is part of a "preset.zip" preset), as "param1". For example, to define a number field for a Moisture in % with 2 decimals, we can use:

            <field>
            <type>number</type>
            <name>Moisture</name>
            <description>Moisture (in %)</description>
            <param1>2</param1>
            </field>

            For example, a stored number of 0.999 will be displayed as 1.00 and 0.002 will be displayed as 0.00.

            Show
            Robert Allerstorfer added a comment - - edited The attached patch allows specifying a number (integer >= 0) of decimal digits a number should be displayed with. The number of decimals can be specified within the "preset.xml" file (which is part of a "preset.zip" preset), as "param1". For example, to define a number field for a Moisture in % with 2 decimals, we can use: <field> <type>number</type> <name>Moisture</name> <description>Moisture (in %)</description> <param1>2</param1> </field> For example, a stored number of 0.999 will be displayed as 1.00 and 0.002 will be displayed as 0.00.
            Hide
            Robert Allerstorfer added a comment -

            The updated version of the patch (MDL-14771_20080512.patch) treats fields containing the number 0 properly. Works perfectly together with the patch I submitted today at MDL-14788.

            Show
            Robert Allerstorfer added a comment - The updated version of the patch ( MDL-14771 _20080512.patch) treats fields containing the number 0 properly. Works perfectly together with the patch I submitted today at MDL-14788 .
            Hide
            Robert Allerstorfer added a comment -

            Now that the MDL-14788 bugfix has been committed and verified, this patch should get committed asap, too. The currently proposed patch does not change any current functionality, so its completely harmless to commit it. The big advantage is that users can start to use the "hidden" feature by importing a well-cared "preset.zip" containing a "preset.xml" with manually added <param1></param1> tags in number fields.

            After this patch has been committed, a GUI should be added to the "Number field" editing page accessable via the "Fields" tab, however, there is no need to add this asap.

            Show
            Robert Allerstorfer added a comment - Now that the MDL-14788 bugfix has been committed and verified, this patch should get committed asap, too. The currently proposed patch does not change any current functionality, so its completely harmless to commit it. The big advantage is that users can start to use the "hidden" feature by importing a well-cared "preset.zip" containing a "preset.xml" with manually added <param1></param1> tags in number fields. After this patch has been committed, a GUI should be added to the "Number field" editing page accessable via the "Fields" tab, however, there is no need to add this asap.
            Hide
            Robert Allerstorfer added a comment -

            The backend code has now been committed, so people can start using this feature via presets. A new issue should be created regarding the frontend code (addition to the GUI).

            Show
            Robert Allerstorfer added a comment - The backend code has now been committed, so people can start using this feature via presets. A new issue should be created regarding the frontend code (addition to the GUI).
            Hide
            Petr Skoda added a comment - - edited

            hi,

            while reviewing the commit I noticed you are using number_float()
            we already have format_float() and unformat_float() - I guess those should be used instead

            also
            $decimals = intval($this->field->param1);
            if (isset($decimals) && is_int($decimals) && $decimals >= 0) {

            might not be what you wanted because it is always set and always integer, right?

            Show
            Petr Skoda added a comment - - edited hi, while reviewing the commit I noticed you are using number_float() we already have format_float() and unformat_float() - I guess those should be used instead also $decimals = intval($this->field->param1); if (isset($decimals) && is_int($decimals) && $decimals >= 0) { might not be what you wanted because it is always set and always integer, right?
            Hide
            Robert Allerstorfer added a comment -

            Hi Petr,

            (1) No, I don't use number_float(). The function I am using to format the number is number_format() - a standard PHP function (http://php.net/manual/en/function.number-format.php).

            (2) Yes, you are right - my integer checking was not useful - the attached patch (MDL-14771_integer-check.patch) corrects this. I have tested it successfully with many different values set in param1, and of course with nothing set in param1 at all.

            Please review again, thanks.

            Show
            Robert Allerstorfer added a comment - Hi Petr, (1) No, I don't use number_float(). The function I am using to format the number is number_format() - a standard PHP function ( http://php.net/manual/en/function.number-format.php ). (2) Yes, you are right - my integer checking was not useful - the attached patch ( MDL-14771 _integer-check.patch) corrects this. I have tested it successfully with many different values set in param1, and of course with nothing set in param1 at all. Please review again, thanks.
            Hide
            Petr Skoda added a comment -

            oops, sorry for the wrong function name - all new code in 1.9.x should be using xxx_float() for float processing, in 2.0 it will be mandatory because we need to support languages that use "floating comma" numbers

            Show
            Petr Skoda added a comment - oops, sorry for the wrong function name - all new code in 1.9.x should be using xxx_float() for float processing, in 2.0 it will be mandatory because we need to support languages that use "floating comma" numbers
            Hide
            Petr Skoda added a comment -

            1/ ctype_digit not allowed, extension might not be present
            2/ when dealing with decimal separators, it must be done both before input, after input and before display

            Show
            Petr Skoda added a comment - 1/ ctype_digit not allowed, extension might not be present 2/ when dealing with decimal separators, it must be done both before input, after input and before display
            Hide
            Robert Allerstorfer added a comment -

            I do not do float processing, so I don't understand why you mention that here.

            Moodle 1.9 requires PHP 4.3.0 or later, and beginning with that version ctype support is built in into PHP's core (see http://php.net/manual/en/ctype.installation.php). Thus, the function ctype_digit() which I use to check the value of param1 should always be available.

            What relationship has your last comment (regarding decimal separators) to the proposed patch?

            Show
            Robert Allerstorfer added a comment - I do not do float processing, so I don't understand why you mention that here. Moodle 1.9 requires PHP 4.3.0 or later, and beginning with that version ctype support is built in into PHP's core (see http://php.net/manual/en/ctype.installation.php ). Thus, the function ctype_digit() which I use to check the value of param1 should always be available. What relationship has your last comment (regarding decimal separators) to the proposed patch?
            Hide
            Petr Skoda added a comment -

            1/ ok those are two different problems, but anyway do not use number_format() directly here
            2/ ctype function caused problems on some servers (MDL-3381), which means it can not be used in 1.9.x - sorry

            Show
            Petr Skoda added a comment - 1/ ok those are two different problems, but anyway do not use number_format() directly here 2/ ctype function caused problems on some servers ( MDL-3381 ), which means it can not be used in 1.9.x - sorry
            Hide
            Robert Allerstorfer added a comment -

            OK, the new patch ("MDL-14771_integer-check-preg_match.patch") does not use a ctype function. Instead of ctype_digit() I am now using preg_match(). I prefer using the allmighty PCREs anyway

            However, PCRE support can be disabled in PHP versions < 5.3. PCREs are used in many Moodle files, but the environment check does not mention it as being required (nor as recommended). Should we file a bug on this?

            It seems that now the only thing you don't like in my proposed patch is the number_format() PHP core function. What is bad with it?

            Show
            Robert Allerstorfer added a comment - OK, the new patch (" MDL-14771 _integer-check-preg_match.patch") does not use a ctype function. Instead of ctype_digit() I am now using preg_match(). I prefer using the allmighty PCREs anyway However, PCRE support can be disabled in PHP versions < 5.3. PCREs are used in many Moodle files, but the environment check does not mention it as being required (nor as recommended). Should we file a bug on this? It seems that now the only thing you don't like in my proposed patch is the number_format() PHP core function. What is bad with it?
            Hide
            Petr Skoda added a comment -

            1/ PCRE - yes please file it

            2/ I thought you would figure that out yourself.

            Show
            Petr Skoda added a comment - 1/ PCRE - yes please file it 2/ I thought you would figure that out yourself.
            Hide
            Robert Allerstorfer added a comment -

            ad 1: bug filed as MDL-15057.
            ad 2: number_format() is used in 17 other Moodle core files. The function works fine - if you have a reason to forbid it in Moodle code, it should be removed from the existing code as well. No idea what should be wrong with it.

            Show
            Robert Allerstorfer added a comment - ad 1: bug filed as MDL-15057 . ad 2: number_format() is used in 17 other Moodle core files. The function works fine - if you have a reason to forbid it in Moodle code, it should be removed from the existing code as well. No idea what should be wrong with it.
            Hide
            Petr Skoda added a comment - - edited

            please read: "all new code in 1.9.x should be using xxx_float() ...." above (xxx_float() means format_float() or unformat_float())

            Show
            Petr Skoda added a comment - - edited please read: "all new code in 1.9.x should be using xxx_float() ...." above (xxx_float() means format_float() or unformat_float())
            Hide
            Petr Skoda added a comment -

            those 17 hits are either from code that was not converted yet (quiz and lesson) or imported libraries (code from adodb and typo3 not used, the graphlib should be revisited in 2.0)
            I spent some time getting rid of these and it annoys me a bit when you keep trying to push it into cvs.

            I guess it is partly my fault because I did not add it to coding style, fixed now - see http://docs.moodle.org/en/Development:Coding#General_rules

            Thanks for working on this!

            Show
            Petr Skoda added a comment - those 17 hits are either from code that was not converted yet (quiz and lesson) or imported libraries (code from adodb and typo3 not used, the graphlib should be revisited in 2.0) I spent some time getting rid of these and it annoys me a bit when you keep trying to push it into cvs. I guess it is partly my fault because I did not add it to coding style, fixed now - see http://docs.moodle.org/en/Development:Coding#General_rules Thanks for working on this!
            Hide
            Robert Allerstorfer added a comment -

            OK, now I understand what you have meant. Is the new pacth (MDL-14771_20080530.patch) now OK for you?

            Show
            Robert Allerstorfer added a comment - OK, now I understand what you have meant. Is the new pacth ( MDL-14771 _20080530.patch) now OK for you?
            Hide
            Petr Skoda added a comment -

            ok for me

            Show
            Petr Skoda added a comment - ok for me
            Hide
            Robert Allerstorfer added a comment -

            Thanks for reviewing!

            Show
            Robert Allerstorfer added a comment - Thanks for reviewing!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: