r59655 MediaWiki - Code Review archive

Seen at translatewiki:

PHP Notice: Undefined variable: summaryClass in /var/www/w/includes/EditPage.php on line 1550

Please update /maintenance/language/messages.inc too

Fixed with r59672.

Seen at translatewiki:

PHP Notice: Undefined index: style in /var/www/w/includes/EditPage.php on line 1723

Fixed with r59695. Thanks.

How extensively was this tested prior to committing? I know we're trying to branch 1.16; a refactor might not be best now.

Tested preview functionality, tested edit conflict, tested extensibility functionality, tested sections, tested section edit not supported feature, etc... I should have all the php notices squashed now, I need to use more strict error settings more often. One error was hidden behind a user pref. The only real danger in this refactor would be unnoticed variables that were missed when code was moved into another function. Depending on what they are used for they should generate some kind of php error in that case, and I think I've caught all of those.

Fair enough. Still want another set of eyes, leaving this 'new' still.

The JavaScript is a bit broken.

-	var textbox = document.getElementById('wpTextbox1');
-	if (!textbox) { return false; }
-
 	// Don't generate buttons for browsers which don't fully
 	// support it.
+	var textbox = document.createElement('textarea'); // abstract, don't assume wpTextbox1 is always there

In FF 3.5 this causes:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLTextAreaElement.selectionStart]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: .../skins/common/edit.js?257 :: mwSetupToolbar :: line 51" data: no]

The script stops and the toolbar does not appear.

+			'onfocus' => "currentFocused = this;",

If you're going to make this a public interface for communicating between PHP and JS, fixed for all time and encouraged for use by extensions, you should think of a better way to do it than setting an unprefixed, ambiguously-named global variable.

+	protected function editFormHeadInit() {

Call this function "showHeader". Your name has the words in the wrong order, "edit form" is implied since that's what class we're in, and initialisation is not what it's doing, rather it's sending HTML to $wgOut and functions which do that usually start with "show". Also it needs a doc comment.

+		// activate checkboxes if user wants them to be always active
+		if ( !$this->preview && !$this->diff ) {
+			# Sort out the "watch" checkbox
+			if ( $wgUser->getOption( 'watchdefault' ) ) {
+				# Watch all edits
+				$this->watchthis = true;

Not vital since it was wrong in the original, but: I would suggest moving this section to an accessor function getWatchThis() instead of relying on initialisation order. Relying on things being initialised in the correct order is fragile, especially when there are subclasses involved. Same with $this->minoredit.

+	function getSummaryInput($summary = "", $labelText = null, $userInputAttrs = null, $userSpanLabelAttrs = null) {

Since the parameter names provide self-documentation, they should get the short, obvious names ($inputAttrs and $spanLabelAttrs) and the local variables should get the prefixed names (e.g. $defaultInputAttrs)

-		if ( $this->kblength === false ) {
+		if ( $this->kblength === false )

I prefer the former style for the reason explained at Manual:Coding conventions.

Otherwise looks good, definitely a useful refactor.

Ok, I'll find a better abstract way of doing the toolbar part.

Do you have another idea for the focused bit?

Originally I wanted to refactor edit.js to use a nice event delegating setup so that all text inputs would be available to the edit buttons. However I realized that focus events didn't bubble, so onfocus needed to be on each individual input.

I thought about some sort of register method, but something like that would cause problems for inputs that don't use id's. I thought about having js go through all the inputs and make them focusable. But that would cause trouble for dynamically generated inputs (a form adding another block of inputs).

So I guess I gave up and just used the global that edit.js was already using. I could use a method that would go through inputs and make them focusable, and later on if inputs were dynamically added the extension could call it again to update. But that might have to end up with a restriction that the onfocus attribute can't be used by extensions, depending on how good our current event system is.

However I ran into a quirksmode article with a quick search now: http://www.quirksmode.org/blog/archives/2008/04/delegating_the.html jQuery 1.4 will have support for it in .live, and a monkeypatch makes it available on .live in current code. I'll look into that so that the toolbar can work cleanly without any need to register edit form inputs.

I think having a class on the input elements which is converted to an onfocus handler by the DOM ready event would be a reasonable way to do it. JS that dynamically creates input boxes would have to call an update function, but I don't think that's unreasonable. Something like initEditToolbarInput(inputElement). Correct me if I'm wrong.

Instead of moving that watchthis stuff to a getWatchThis method, I moved the form related part of it into importFormData and the other parts to initialiseForm. That block was pretty much a bit of slight condition abuse. "If we're in the edit form (not save), not in preview, not in diff" which was really a bad way of checking for the initial form. Which is what initialiseForm's purpose is.

And I fixed that currentFocused use with some nice event delegation.