r75769 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r75768‎ | r75769 | r75770 >
Date:00:14, 1 November 2010
Author:hartman
Status:reverted (Comments)
Tags:
Comment:
Avoid unnecessary linebreaks in difflines. Fixes bug 25725
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/diff/DifferenceEngine.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/diff/DifferenceEngine.php
——@@ -1045,7 +1045,7 @@
10461046
10471047 function getLines() {
10481048 $this->_flushLine('~done');
1049 - return $this->_lines;
 1049+ return preg_replace( '/\n/m', '', $this->_lines);
10501050 }
10511051 }
10521052
Index: trunk/phase3/RELEASE-NOTES
——@@ -380,6 +380,7 @@
381381 * (bug 25642) A exception is now thrown instead of a fatal error when using
382382 $wgSMTP without PEAR mail package
383383 * (bug 19633) When possible, Upscale small SVGs when creating thumbnails.
 384+* (bug 25725) Unwanted linebreaks in diffs.
384385
385386 === API changes in 1.17 ===
386387 * (bug 22738) Allow filtering by action type on query=logevent.

Follow-up revisions

RevisionCommit summaryAuthorDate
r80475Use str_replace instead of preg_replace. Follow up of r75769hartman23:16, 17 January 2011
r80620Revert r75769, r80475: mistaken attempt to fix bug 25725 by deleting random n...tstarling09:37, 20 January 2011
r80621Fix for bug 25725: don't insert any whitespace inside the divs, so that "whit...tstarling09:53, 20 January 2011

Comments

#Comment by Platonides (talk | contribs)   15:45, 5 December 2010

Does this need to be ported to WikiDiff?

#Comment by MarkAHershberger (talk | contribs)   19:42, 11 January 2011

marking ok since it is trivial and fixes the problem

#Comment by Platonides (talk | contribs)   00:26, 12 January 2011

mah, you may not be aware that there is a wikidiff2 extension written in C++ whose purpose is to do the same thing as the internal differ (and AFAIK is actually what runs in the cluster).

So I would still like to know if this change:

  1. Isn't needed in wikidiff2 (ok)
  2. Does require a missing commit to wikidiff2 (fixme)
  3. wikidiff2 wasn't looked at (fixme)
#Comment by MarkAHershberger (talk | contribs)   00:57, 12 January 2011

So, someone who is familiar with the infrastructure should be reviewing this. I'll assign to catrope and he can take it from there.

#Comment by Catrope (talk | contribs)   19:08, 12 January 2011

The least you could do here is use strlen() or strtr() instead of using a (multiline!) regex for such a trivial replacement.

As to the wikidiff2 question: no idea, passing the buck to Tim.

#Comment by TheDJ (talk | contribs)   13:35, 13 January 2011

It is a multiline problem however. (the words are divided over multiple lines, in order to reuse the 'perline' diffcode on words).

#Comment by Catrope (talk | contribs)   13:38, 13 January 2011

Right, you only want to clear lines that only consist of newlines, or something like that, you don't want to eradicate all newlines.

#Comment by Platonides (talk | contribs)   23:19, 13 January 2011

However, this preg_replace is removing all newlines.

Although it may be removing the new line in an already splitted line, doing a manual rtrim(, "\n")

#Comment by TheDJ (talk | contribs)   23:24, 13 January 2011

The intent is to remove all new lines. 

It
turns
this
back
into

It turns this back into
#Comment by Catrope (talk | contribs)   09:46, 14 January 2011

Then just use str_replace("\n", "", $foo)

#Comment by TheDJ (talk | contribs)   23:17, 17 January 2011

Done in r80475

#Comment by Tim Starling (talk | contribs)   09:32, 20 January 2011

Bug 25725 is not reproducible with the PHP diff engine, it affects wikidiff2 exclusively. This patch doesn't do anything useful, so I'm going to revert it.

The output from the PHP diff engine does not contain any lines breaks inside the divs selected by the CSS which the reporter uses. It does have line breaks in between the table rows, but those line breaks are there to improve the readability of the HTML source and do not affect formatting.

wikidiff2 similarly has whitespace added in between the table rows and cells, to improve HTML readability, but it apparently also has line breaks added inside the divs, which is problematic.

Note that the regex /\n/m is exactly equivalent to /\n/. The /m modifier only affects ^ and $.

Status & tagging log