User:Werdna/Multilingual LiquidThreads Feedback

Here is my feedback for the Multilingual LiquidThreads code:

General Comments

It's obvious that there are a number of significant authors, and unfortunately their editors are configured with different indentation options. Some lines are indented with a tab, some lines are indented with two spaces, and most lines are indented with some combination of the two. MediaWiki code is indented with one tab per indentation level. I've fixed up some of the indentation in my local working copy, and I can commit the changes if you like.
Some of your code is commented out. I'd prefer that either an explanatory comment is left about what the code does and why it's commented out, or it's removed entirely. This will avoid some confusion.

`MetaTranslator.php`

> This file is no longer used. Thank you for your comments.

`canAttach`

	$comp1 = @preg_replace( $pattern, '', $escape );
	$comp2 = @preg_replace( $pattern, '', $description );

You shouldn't use @ to suppress warnings. Instead, write:

	wfSuppressWarnings();
	$comp1 = preg_replace( $pattern, '', $escape );
	$comp2 = preg_replace( $pattern, '', $description );
	wfRestoreWarnings();

See Manual:Coding_conventions#PHP_pitfalls

`attachDescriptions`

          $this->noTranslations[$i]['escape'] .= '(' . $description . ')';

I think you shouldn't have these parentheses hardcoded, and instead use an interface message to configure how the untranslated text appears. For starters, English usage rules would demand a space before the (.

Strip markers

I see that you're using strip markers to protect parts of the input from translation. However, I don't think that your strip markers are distinctive enough to avoid collisions with user input. You should look at the strip-marker system used in MediaWiki's parser (see Parser::insertStripItem() in includes/parser/Parser.php. In particular, the MediaWiki strip markers use a non-ASCII control character:

	const MARKER_SUFFIX = "-QINU\x7f";
		$this->mUniqPrefix = "\x7fUNIQ" . self::getRandomString();

Parse performance

In a lot of places you're looping over the text one character at a time to replace balanced delimiters. I'd recommend following the example of StringUtils::delimiterReplaceCallback (defined in includes/StringUtils.php), and using strcspn or preg_match to find the next delimiter.

`MultilangLqtHooks.php`

`saveReply`

		$info['replyTo'] = $replyTo;

PHP objects are accessed by reference, this line is unnecessary.

`onThreadCommands`

		$langridAccessObject = new LanguageGridAccessObject();
		if ( !$langridAccessObject->needsNoTranslation( $thread->id() ) ) {
			$translatedRoot = LanguageGridLqtRootManager::getTranslatedRootByThread( $thread, $langridAccessObject );
			if ( isset( $translatedRoot ) ) {
				$thread->setRoot( $translatedRoot );
			}
		}

This is an odd place to put this logic. I'm also a little leery about using setRoot() for this purpose, because it will end up changing the root in the database. Maybe we should add a new "displayRoot" property for each thread, which isn't persisted to the database but is used for display purposes only.

> Thank you for your comments. We agree with your suggestion about new property for Thread. Could you modify LQT code to add "displayRoot" property to Thread?

		$translated_history_url = LqtView::permalinkUrlWithQuery( $thread, array( 'action' => 'history' ) );
		$commands['history'] = array( 'label' => wfMsgExt( 'history_short', 'parseinline' ),
						 'href' => $translated_history_url,
						 'enabled' => true );

I suppose this is here because you've only just changed the root and the old history URL will be wrong. It should go when I implement what I've described above.

`onThreadPermalinkView`

The variable name $threadParmalinkView is misspelled.

`translateSubject`

			$html .= ' <font color="#f00">(Translation Failed)</font> ';

This should use an interface message, so that it appears in a user's language.

`translateSubjectforTOC`

Again, I'm leery of using setSubject to achieve your aims. We'd be better off storing a "display subject" like we would with setRoot, or moving the hook closer to display so you can display the subject instead.

> Thank you for your comment. We think it is better way to add a "display subject" like property to Thread because subjects is not only displayed into TOC but also each thread. Could you modify LQT code? Please tell us if we have mistaken ideas.

`onThreadMajorCommands`

				$sourceUrl = LqtView::talkpageUrl( $thread->title() , 'source', $thread,
						true /* include fragment */, true );

$thread->title() should be $thread->article()->getTitle(). You want to refer to the talkpage, not to the thread's permalink. Consider using LqtView::threadInContextLink(). Its signature is

static function linkInContext( $thread, $contextType = 'page', $text = null ) {

.

`onTalkpageMain`

		if ( is_nulL( $postlang ) ) {

is_nulL –> is_null

More generally, I'm not sure what this is supposed to do. I'd like it if there were an appropriate comment telling me what the function of that method was.

`showLanguageSelector`

	static function showLanguageSelector( &$e, $t ) {

It's weird to use the same function for two hooks. Things would be much clearer if you just had two hooks that called the same function on the backend.
Code is much more readable when you use proper variable names that aren't single characters. I know LiquidThreads does it in some places — mostly from the previous author.

		$lang_select_html .= Xml::openElement( 'select', array( 'name' => 'wpPostlang', 'id' => 'wpPostlang', 'style' => $select ) );

$select is not set.

		// $langridAccessObject = new LanguageGridAccessObject();
		// $postlang = $langridAccessObject->getTargetLanguage();

You should probably either explain why this is commented out, or remove it entirely.

		$title = $wgTitle->getDBkey();
		$res = $client->getSupportedTranslationLanguagePairs( $title );

		if ( strtoupper( $res['status'] ) == 'ERROR' ) {
			$title = $t->getTitle();
			$res = $client->getSupportedTranslationLanguagePairs( $title );
		}

You're sending a string to getSupportedTranslationLanguagePairs in one case, and a Title in another case. One of these is wrong.
Using $wgTitle is not recommended. You should figure out the title from context (in this case, the thread or talkpage parameter). It will fail if this method is called from the API (as it is in the current iteration of LiquidThreads).

`LanguageGridAccessObject.php`

`getTargetLanguage`

The data flow of your code would be more obvious if, instead of using globals, you actually passed the relevant information down the call chain. This would mean that needsTranslation could be deprecated.