Last modified: 2012-01-03 10:55:27 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T27697, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 25697 - Diff: Empty context lines not showing
Diff: Empty context lines not showing
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
wikidiff2 (Other open bugs)
unspecified
All All
: High normal with 1 vote (vote)
: ---
Assigned To: Nobody - You can work on this!
http://en.wikipedia.org/w/index.php?t...
: easy, patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-28 20:05 UTC by Erwin Dokter
Modified: 2012-01-03 10:55 UTC (History)
9 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch for wikidiff.cpp (643 bytes, patch)
2011-07-06 12:31 UTC, Erwin Dokter
Details
Patch for wikidiff2.cpp (689 bytes, patch)
2011-07-06 12:31 UTC, Erwin Dokter
Details

Description Erwin Dokter 2010-10-28 20:05:03 UTC
When viewing a diff that contains empty context lines (gray), those lines are not shown. This is due to all cells in that row having no content, which reverts the row height to zero. It used to show a thin gray line, but a change in diff.css (table.diff td {padding: 0px;}) made that disappear as well. Example in URL should show an empty line directly above "==Professional acting career==".

Having the following CSS in your own stylesheet restores the empty line:
td.diff-marker {height: 1.5em;}

However, that is a hack; A better solution is not to have rows with all empty cells. At least one cell needs to have content, and all rows start with a cell with a diff marker; these are either empty, or contain a + or - sign. I suggest putting a non-breaking space ( ) in the empty diff marker cells. This will cause the row to have content and force the proper height automatically.
Comment 1 Erwin Dokter 2010-10-28 20:17:50 UTC
( ) shoud be ( ).
Comment 2 Thana 2010-10-29 00:01:54 UTC
yuck. inserting meaningless invisible placeholder content is clearly
the hack. here you would be using brute force to prevent individual
sites and users from customizing the appearance of diff.

recommend immediate closure WONTFIX.
Comment 3 Thana 2010-10-29 00:31:49 UTC
specifying a reasonable default minimum td height in diff.css would be best, as
this can be over-ridden by anyone who wants to.

for example, see diff view highlighted by browser's "select all" command:
http://i55.tinypic.com/vpiyxd.png

spaces impart specific meaning as seen in the vicinity of the cursor, to wit:
the nbsp on line 652 implies i have vandalized your comment and replaced one
line of it by a single non-breaking space. this is different from replacing
it by a totally blank line.

the diff view should remain faithful to this distinction in all cases.
Comment 4 Derk-Jan Hartman 2010-10-29 11:11:12 UTC
Perhaps just a min-height: 1.5em; then ?
Comment 5 Erwin Dokter 2010-10-29 11:47:10 UTC
@Thana: Adding a non-visible character is a common way to force table cells to behave as intended. It prevents nothing in the way customizing its appearence, on the contrary.

Also, you misunderstand where I want to place the space; it should go in the first cell of the row, which is normally occupied by the plus- or minus sign (td.diff-marker), *not* in the cell containing the actual diff-text. So you would see that "unwanted" select-block above and below the minus sign in the most-left column in your example.

@Derk-Jan Hartman: The height is skin-specific, and should match each ckin, that means 1.5em for the modern (sans-serif based) skins and 1em for the classic (serif based) skins. So that is not an ideal solution. By giving the empty cell content, it would scale to the right size automatically for every skin.
Comment 6 Erwin Dokter 2010-10-29 11:57:36 UTC
Also, it doesn't have to be a   necessarely; it could also be another neutral character (in addition to the plus- and minus sign) denoting a no-change in that line.
Comment 7 Erwin Dokter 2010-10-29 12:29:11 UTC
I just found out that the unclassed cells (the white space after a deleted line or before a added line) contain this code: <td colspan="2">&nbsp;</td>
So adding a &nbsp; in empty diff-marker cells should not be that controverial.
Comment 8 Derk-Jan Hartman 2010-10-29 18:28:57 UTC
Done in r75658
Comment 9 Erwin Dokter 2010-10-29 20:34:32 UTC
Thank you! When is this this revision expected to go live? If it going to take a while, I can apply the other fix temporarely.
Comment 10 Bawolff (Brian Wolff) 2010-10-29 22:32:28 UTC
(In reply to comment #9)
> Thank you! When is this this revision expected to go live? If it going to take
> a while, I can apply the other fix temporarely.

I have no idea what the current status of code deployment is other than "way behind". However with that said the answer is, probably not going live for a little while (I'd guess a couple months, but these are just random guesses that don't have any bearing on reality).
Comment 11 Antoine "hashar" Musso (WMF) 2010-10-29 22:34:01 UTC
Given it looks like a small independant change, it might be merged and scaped live quickly :-)
Comment 12 Erwin Dokter 2010-10-30 00:10:08 UTC
How do I know when a particular revision is live?
Comment 13 Derk-Jan Hartman 2010-10-30 00:15:09 UTC
At the moment this is the deployment branch: http://svn.wikimedia.org/viewvc/mediawiki/branches/wmf/1.16wmf4/

Any code that is not there, is not deployed. I don't think this will be deployed before 1.17, so it's likely that even when it does get deployed, it will be on a 1.17wmf branch.
Comment 14 Erwin Dokter 2011-07-05 23:15:09 UTC
Since bug 25725 mentions that the PHP diff engine is no longer used, should this fix not be applied in /trunk/extensions/wikidiff2/wikidiff2.cpp instead?
Comment 15 Erwin Dokter 2011-07-06 12:31:28 UTC
Created attachment 8748 [details]
Patch for wikidiff.cpp
Comment 16 Erwin Dokter 2011-07-06 12:31:51 UTC
Created attachment 8749 [details]
Patch for wikidiff2.cpp
Comment 18 P.Copp 2011-07-27 13:04:59 UTC
(In reply to comment #17)
> Marking as fixed; see
> http://en.wikipedia.org/w/index.php?title=Matt_Smith_(actor)&diff=next&oldid=392393612

It works only because of a local fix, see <http://en.wikipedia.org/w/index.php?title=MediaWiki:Vector.css&diff=393682546&oldid=392862614>.

Wikidiff2 still needs to be adapted, so reopening and changing component.
Comment 19 Erwin Dokter 2011-12-31 16:49:25 UTC
Bumping this... no peep in five months. This kinda block bug 33335 (new diff color scheme). With this patch, diffs will finally be 'complete' and all local hacks can be removed once deployed.
Comment 20 Roan Kattouw 2012-01-03 10:55:27 UTC
Modified patch applied in r107875.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links