My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 1260: Allow for multiline comments submitted via SSH
2 people starred this issue and may be notified of changes. Back to list
Status:  WontFix
Owner:  ----
Closed:  Feb 2012


Sign in to add a comment
 
Reported by favore...@gmail.com, Feb 12, 2012
All comments that are submitted via SSH have their newlines stripped out.
Later versions of GWT allow for SafeHtmlBuilder().appendEscapedLines(), the one currently used in Gerrit doesn't.

However, a simple patch of /gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/CommentPanel.java
	
(https://code.google.com/r/favoretti-gerrit/source/detail?r=0e919976ded000b8ecb67d7c877875599b1e7bd5)

would allow replacing "\n"'s with "<br>" upon rendering, thus, enabling proper rendering of newlines.

Please consider applying.

Feb 13, 2012
Project Member #1 nas...@codeaurora.org
Multiline comments over ssh are possible:
$ ssh -p 29418 <server_name> gerrit review --verified=-1 \
 \'--message='Message with more than one line.

We needed two paragraphs here.'\' \
 12,1

http://groups.google.com/group/repo-discuss/browse_thread/thread/1c00ac6ba5c447d4/7a5fd86ad31118f7?lnk=gst&q=Multiline+comments+#7a5fd86ad31118f7

Is that what you're looking for?
Status: AwaitingInformation
Feb 13, 2012
#2 favore...@gmail.com
Not really. If you follow the CommentPanel rendering code, it will not replace newlines with <br>'s, hence CommentPanel in web interface will ignore those newlines. 

I found this post of course, before making this modification and tried it inwards and outwards. Database stores those newlines correctly, they are just rendered improperly in the web interface.

Feb 13, 2012
Project Member #3 nas...@codeaurora.org
Ok, can you post an example ssh command, what you see Gerrit render now, and what you would like displayed instead? Also, this sounds like it's not specific to SSH. Do you see the same behavior with newlines via the web UI?
Feb 13, 2012
#4 favore...@gmail.com
Ok, I admit my explanation might be a bit cumbersome.

Example of command is pretty much the same you used in your abovestanding post.
In case of stock 2.2.2, Gerrit will render comment as:

"Message with more than one line.  We need two paragraphs here."

What I want it to render is exactly what was submitted to the database:

"Message with more than one line.

We need two paragraphs here."

The only gerrit I have handy now has been patched, I could provide you with a few screenshots on a dev gerrit installation if you want, though. I'll need to revert this change and shoot. Let me know if screenshots will help.

Thanks!

Feb 13, 2012
#5 favore...@gmail.com
Ow, and just to be sure, I'm talking about rendering in web UI only.
And indeed, I just tested, my apologies, this is not related to SSH-only.

See the web-comment screenshot results.

Screen Shot 2012-02-13 at 5.34.51 PM.png
21.6 KB   View   Download
Screen Shot 2012-02-13 at 5.34.56 PM.png
11.0 KB   View   Download
Feb 13, 2012
Project Member #6 nas...@codeaurora.org
I just tested that command on 2.1.8 and 2.2.2 and it renders just as you would like it to. What browser are you using? I'm using Chrome and Firefox.
Feb 13, 2012
Project Member #7 nas...@codeaurora.org
Ah, looking at your first screenshot, you're missing a newline. Gerrit comments are like wiki formatting, you need two newlines to form a new paragraph. Try the exact command I pasted above.
Feb 13, 2012
#8 favore...@gmail.com
I'm using firefox 10 in this case. 
I wonder though how can it be that it renders properly. Just judging from the code, either your browser renders \n properly due to some magic, or mine doesn't due to the same reason or I see now <br>'s between those lines. See another screenshot with the HTML that is underlaying.

Screen Shot 2012-02-13 at 5.39.10 PM.png
38.5 KB   View   Download
Feb 13, 2012
#9 favore...@gmail.com
Ahhhh, that would explain it.
Well, then let me dig into the details a bit. 
We use gerrit to manage puppet repository (puppetlabs.com)

What we have is a patchset-created-hook that runs a lint parser on the commited code and auto-reviews either +1 or -1 based on whether parser run succeeded or not.

In case of +1 all is savvy, as we just add one line of comments, in case of -1 we want the parser output to be added to a comment via that same SSH command. There's no real way except dirty hacking to insert an extra linebreak into the parser output code. Is there some tag we can use to insert preformatted text?

Thanks again.

Feb 13, 2012
#10 favore...@gmail.com
And having paragraphed output indeed helps, although it creates an unnecessary extra linebreak (that's natural for a new paragraph of course). Although imagining parser output of 50+ lines with attached output will not be aesthetically pleasant :)

Although I understand that my proposed patch is not in line with the wiki standard, I'm curious if there's a normal way to work this problem around.
Screen Shot 2012-02-13 at 5.52.59 PM.png
9.8 KB   View   Download
Feb 13, 2012
Project Member #11 bklarson@gmail.com
The only other option I can think of is using a bullet list.  I'm not sure if that would work for your situation.

To create a list, start each item with a '*' and separate with a newline.

* item 1
* item 2
* item 3
Feb 13, 2012
#12 favore...@gmail.com
Hmm, would you be willing to accept a patch to wikify to have some sort of pre-formatted text tag? Or you don't want to go the "tag my comments" path?
Feb 13, 2012
Project Member #13 bklarson@gmail.com
One other idea I thought of - you can specify pre-formatted text by starting the lines with a space.  The formatter will then honer newlines explicitly.  The downside is that it also uses a monospace font, because most of the time this is used it is to display code.
Feb 13, 2012
#14 favore...@gmail.com
I think I just found a way. Looking at the wikify code:
  private static boolean isPreFormat(final String p) {
    return p.contains("\n ") || p.contains("\n\t") || p.startsWith(" ")
        || p.startsWith("\t");
  }

prepending each line with a space should get me where I want to be. Although final formatting is somewhat interesting...


Screen Shot 2012-02-13 at 6.12.48 PM.png
19.2 KB   View   Download
Feb 13, 2012
#15 favore...@gmail.com
Anyway, thanks a lot for your time, I'll revert our local patch, feel free to close this as a non-issue. Sorry for wasting your time, should've read the code more closely :)

Feb 13, 2012
Project Member #16 bklarson@gmail.com
Glad you found a solution
Status: WontFix
Sign in to add a comment

Powered by Google Project Hosting