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:
    • Rank:
      50841

      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.

        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 Škoda added a comment -

          I have tested it in several languages too, works fine

          Show
          Petr Škoda 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: