Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40109

Double space between dates when showing the section name using format_weeks

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              poltawski 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
              poltawski 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
              dmonllao 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
              dmonllao 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
              poltawski 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
              poltawski 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
              dmonllao 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
              dmonllao 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
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              dmonllao 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
              dmonllao 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
              samhemelryk Sam Hemelryk added a comment -

              Thanks David, this has been integrated now

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks David, this has been integrated now
              Hide
              dmonllao 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
              dmonllao 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
              samhemelryk Sam Hemelryk added a comment -

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

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

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

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

              I have tested it in several languages too, works fine

              Show
              skodak Petr Skoda added a comment - I have tested it in several languages too, works fine
              Hide
              poltawski 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
              poltawski 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:
                    Fix Release Date:
                    8/Jul/13