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

      Gliffy Diagrams

      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: