595: Would be nice to have an option in the diff viewer for side-by-side or top-bottom

nithi******@gmai***** (Google Code) (Is this you? Claim this profile.)
Feb. 4, 2014
988, 1237 
Describe the enhancement and the motivation for it.

When viewing a diff on a tiny laptop monitor, the side-by-side view is
cumbersome to use. It word wraps some files, others you have to scroll
sideways.

If we could switch between side or top-bottom, that would be great.


What operating system are you using? What browser?

Win XP, FFox 3


Please provide any additional information below.
#1 nithi******@gmai***** (Google Code) (Is this you? Claim this profile.) 
I submitted this as enhancement, but it still shows up as type defect.
chipx86
#2 chipx86 
No chance of this before 2.0. It's a *lot* of work.
  • -Type-Defect
    +Type-Enhancement
    +Milestone-Release2.0
chipx86
#3 chipx86
david
#4 david
david
#5 david
  • +Component-DiffViewer
#6 ben.h*****@gmai***** (Google Code) (Is this you? Claim this profile.) 
chipx86, what all would need to be changed for this to happen? I'm trying to figure out how much work it'd be to attempt a patch.
chipx86
#7 chipx86 
Quite a lot.

For the diff viewer, you'd need whole new templates to handle the rendering, a new JavaScript backend to deal with all the interactivity. Probably some changes to the chunk metadata handling (for things like move detection and so on) so that it makes more sense in a top-bottom mode.

You'd then need to cache this separately, so the caching code would need to be updated. If users had the option between the views, your potential cache requirements would double.

Then, if you want to show these in the review request, you'd need to change that logic to show the correct view, whichever that may be (if that depends on the user, you've now made things considerably more complicated and expensive). You'd also have to update the diff loading spinner height calculation based on that new mode.

In my opinion, it's not worth it. I'm actually not convinced I want that in the codebase. Having multiple ways to show the diffs just restricts what we can do in the future and increases our testing needs.
#8 ben.h*****@gmai***** (Google Code) (Is this you? Claim this profile.) 
Thanks for the info. I understand your hesitance to complicate the codebase, but for our internal users, lack of inline view is their #1 UX complaint about ReviewBoard. Side-by-side can be easier to read, but on smaller screens it's a pain, and a lot of people are used to inline from source browsers (like GitHub), bug trackers (like Trac) or other review tools (like the one we're deprecating in favor of Reviewboard). 

It's good to know what stuff needs to be touched but I'm still hopeful that it could be done, especially given that the diffs basically start off in "inline view" and get transformed into side-by-side, so you at least don't need to do any extra transformations.
#9 somsom******@gmai***** (Google Code) (Is this you? Claim this profile.) 
I think Issue 988 and this one (Issue 595) are not duplicate, at all. 595 asks for top-bottom arrangement, while 988 asks for a tiny CSS change to switch off wrapping of pre-formatted text.

Note that browsers behaving differently, Firefox makes the undesired wrapping. I am not sure if it is a ReviewBoard or Firefox bug.
david
#10 david
  • -Milestone-Release2.0
david
#11 david 
This isn't going to happen.
  • +WontFix