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

Does the code become cleaner if we use moodle_url?

• 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!

• 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.

• 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 {code}
  if ($button->disabled) { $buttonattributes['disabled'] = 'disabled'; } {code}
  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

Thanks

Thanks
Pigui.