Moodle
  1. Moodle
  2. MDL-40109

Double space between dates when showing the section name using format_weeks

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.5, 2.6
    • Fix Version/s: 2.4.5, 2.5.1, FRONTEND
    • Component/s: Course
    • Labels:

      Description

      Discovered while working on MDL-39795, the double space is not visible as browsers merge them into a single one.

      get_string('strftimedateshort') includes a space before all date formats; attaching a patch removing the second space from the dates concat string.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Hi David,

            I'm not sure you mean strftimedateshort? In english that is:

            lang/en/langconfig.php:$string['strftimedateshort'] = '%d %B';
            

            Also could this be langpack dependent, thus should it use trim instead?

            Show
            Dan Poltawski added a comment - Hi David, I'm not sure you mean strftimedateshort? In english that is: lang/en/langconfig.php:$string['strftimedateshort'] = '%d %B'; Also could this be langpack dependent, thus should it use trim instead?
            Hide
            David Monllaó added a comment -

            Yes, I thought the space prefixing the language-dependant string was there for some reason so I didn't clean anything else.

            You mean something like this?

            $dateformat = trim(get_string('strftimedateshort'));
            $weekday = userdate($dates->start, $dateformat);
            $endweekday = userdate($dates->end, $dateformat);
            return ' ' . $weekday . ' - ' . $endweekday;
            

            Show
            David Monllaó added a comment - Yes, I thought the space prefixing the language-dependant string was there for some reason so I didn't clean anything else. You mean something like this? $dateformat = trim(get_string('strftimedateshort')); $weekday = userdate($dates->start, $dateformat); $endweekday = userdate($dates->end, $dateformat); return ' ' . $weekday . ' - ' . $endweekday;
            Hide
            Dan Poltawski added a comment -

            Well, is it the timeformat which is the problem? To me, it does not look like it strftimedateshort in lang/en/ does not include spaces?

            Show
            Dan Poltawski added a comment - Well, is it the timeformat which is the problem? To me, it does not look like it strftimedateshort in lang/en/ does not include spaces?
            Hide
            David Monllaó added a comment -

            The time format is ok, the problem with the current code is that $dateformat prefixes it with a space and the returned value is adding an extra space before $endweekday.

            Attaching current code:

            $dateformat = ' '.get_string('strftimedateshort');
            $weekday = userdate($dates->start, $dateformat);
            $endweekday = userdate($dates->end, $dateformat);
            return $weekday.' - '.$endweekday;
            

            Show
            David Monllaó added a comment - The time format is ok, the problem with the current code is that $dateformat prefixes it with a space and the returned value is adding an extra space before $endweekday. Attaching current code: $dateformat = ' '.get_string('strftimedateshort'); $weekday = userdate($dates->start, $dateformat); $endweekday = userdate($dates->end, $dateformat); return $weekday.' - '.$endweekday;
            Hide
            Dan Poltawski added a comment -

            So, since that is not being specified by the timeformat, is it actually OS dependent on whether it adds that space? The timeformat doesn't request it.

            Show
            Dan Poltawski added a comment - So, since that is not being specified by the timeformat, is it actually OS dependent on whether it adds that space? The timeformat doesn't request it.
            Hide
            Dan Poltawski added a comment -

            OK, after talking with david, i understand, I was missing this line adding a space!!

            $dateformat = ' '.get_string('strftimedateshort');
            

            Well, my +1 to remove that space, there is no need for a leading space and it makes the code cleaner to read if things are doing as they look like they should be.

            Show
            Dan Poltawski added a comment - OK, after talking with david, i understand, I was missing this line adding a space!! $dateformat = ' '.get_string('strftimedateshort'); Well, my +1 to remove that space, there is no need for a leading space and it makes the code cleaner to read if things are doing as they look like they should be.
            Hide
            David Monllaó added a comment -

            Yes, and even more after checking the history... https://github.com/moodle/moodle/blob/MOODLE_13_STABLE/course/format/weeks/format.php#L129, it seems an inherited space. Pull branches updated

            Show
            David Monllaó added a comment - Yes, and even more after checking the history... https://github.com/moodle/moodle/blob/MOODLE_13_STABLE/course/format/weeks/format.php#L129 , it seems an inherited space. Pull branches updated
            Hide
            Sam Hemelryk added a comment -

            Thanks David, this has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks David, this has been integrated now
            Hide
            David Monllaó added a comment -

            Sorry, I'm breaking a behat tests with this issue.

            Adding fixes on top of integration (for 25 and master)

            • git pull git://github.com/dmonllao/moodle.git MDL-40109_25-fix
            • git pull git://github.com/dmonllao/moodle.git MDL-40109_master-fix
            Show
            David Monllaó added a comment - Sorry, I'm breaking a behat tests with this issue. Adding fixes on top of integration (for 25 and master) git pull git://github.com/dmonllao/moodle.git MDL-40109 _25-fix git pull git://github.com/dmonllao/moodle.git MDL-40109 _master-fix
            Hide
            Sam Hemelryk added a comment -

            Thanks David - I've pulling in those two branches now.

            Show
            Sam Hemelryk added a comment - Thanks David - I've pulling in those two branches now.
            Hide
            David Monllaó added a comment -

            Thanks Sam, all behat tests passing with this last fix integrated and MDL-38555 reverted

            Show
            David Monllaó added a comment - Thanks Sam, all behat tests passing with this last fix integrated and MDL-38555 reverted
            Hide
            Petr Skoda added a comment -

            I have tested it in several languages too, works fine

            Show
            Petr Skoda added a comment - I have tested it in several languages too, works fine
            Hide
            Dan Poltawski added a comment -

            Thanks for your contributions!

            _main:
            @ BB#0:
                    push    {r7, lr}
                    mov     r7, sp
                    sub     sp, #4
                    movw    r0, :lower16:(L_.str-(LPC0_0+4))
                    movt    r0, :upper16:(L_.str-(LPC0_0+4))
            LPC0_0:
                    add     r0, pc
                    bl      _printf
                    movs    r1, #0
                    movt    r1, #0
                    str     r0, [sp]                @ 4-byte Spill
                    mov     r0, r1
                    add     sp, #4
                    pop     {r7, pc}
             
                    .section        __TEXT,__cstring,cstring_literals
            L_.str:                                 @ @.str
                    .asciz   "This code is now upstream!"
            

            Show
            Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4-byte Spill mov r0, r1 add sp, #4 pop {r7, pc}   .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: