Moodle
  1. Moodle
  2. MDL-19077 Implement the theme engines proposal
  3. MDL-19756

convert the remaining weblib output functions to $OUTPUT methods

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      35388
    1. confirm_dialog.patch
      2 kB
      Dongsheng Cai
    2. outputlib.patch
      125 kB
      Nicolas Connault

      Issue Links

        Activity

        Hide
        Nicolas Connault added a comment -

        Attached a patch with the conversion of the helpbutton() function.

        Show
        Nicolas Connault added a comment - Attached a patch with the conversion of the helpbutton() function.
        Hide
        Nicolas Connault added a comment -

        Updated patch

        Show
        Nicolas Connault added a comment - Updated patch
        Hide
        Tim Hunt added a comment -

        Should we allow $linktext to be true/false/"Some text" so you can have different visible text and tooltip. Hmm. or is it better to have no tooltip when there is visible text?

        We should clean up $tooltip = get_string... to not concatenate strings.

        Does the code become cleaner if we use moodle_url?

        Show
        Tim Hunt added a comment - Should we allow $linktext to be true/false/"Some text" so you can have different visible text and tooltip. Hmm. or is it better to have no tooltip when there is visible text? We should clean up $tooltip = get_string... to not concatenate strings. Does the code become cleaner if we use moodle_url?
        Hide
        Nicolas Connault added a comment -

        Attached a patch with migration of *_to_popup_window() functions.

        Show
        Nicolas Connault added a comment - Attached a patch with migration of *_to_popup_window() functions.
        Hide
        Nicolas Connault added a comment -

        Updated the popup patch

        Show
        Nicolas Connault added a comment - Updated the popup patch
        Hide
        Nicolas Connault added a comment -

        Just attached a general patch that includes all my changes.

        Show
        Nicolas Connault added a comment - Just attached a general patch that includes all my changes.
        Hide
        Nicolas Connault added a comment -

        Attached another more recent patch. Be mindful that some of the API details are still under consideration, particularly the abstraction details of action_icon and help_icon.

        Show
        Nicolas Connault added a comment - Attached another more recent patch. Be mindful that some of the API details are still under consideration, particularly the abstraction details of action_icon and help_icon.
        Hide
        Nicolas Connault added a comment -

        Finished refactoring helpbutton

        Show
        Nicolas Connault added a comment - Finished refactoring helpbutton
        Hide
        Tim Hunt added a comment -

        More review comments:

        • You've got a bit of trailing whitespace in deprecatedlib.php
        • I am not going to criticise duplicated code in deprecatedlib.
        • I don't like html_spacer::br. Surely the only place we want that to be used is in deprecated print_spacer, so don't add it to the UI. Just do
          if ($br) {
              $output .= '<br />';
          }
          

          In the deprecated function for backwards compatibility.

        • print_textarea has an incomplete comment "* When using this function, you should"
        • Actually, is print_textarea finished? I assume not.
        • Looking at prepare_event_handlers:
        • I don't think this method should be on moodle_core_renderer anyway.
        • moodle_core_renderer::link does not do id parameters, even though a html_component may have one.
        • moodle_core_renderer::button still using the onclick attribute?
        • I think prepare_url should be in weblib.php, next to the moodle_url class.
        • prepare_url has trailing whitespace on some lines.
        • Why does prepare_url do
                  // Clean params
                  foreach ($url->params() as $var => $val) {
                      $url->param($var, s($val));
                  }
          

          moodle_url already escapes ouptut correctly, and anyway that code has not effect.

        • prepare_url needs unit tests!
        Show
        Tim Hunt added a comment - More review comments: You've got a bit of trailing whitespace in deprecatedlib.php I am not going to criticise duplicated code in deprecatedlib. I don't like html_spacer::br. Surely the only place we want that to be used is in deprecated print_spacer, so don't add it to the UI. Just do if ($br) { $output .= '<br />'; } In the deprecated function for backwards compatibility. print_textarea has an incomplete comment "* When using this function, you should" Actually, is print_textarea finished? I assume not. Looking at prepare_event_handlers: YAHOO.util.Event.preventDefault does not work like that! You need to call it in the JavaScript code, passing the event object that was passed to the even handler. An example of the correct usage is in http://cvs.moodle.org/moodle/mod/quiz/quiz.js?view=markup I don't think this method should be on moodle_core_renderer anyway. moodle_core_renderer::link does not do id parameters, even though a html_component may have one. moodle_core_renderer::button still using the onclick attribute? I think prepare_url should be in weblib.php, next to the moodle_url class. prepare_url has trailing whitespace on some lines. Why does prepare_url do // Clean params foreach ($url->params() as $ var => $val) { $url->param($ var , s($val)); } moodle_url already escapes ouptut correctly, and anyway that code has not effect. prepare_url needs unit tests!
        Hide
        Tim Hunt added a comment -

        Lots more comments.

        • In several places you have incomplete PHP docs like "* @return string".
          Elsewhere I have said "* @return string the HTML to be output."
        • action_icon should not call $link->prepare(); or $image->prepare();. $this->image and $this->link do that.
        • The previous comment applies in lots of other places too. I think you need to review all ->prepare()
        • link_to_popup has PHP doc that talks about button.
        • link_to_popup should use prepare_url rather than its own code.
        • The way you have laid out arrays like $tagoptions = array( does not match the coding guidelines.
        • Since we already have JS calling openpopup, we don't need the "this.target='$popup->name'; "; bit. (In several places.)
        • I don't think $popup->image makes sense.
        • I hate the excessive whitespace indent on comments like (user_picture)
               * @param object       $userpic Object with at least fields id, picture, imagealt, firstname, lastname
               *                               If any of these are missing, or if a userid is passed, the database is queried. Avoid this
          
        • I don't think the Note: on user_picture is clear. Instead I would write

        This method can be used in two ways:
        <pre>
        // Option 1:
        $userpic = new user_picture();
        // Set properties of $userpic
        $OUTPUT->user_picture($userpic);

        // Option 2: (shortcut for simple cases)
        // $user has come from the DB and has fields id, picture, imagealt, firstname and lastname
        $OUTPUT->user_picture($user, $COURSE->id);
        </pre>

        • I don't think user_picture needs to clone clone($userpic). (But I might be wrong)
        • I don't see any benefit in adding class="link" to every link. Just bloat.
        • Should help_icon::prepare complain if the user tries to set ->url manually?
        • In html_action_component. I think we need to just choose one API, $jsfunction/$jsfunctionargs or $onclick. Giving a choice is just confusing.
        • Why isn't html_action_component a html_action_component?
        • In html_image, why not just do public $alt = HTML_ATTR_EMPTY; makes prepare() smaller.
        • In html_image::prepare,
          if (empty($this->title)) {
              $this->title = $this->alt;
          }
          

          is jsut wrong. There are many cases when an image should have alt text, but no tooltip.

        • user_picture needs PHPdoc comments. In particualr I don't understand why we need both $link and $url. (You could allow $link to be true (for default link), flase for no link or a url.

        Do not take the number of comments as an indication that this patch is bad. Quite the contrary, it is close, but not quite there yet.

        Show
        Tim Hunt added a comment - Lots more comments. In several places you have incomplete PHP docs like "* @return string". Elsewhere I have said "* @return string the HTML to be output." action_icon should not call $link->prepare(); or $image->prepare();. $this->image and $this->link do that. The previous comment applies in lots of other places too. I think you need to review all ->prepare() link_to_popup has PHP doc that talks about button. link_to_popup should use prepare_url rather than its own code. The way you have laid out arrays like $tagoptions = array( does not match the coding guidelines. Since we already have JS calling openpopup, we don't need the "this.target='$popup->name'; "; bit. (In several places.) I don't think $popup->image makes sense. I hate the excessive whitespace indent on comments like (user_picture) * @param object $userpic Object with at least fields id, picture, imagealt, firstname, lastname * If any of these are missing, or if a userid is passed, the database is queried. Avoid this I don't think the Note: on user_picture is clear. Instead I would write This method can be used in two ways: <pre> // Option 1: $userpic = new user_picture(); // Set properties of $userpic $OUTPUT->user_picture($userpic); // Option 2: (shortcut for simple cases) // $user has come from the DB and has fields id, picture, imagealt, firstname and lastname $OUTPUT->user_picture($user, $COURSE->id); </pre> I don't think user_picture needs to clone clone($userpic). (But I might be wrong) I don't see any benefit in adding class="link" to every link. Just bloat. Should help_icon::prepare complain if the user tries to set ->url manually? In html_action_component. I think we need to just choose one API, $jsfunction/$jsfunctionargs or $onclick. Giving a choice is just confusing. Why isn't html_action_component a html_action_component? In html_image, why not just do public $alt = HTML_ATTR_EMPTY; makes prepare() smaller. In html_image::prepare, if (empty($ this ->title)) { $ this ->title = $ this ->alt; } is jsut wrong. There are many cases when an image should have alt text, but no tooltip. user_picture needs PHPdoc comments. In particualr I don't understand why we need both $link and $url. (You could allow $link to be true (for default link), flase for no link or a url. Do not take the number of comments as an indication that this patch is bad. Quite the contrary, it is close, but not quite there yet.
        Hide
        Nicolas Connault added a comment -

        Finished fixing all of the issues reported by Tim.
        Also added YUI tooltips over helpbuttons.

        Show
        Nicolas Connault added a comment - Finished fixing all of the issues reported by Tim. Also added YUI tooltips over helpbuttons.
        Hide
        Nicolas Connault added a comment -

        Attached a patch for only weblib.php, outputlib.php and deprecatedlib.php

        Show
        Nicolas Connault added a comment - Attached a patch for only weblib.php, outputlib.php and deprecatedlib.php
        Hide
        Tim Hunt added a comment -

        More review comments.

        • No HTML in lang strings. Also, playing around, I think it works better if you do
          $string['clickhelpiconformoreinfo'] = '... continues ... Click the help icon to read the full article.';
              echo shorten_text($output, 400, false, '<span class="readmore">' . get_string('clickhelpiconformoreinfo') . '</span>');
          

          and a rule in the theme for .readmore to be italic.

        • Help tooltip should have a 'loading' animated gif while the content is loading.
        • $this->page->requires->yui_lib('container'); should not be in header, and $this->page->requires->js_function_call('init_help_icons'); should not be in footer. Instead they should both be in
        • prepare_event_handlers needs to be fixed as above ($this->page->requires->yui_lib('event'); is already in required_event_handler, and YAHOO.util.Event.preventDefault does not work like that.)
        • Set a default of null for $link->title etc. so don't need to check for if (!empty($link->title)) { everywhere.
        • $button->prepare(); should set $button->disabled to '' or 'disabled', so
          if ($button->disabled) {
              $buttonattributes['disabled'] = 'disabled';
          }
          

          can be just
          $buttonattributes['disabled'] = $button->disabled;

        • the logic
          if (!empty($button->title) && !empty($popup->title)) {
              $buttonattributes['title'] = $popup->title;
          }
          

          looks wrong.

        • I don't think $linktext should be a separate parameter to action_icon. Should be a property of the class
        • In link_to_popup, the code
          if (empty($popup->url)) {
              $popup->url = $link->url;
          }
          

          is duplicated.

        • spacer has incorrect PHPdocs ( with optional line break.)
        • I don't think html_action_component is-a moodle_html_component It is more like a moodle_url, that is, a separate class that is sometimes used as an attribute of a moodle_html_component
        Show
        Tim Hunt added a comment - More review comments. No HTML in lang strings. Also, playing around, I think it works better if you do $string['clickhelpiconformoreinfo'] = '... continues ... Click the help icon to read the full article.'; echo shorten_text($output, 400, false , '<span class= "readmore" >' . get_string('clickhelpiconformoreinfo') . '</span>'); and a rule in the theme for .readmore to be italic. Help tooltip should have a 'loading' animated gif while the content is loading. $this->page->requires->yui_lib('container'); should not be in header, and $this->page->requires->js_function_call('init_help_icons'); should not be in footer. Instead they should both be in prepare_event_handlers needs to be fixed as above ($this->page->requires->yui_lib('event'); is already in required_event_handler, and YAHOO.util.Event.preventDefault does not work like that.) Set a default of null for $link->title etc. so don't need to check for if (!empty($link->title)) { everywhere. $button->prepare(); should set $button->disabled to '' or 'disabled', so if ($button->disabled) { $buttonattributes['disabled'] = 'disabled'; } can be just $buttonattributes ['disabled'] = $button->disabled; the logic if (!empty($button->title) && !empty($popup->title)) { $buttonattributes['title'] = $popup->title; } looks wrong. I don't think $linktext should be a separate parameter to action_icon. Should be a property of the class In link_to_popup, the code if (empty($popup->url)) { $popup->url = $link->url; } is duplicated. spacer has incorrect PHPdocs ( with optional line break.) I don't think html_action_component is-a moodle_html_component It is more like a moodle_url, that is, a separate class that is sometimes used as an attribute of a moodle_html_component
        Hide
        Nicolas Connault added a comment -

        Fixed all reported issues, Tim, plus did some more refactoring in various places.

        Show
        Nicolas Connault added a comment - Fixed all reported issues, Tim, plus did some more refactoring in various places.
        Hide
        Nicolas Connault added a comment -

        Deprecated print_paging_bar and notice_yesno, patch updated

        Show
        Nicolas Connault added a comment - Deprecated print_paging_bar and notice_yesno, patch updated
        Hide
        Dongsheng Cai added a comment -

        Hi, Nicolas, I made a patch to make confirm_dialog support callback function, can you please review it?

        Thanks

        Show
        Dongsheng Cai added a comment - Hi, Nicolas, I made a patch to make confirm_dialog support callback function, can you please review it? Thanks
        Hide
        Jordi Piguillem Poch added a comment -

        Hi,
        method textarea is still unimplemented. Will it be added to the output renderer?

        Thanks
        Pigui.

        Show
        Jordi Piguillem Poch added a comment - Hi, method textarea is still unimplemented. Will it be added to the output renderer? Thanks Pigui.
        Hide
        Tim Hunt added a comment -

        Petr dealt with this in his theme changes.

        Show
        Tim Hunt added a comment - Petr dealt with this in his theme changes.
        Hide
        Martin Dougiamas added a comment -

        Where was textarea dealt with? I cant find a replacement function in $OUTPUT. I'm removing the debugging notice in print_textarea until we have one.

        Show
        Martin Dougiamas added a comment - Where was textarea dealt with? I cant find a replacement function in $OUTPUT. I'm removing the debugging notice in print_textarea until we have one.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: