Edgewall Software

Ticket #1420 (closed defect: fixed)

Opened 4 years ago

Last modified 20 months ago

long unwrappable lines in changeset are partially hidden

Reported by: cboos Owned by: mgood
Priority: normal Milestone: 0.10.4
Component: version control/changeset view Version: devel
Severity: minor Keywords: diff
Cc:

Description

While checking my recent [1505] commit, I've noticed that lines 131/140 are displayed incorrectly, in inline mode or side-by-side mode.

This is the long line:

-              r"(?P<modulehref>!?((?P<modulename>bug|ticket|browser|source|repos|report|query|changeset|wiki|milestone|search):(?P<moduleargs>(&#34;(.*?)&#34;|'(.*?)')|([^ ]*[^'~_\., \)]))))",
+              r"(?P<modulehref>!?((?P<modulename>%s):(?P<moduleargs>(&#34;(.*?)&#34;|'(.*?)')|([^ ]*[^'~_\., \)]))))" % _wiki_modules,

It affects both Firefox and Internet Explorer, on Windows.

Attachments

mob_db-diff.png (22.0 KB) - added by cboos 2 years ago.
Screenshot of an overlapping diff

Change History

  Changed 4 years ago by mgood

  • owner changed from jonas to mgood
  • status changed from new to assigned
  • summary changed from Long lines are not correctly wrapped up in diffs to long unwrappable lines in changeset are partially hidden
  • milestone set to 0.9

Line-breaking behavior is up to the browser. The problem is that the line here contains a very long string with no whitespace or other breakable characters. So, since there's no logical way to wrap the line, the content goes outside of the box. The CSS overflow properites seem to be set to hidden, so you can't see the end of the line.

We could probably display the overflow in this case since these kinds of unwrappable lines should be rare, and most changes will continue to display the same as they do now.

I can resolve this along with some of the related changes I'm making for #1397.

  Changed 3 years ago by cboos

  • status changed from assigned to closed
  • resolution set to fixed

This has been fixed in r1764

  Changed 2 years ago by cboos

  • status changed from closed to reopened
  • resolution fixed deleted
  • milestone changed from 0.9 to 0.10.1

Well, r1764 actually set overflow: hidden, which is not what mgood proposed and what I verified would solve the issue, at least for <ins>/<del> blocks.

Also note that for fixing #3966, we introduced a white-space: pre setting which makes matter worse for long lines, which are not wrapped anymore even if they contained white space.

So introducing the following change:

  • htdocs/css/diff.css

     
    108108.diff table tbody td { 
    109109 background: #fff; 
    110110 font: normal 11px monospace; 
    111  overflow: hidden; 
     111 overflow: visible; 
    112112 padding: 1px 2px; 
    113113 vertical-align: top; 
    114114} 

seems to be still in order.

  Changed 2 years ago by cboos

Illustration of the problematic side-effects of #3966: http://trac.edgewall.org/wiki/TracUsers?action=diff&version=313 (i.e. TracUsers@312:313)

  Changed 2 years ago by cboos

  • status changed from reopened to closed
  • resolution set to fixed

In r4156-4157, I think I found a good compromise:

  • full lines added and removed are wrapped; there it's not that important that whitespace is preserved, as there's no old vs. new to compare character for character, and also #3966 can't happen
  • intra line changes preserve space, and in this case, long lines are possibly overflowing the box; not that nice, but it's more important to be correct than ugly in this case, and as noted above, the overflow situation should not happen that often.

Ported to 0.10-stable in r4158.

  Changed 2 years ago by theultramage@…

This is a followup from #4070, since nothing was resolved due to misunderstanding and ignorance. So, once more:

I use trac v0.11 devel, HEAD revision, to view revisions stored in svk repository mirrors. One CSV file always views incorrectly - the lines don't wrap and get clipped (they're fairly long). I'm using tracd to generate the pages.

You can see how the file looks like (yes, the file view itself is affected, not just the changset view) on v0.11 devel by opening this url. And you can view how it looks like on v0.10 by using this url (not my server, I just use it as reference because mine isn't publicly available).

Before you object that it's an old version, I can say that the bug looks exactly the same on both versions. If this is purely a browser problem then sorry, but I tried multiple browsers and all display the same thing...

Changed 2 years ago by cboos

Screenshot of an overlapping diff

  Changed 2 years ago by cboos

Well, both this issue and #4070 were about the diff view... Now you're talking about the TracBrowser and it's true that there's still a problem there.

But the diff view looks ok, as you can see in the attachment:mob_db-diff.png

  Changed 2 years ago by theultramage@…

  • status changed from closed to reopened
  • resolution fixed deleted

Hah. Well what do you know... it's really true. So I made a list.

Where it doesn't work:
- Internet Explorer 6 (displays 3mm-wide scrollbar space, but cuts off text inside)
- Internet Explorer 7 (the same, plus it has horrible scrolling performance on trac pages)
- Mozilla Firefox <= 2.0 RC3 (same as IE I think)
- Opera latest build (lets text overflow but doesn't give a scrollbar)

Where it works:
- Mozilla Firefox 2.0 release

Latest unsupported CSS2.1 feature, maybe? Affects both changeset view and browser.

PS: the 'attach file' function still doesn't work for me on this bug tracker :\

  Changed 2 years ago by cboos

Ok, the current solution doesn't work with Internet Explorer.

  Changed 2 years ago by cboos

  • milestone set to 1.0

... plus the current solution also seems to be far from optimal when diffing big paragraphs, e.g.

http://trac.edgewall.org/wiki/TracTicketTriage?action=diff&version=31

Unfortunately, allowing wrapping here would reintroduce #3966.

follow-up: ↓ 12   Changed 22 months ago by cboos

  • milestone changed from 1.0 to 0.11

Another, rather extreme, example is r4620...

I think we should reintroduce wrapping, even if this means a regression for #3966. Any objections/better idea?

in reply to: ↑ 11   Changed 22 months ago by mgood

  • status changed from reopened to new

Replying to cboos:

I think we should reintroduce wrapping, even if this means a regression for #3966. Any objections/better idea?

Yes, #3966 looks like it just needs an &nbsp; so the <ins> element isn't collapsed for being empty.

  Changed 22 months ago by mgood

  • status changed from new to closed
  • resolution set to fixed

r4622 fixes this without breaking #3966.

  Changed 22 months ago by mgood

  • milestone changed from 0.11 to 0.10.3

The fix has also been ported to 0.10 in r4622.

  Changed 20 months ago by mgood

  • milestone changed from 0.10.3 to 0.10.4

Oops, this wasn't actually committed until after the 0.10.3 release.

Add/Change #1420 (long unwrappable lines in changeset are partially hidden)

Author



Change Properties
<Author field>
Action
as closed
Next status will be 'reopened'
to The owner will change from mgood. Next status will be 'closed'
 
Note: See TracTickets for help on using tickets.