Last Comment Bug 40896 - Bugzilla needs a "preview" mode for comments
: Bugzilla needs a "preview" mode for comments
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: unspecified
: All All
: P3 enhancement with 34 votes (vote)
: Bugzilla 5.0
Assigned To: Byron Jones ‹:glob›
: default-qa
Mentors:
: 288210 316823 327356 424113 460834 (view as bug list)
Depends on:
Blocks: 975204 993920
  Show dependency treegraph
 
Reported: 2000-05-28 14:58 PDT by hacksaw
Modified: 2015-02-21 07:54 PST (History)
34 users (show)
justdave: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch - shows only the user's submitted comment (5.44 KB, patch)
2003-03-11 23:45 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
no flags Details | Diff | Review
40896_1.patch (6.44 KB, patch)
2014-01-30 23:07 PST, Byron Jones ‹:glob›
LpSolit: review-
Details | Diff | Review
Tabs in Standard skin (7.41 KB, image/png)
2014-01-31 05:01 PST, Gervase Markham [:gerv]
no flags Details
Tabs in Dusk skin (7.22 KB, image/png)
2014-01-31 05:02 PST, Gervase Markham [:gerv]
no flags Details
40896_2.patch (8.18 KB, patch)
2014-02-10 21:41 PST, Byron Jones ‹:glob›
gerv: review-
Details | Diff | Review
40896_3.patch (8.19 KB, patch)
2014-02-11 23:44 PST, Byron Jones ‹:glob›
gerv: review+
Details | Diff | Review

Description hacksaw 2000-05-28 14:58:16 PDT
It would be cool it one could get preview of what their bug report and/or
additional comments will end up looking like before they actually get stored in
the database.  I always end up finding annoying formating issues with my own
submissions that must really irritate the people who have to look at them.
Comment 1 Stephan Niemz 2001-02-28 09:13:00 PST
moving to real milestones...
Comment 2 Andreas Franke (gone) 2001-09-08 05:06:30 PDT
-> Bugzilla product, Creating/Changing Bugs component, reassigning.
Comment 3 Andreas Franke (gone) 2002-06-21 04:35:47 PDT
See also bug 5179 comment 6.
Comment 4 Jason Bassford 2002-08-07 06:57:48 PDT
This seems to me to be a better option than bug 540 - which opens up issues with
censorship / historical alteration after the fact - but not necessarily mutually
exclusive.

FWIW, I've just got my own bug 161470 duped to bug 540, but it really also
encompassed this bug.  I'm tired of making mistakes, which mostly appear after
the fact as glaringly obvious because I've had mental dyslexia and entered a bug
number that's different from what I'd intended by one number.  Only after the
post, and seeing the created hyperlink with a cross through it (I'd meant to
indicate an open bug rather than one already closed), do I realise what I've done.

If I had a chance to see the "final" version of the post onscreen (with
hyperlinks and not cramped within the confines of the Additional Comments text
box) I'd be able to spot far more mistakes like that and fix them before they
hit the database and result in a) spam due to correction posts, and b)
embarrassment.
Comment 5 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-03-11 21:54:25 PST
I think this would be pretty cool.  I'd probably want it as a checkbox next to
the Commit button or something though.  I'd imagine some people would be annoyed
by the extra click to verify the submit if they were in a hurry and didn't care
if their comment was stupid. :)
Comment 6 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-03-11 23:45:09 PST
Created attachment 116940 [details] [diff] [review]
Patch - shows only the user's submitted comment

Don't know why I spent a half hour working on this, unless it was only because
I was sick of hearing timeless whine about how he couldn't figure it out on IRC
after he'd been screwing with it for an hour.

Timeless doesn't like my implementaton because it only shows the comment the
user was just submitting.  He thinks it needs to show the entire bug's comments
so the comment anchor references will work.

This patch is live at http://landfill.bugzilla.org/bzdave/ if you want to try
it out.  Just pick a random bug and make a comment, make sure to check the
"preview" box next to the commit button.
Comment 7 Gervase Markham [:gerv] 2003-03-12 00:04:39 PST
I'm mildly opposed to this. I'd be heavily opposed if it were in any way
compulsory or on by default, but I'm still mildly opposed to the optional version.  

Reasons: it's another step in the UI complexity creep, to add a feature which
IMO isn't really necessary. It's also another step on the "Bugzilla as
discussion forum/bulletin board" road, which I think is bad - the more we move
in that direction, the more people will discuss rather than _do_.

So people mess up comments occasionally - big deal. Post a correction. That's
what happens day-to-day on b.m.o, and it works. I suspect that this option will
actually lower productivity, because people will spend more time previewing and
checking their posts than they used to spend posting the occasional correction. 

For those with a persistent habit of mistyping bug numbers, I suggest you copy
and paste them :-) The selection clipboard (on OSes which support X Window,
anyway) is great for this.

Gerv
Comment 8 Aleksey Nogin 2003-03-12 00:11:38 PST
> I'd probably want it as a checkbox next to the Commit button or something
> though.

In that case, the default state of the checkbox should be user-configurable. I
very often mistype things and I would definitely like to be able to set the
default to "preview".
Comment 9 Aleksey Nogin 2003-03-12 00:13:31 PST
Comment on attachment 116940 [details] [diff] [review]
Patch - shows only the user's submitted comment

How about previewing in enter_bug.cgi? I would argue that the initial report is
more important than random extra comment and if only one of the two will get a
preview mode, it should be the enter_bug one. And enter_bug preview should
definitely show all fields.
Comment 10 Gervase Markham [:gerv] 2003-03-12 00:36:55 PST
An enter_bug preview is just as pointless, because once you've submitted, you
get the bug back, so you can change any fields which turn out to be set wrong
anyway. 

Gerv
Comment 11 Aleksey Nogin 2003-03-12 01:27:51 PST
> An enter_bug preview is just as pointless, because once you've submitted, you
> get the bug back, so you can change any fields which turn out to be set wrong
> anyway.

Not the initial description and only if you do not mind generating extra bugmail.
Comment 12 Bradley Baetz (:bbaetz) 2003-03-12 01:56:41 PST
I don't think we need a preview mode, really. Theres no extra formating applied
by bugzilla - What You Type is What you Get (apart from the buglink stuff which
a commenter has no control over). So this is only used as a 'am I really sure I
want to do this' step, which would:

a) Be really annoying
b) Have no use, since you can always undo a change later
Comment 13 Jason Bassford 2003-03-12 05:25:50 PST
I'll just state two things.  A preview would be immensely useful to me in order
to avoid typos and, in particular, typos involving a bug number that I've got
wrong.  If the link to the bug is active in the preview, I'd be able to
immediately tell that it's not going where I'd meant it to go and change it. 
(Even with a copy/paste scenario, you could still get the bug number wrong
because it's not an OBVIOUS typo until after the fact.)

Also, of all of the places where a preview should exist, it should be the
initial bug report.  It's far more important to get that first summary correct
than it is any other comment that appears later.

While I appreciate Gerv's comment about how extra bugmail is no big deal, nor
are typos, I, as the one screwing up, don't personally agree.  I'd rather not a)
embarrass myself publicly, which a preview can help fix, b) be the cause of
bugmail that some people, including myself, do find annoying, or c) have to read
correction bugmail that other people post.

Since this is a feature that's already been indicated should be optional, and
which would help at least some people, I don't see it as anything but a benefit.

> a) Be really annoying

It would be immensely useful to me and not annoying at all.  If you find the
feature annoying you don't need to use it.

> b) b) Have no use, since you can always undo a change later

Without bug 540, there is no real way of undoing it.  And, even if an additional
comment with the change "sort of" works, or changing the status of a field,
bugmail is still generated.
Comment 14 Gervase Markham [:gerv] 2003-03-12 15:49:27 PST
An "optional" feature still has a cost, in increased UI complexity, and
maintenance. 

Typos are only a problem if people can't figure out what you mean. Learning to
type while looking at the screen helps to cut them down; but you could also
install a product which spellchecked textareas. I don't know if the current
spellchecker does that - if it doesn't, file an enhancement request.

The question you need to answer is, with the exception of buglinks, what does a
preview give you that just rereading the comment in the textbox doesn't? 

Gerv
Comment 15 Jason Bassford 2003-03-12 16:34:03 PST
> The question you need to answer is, with the exception of buglinks,
> what does a preview give you that just rereading the comment in the textbox 
> doesn't? 

Assuming that no typos are made, nothing.  However, the reason that this bug
exists is because typos ARE made, and they're made despite having re-read
comments made in the textbox.  (Being able to re-read textbox comments has
existed for years, yet typos are still generated and people still lament the
fact that they *wouldn't* be made nearly as often if they'd only had a chance to
spot them in some other fashion.)  Seeing the "final" product as it will appear
is quite different from having to proof-read it by scrolling up and down within
in a text box - especially if it's long comment (sort of like this one) that's
being generated.  While there's no doubt that more typos than currently exist
could be prevented by having everybody extremely carefully proof everything
before sending it, that's an ideal situation, not a practical one.

Having a "last chance", where the final result is seen exactly as it will
actually appear, will help prevent anything that would normally slip through
otherwise.  The "cost" in terms of complexity needs to be weighed against the
amount of annoyance, and database space, it will save by not having to file one
or more "corrective" bugmail producing comments after the fact.
Comment 16 Jason Bassford 2003-03-12 16:38:11 PST
Finally - I think there's now been enough debate on this topic.  (And I actually
happen to agree with points made for both sides, even though I'm in still in
favour of one over the other.) Further "pro" and "con" views are just so much
noise at this point.    A patch has been submitted.  It should be reviewed, or
not.  Anything further should be from a stricly Bugzilla / technical standpoint.
 Further debate should be taken elsewhere.
Comment 17 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-03-12 17:45:22 PST
It's actually not very complex (well, the way I did it it's not, anyway, I think
timeless' version if he ever posts it is a little more complex :)  

You'll notice in the patch that I attached that there's a whole 9 lines of code
added to deal with it (if you don't count the whitespace and comments), and the
rest is done with a template that was cloned from the existing mid-air template
and modified. :)

I personally think being able to see the tooltips for the bug numbers you
entered in and of itself is enough reason to have this.  I thought it was a cool
idea when I saw it.
Comment 18 Jacob Steenhagen 2003-03-12 18:21:49 PST
It never ceases to amaze me how often I catch a typo dispalyed on the page that
I didn't catch in the text box. Does that mean I'd use a preview mode. Maybe,
but more for the bug number verification than anything else. I agree with Dave
that this is a useful option.

Regarding spell-checking the text area: I wish Mozilla would do this, but unless
it's changed and I didn't notice it Mozilla doesn't come with any spell checker,
only the framework. I seem to remember seeing a bug once upon a time asking for
the ability to spell check text areas, but I don't recall any outcome (and I'm
too lazy to search for it right now).
Comment 19 Aleksey Nogin 2003-03-12 18:41:50 PST
Spell-checking text fields is bug 23421. Spell-checking complete form (multiple
fields) is bug 16409. Dropping mozdev spellchecker (that have been around since
12/2001) into Mozilla is bug 56301.
Comment 20 Gervase Markham [:gerv] 2003-03-15 12:33:41 PST
Well, you da boss.

The standard way of doing this seems to be to have two buttons - "Preview" and
"Submit". But I don't know if that's appropriate if we are just previewing the
comment.

If we are having a user pref (and we will probably need one), we should have:

Preview Comments: ( ) Always
                  ( ) Ask Me
                  ( ) Never

There would only be an (unchecked) UI checkbox if they selected Ask Me. Some
people always want it, some people never want it. This way, those people aren't
bothered by useless UI. I'd say Never should be the default for new accounts
(start with the simpler interface) but might be persuaded by an argument for Ask Me.

Gerv
Comment 21 Myk Melez [:myk] [@mykmelez] 2003-03-18 13:58:34 PST
There's more to this than the issue of whether preview is sometimes useful or
would be more pain than it would be worth to implement and maintain.  There is a
larger philosophical issue of what kind of communications Bugzilla comments are
intended to support: quick, chatty, informal conversation among peers (as was
likely the original intent) or slow, deliberative, formal discussion between
strangers partially motivated by the desire for respect and a reputation in the
community.

Preview is useful for formal discussion because typos and other mistakes detract
from the message of a formal text and the reputation of its author.  Preview is
harmful for informal conversation, though, because it implies formality and
influences people to spend extra time reviewing comments that they would
otherwise have just submitted.

The cost of adding preview is the reduced efficiency of formal communication,
and while this cost is hard to measure, it is not negligible.  We need to
consider this issue before deciding to implement this feature.  If, however, the
consensus is that Bugzilla discussion is already formal enough to warrant the
feature and should remain that way, then I agree with Gerv about adding a
preference for the feature and defaulting it to off.
Comment 22 Gervase Markham [:gerv] 2003-03-18 14:51:45 PST
Myk raises a good point.

Joel (as in, on Software) makes a related point also, in:
http://www.joelonsoftware.com/news/20020912.html
that the more a bug system becomes high-ceremony, the less likely people are to
use it. One of the things it currently says on the website about the aims of
Bugzilla is that we should concentrate on being a bug tracking system, and so
perhapsnot a testcase management system, project management system (hmm) or
discussion board (my examples.)

Another thought: adding this feature gives well-meaning managers an opportunity
to damage their team's productivity (without knowing it) by turning it on.

Gerv
Comment 23 Jason Bassford 2003-03-18 19:20:10 PST
> The cost of adding preview is the reduced efficiency of formal
> communication

I really have no idea how formal communication can be considered to result in
*reduced* efficiency.

If I spend 10 minutes coming up with a well written bug report that clearly
spells everything out, partly because I'm able to preview what I've written, go
back, and make changes - as opposed to 2 minutes blurting out a series of typo
ridden, badly thought out comments, along with follow-ups to correct previous
comments which I didn't catch the first time around - I don't see how the more
formal result can be thought to be a worse state of affairs.  Surely, clear and
simple communication of an idea is preferable to something that requires more
effort on the part of everybody to understand and clarify.
Comment 24 Bradley Baetz (:bbaetz) 2003-03-18 23:26:41 PST
But nothing is stopping you reading your comments before you press the submit
button.

IF you have a forced preview, peopel are just going to mindlessly ignore it
anyway, and just automatically hit ok.
Comment 25 Gervase Markham [:gerv] 2003-03-19 00:28:03 PST
> I really have no idea how formal communication can be considered to result in
> *reduced* efficiency.

You must be in management ;-)

Your straw man is invalid; Bugzilla's lack of a preview mode should not affect
whether your bug-report is well thought out or how long you spend on it.

Here's another option, should you desire one. Use:
http://bugzilla.mozilla.org/page.cgi?id=linkify.html
(perhaps slightly modified, so you can resubmit easily) as your preview
mechanism, and copy-and-paste into the actual bug when you've got it right.

Gerv
Comment 26 Jason Bassford 2003-03-19 05:20:17 PST
> But nothing is stopping you reading your comments before you press the
> submit button.

There is a big difference between scrolling back and forth within a small text
area, and seeing the entire result up on the actual page as it will finally
appear.  (Even if we could get an "Additional Comments" button that would take
you to a textarea much larger than the one we already have it would be an
improvement.)  Also, as has been stated repeatedly, this is also about
linkifying bug numbers and being able to get the tooltip text to appear for
verification.  

> IF you have a forced preview, peopel are just going to mindlessly ignore
> it anyway, and just automatically hit ok.

<sigh> Nobody here has said anything about forcing this.  It should be optional
and disabled by default.  It's only if the poster wants a preview that they can
turn it on for themeselves.

> Bugzilla's lack of a preview mode should not affect whether your bug-report
> is well thought out or how long you spend on it.

Well, it does.  It does every day that I file additional comments.  There hasn't
been a day that's gone by in the last two years or more when I haven't noticed,
after the fact, a typo of grammatical error (however small) in at least one of
my submissions - a mistake that I could easily have caught if I had had a chance
to see the final result and not had to scroll up and down in the tiny input
window I have to work with here.  That's the very reason why anybody would even
bother to file this bug in the first place.

(And please don't tell me that I can compose it somewhere else then copy/paste.
 If I'm quoting somebody, or looking at various other fields within the bug,
making changes, I'd like to remain within the same work environment rather than
switching back and forth between tools.  I *could* do that but then it really
*would* be too much like an actual job, and I'm not going to do that until
somebody pays me for my reports and insists on a certain level of
syntactic/grammatic correctess.)

Okay, I've just recycled all of the points I, and others, made earlier on in
this bug.  So have the people I just bothered replying to.  Why are we repeating
ourselves?  Nobody's going to get the other person to change their mind by just
bringing up the same points.  (I don't see any moments of sudden enlightenment
sneaking up on me after hearing the same thing for the 10th time. <grin>) 
Obviously, I'm the worst offender because *I'm* the one who posted comment 16 -
and here I am debating some more...
Comment 27 Gervase Markham [:gerv] 2003-03-21 15:31:14 PST
> (Even if we could get an "Additional Comments" button that would take
> you to a textarea much larger than the one we already have it would be an
> improvement.) 

That can easily be fixed with a bookmarklet which expands the textarea to any
size you like. Perhaps that would help?

Gerv
Comment 28 Aleksander Slominski 2003-04-25 09:11:03 PDT
preview is standard in almost every web based input system.

is there any _technical_ reason why bugzilla can not have it? it may be turned
off by default if bugzilla developers dont like it but it should be available ... 

just my 2c
Comment 29 Tom Sommer 2003-06-08 14:52:35 PDT
A preview for the bug reports would be extremely nice indeed.

Perhaps also an option to force users to preview their bugs before they submit them.

Bugzilla really needs this feature
Comment 30 Krishna Sethuraman 2004-07-29 19:59:22 PDT
My best justification for this feature is for previewing the initial comment. 
More than once, I've turned over my automobile insurance envelope to lick it,
and the little note on the flap reminded me to put my policy number on the
check.  Having the option to provide a reminder to the user of the form: 

Please double-check that all necessary information is entered:
* description
* how to reproduce
* your software and firmware versions
* etc

along with a preview of the bug-creation description, IMHO, is sufficient
justification to provide a preview feature.  Consider further that less-savvy
users enter initial descriptions on bugs, and a reminder/preview can be of value
here.

Since a patch is available, what more (technically or philosophically) needs to
be hashed out before this is implemented?  Even if mozilla's bugzilla doesn't
need it, other projects (kde, etc) use bugzilla, and their bug-tracking
philosophy may prefer to provide preview on a site-wide and/or per-user basis. 
KDE has modified their copy of bugzilla (or enabled the correct feature?) to
right-justify text; in a case like this, preview would be good to make clear
that line breaks appear in the right spots.

I don't want to insult the developers' intelligence by pointing out 

* if it served their needs, individual bugzilla installations would less likely
take the attached patch and integrate it into their site; but more likely would
turn it on if it were an existing bugzilla feature

* people simple expect to have a preview option on web based collaborative
communication mechanisms with unrestricted populations.  Of course, this doesn't
justify it on technical grounds.

Thank you for an excellent product.
Comment 31 Stewart Gordon 2004-07-30 02:19:21 PDT
(In reply to comment #30)
> Having the option to provide a reminder to the user of the form: 
> 
> Please double-check that all necessary information is entered:
> * description
> * how to reproduce
> * your software and firmware versions
> * etc

People who want it on this level are likely to use Bugzilla Helper.  But this by
itself misses the point half of us are trying to make, of being able to check
for mistyped bug numbers.

> Since a patch is available, what more (technically or
> philosophically) needs to be hashed out before this is implemented?
> Even if mozilla's bugzilla doesn't need it, other projects (kde,
> etc) use bugzilla, and their bug-tracking philosophy may prefer to
> provide preview on a site-wide and/or per-user basis.

I'm not sure if having it per user makes much sense.  What warrants a single
command to have a preference to show/hide the UI?  I can't see any problem with
the common solution of having separate Preview and Commit buttons - whose way
would it get in?
Comment 32 Frédéric Buclin 2005-03-29 14:29:18 PST
*** Bug 288210 has been marked as a duplicate of this bug. ***
Comment 33 timeless 2005-08-07 17:39:06 PDT
my views:
http://viper.haque.net/~timeless/blog/91/
http://viper.haque.net/~timeless/blog/92/

I'm not saying that preview should be mandatory by default. It might be reasonable for a product to be 
able to set preview as a requirement for certain groups, but that's really beyond the scope of this bug. 
For the purposes of this bug, a button next to commit labeled "Preview" will be suffice.

I'd like to suggest that a preview feature is considerably less expensive than the alternative which as i 
see it is mutable comments.
Comment 34 Marc Schumann [:Wurblzap] 2005-11-17 02:04:30 PST
*** Bug 316823 has been marked as a duplicate of this bug. ***
Comment 35 Frédéric Buclin 2006-02-17 05:33:29 PST
*** Bug 327356 has been marked as a duplicate of this bug. ***
Comment 36 Greg Watson 2006-06-23 08:06:40 PDT
What happened to this?  It was three years ago and I don't see a solution or final comment.  

I had an issue just today where perview would have helped.  While I had thought I read everything in the bug I was submitting, bit missed an IP address that line-wrapped in the input text field.  After posting I relized (since it was no longer wrapped) I had posted my public ip address.  The site said you couldn't edit bugs and so I was left having them restrict it. 

A preview button next to submit would have solved this problem for me.  I would like to have everyone revisit this issue.  
Comment 37 Felipe Corrêa da Silva Sanches 2006-06-23 08:17:09 PDT
(In reply to comment #36)
Maybe a WYSIWYG AJAX preview would be even better...
Comment 38 Aleksander Slominski 2006-06-23 08:20:39 PDT
ANY preview will be better than current one (I do not care much if it is WYSIWYG , or AJAX or whatever ...)
Comment 39 Stewart Gordon 2006-06-23 08:29:21 PDT
If it's not WYSIWYG, then it would be wrong to call it a preview, surely?
Comment 40 Aaron Larson 2006-10-31 14:31:53 PST
This bug seems pretty dormant.  I'd certainly like to see *something* rather than nothing in this regard.

Personally, I'd be happy if there was just a "preview" button next to the "additional comments" that would bring up a new pane to review, or perhaps a new block in the same display, pretty much anything.  My users make simple typos that are mildly annoying just enough that they stop doing things I'd really like to encourage.  E.g., See "bug 1234 commnet 3", stray punctuation, etc.  The additional comments *is* a markup language, albeit a pretty simple one.
Comment 41 Marc Schumann [:Wurblzap] 2008-03-20 07:00:46 PDT
*** Bug 424113 has been marked as a duplicate of this bug. ***
Comment 42 Frédéric Buclin 2008-10-20 13:55:07 PDT
*** Bug 460834 has been marked as a duplicate of this bug. ***
Comment 43 Chris McKenna 2013-07-18 13:29:31 PDT
I know this is a very old bug, but it is still something that would be very useful. Several times over the past few days I've made and seen typos in links / formatting that required a second comment to correct.
Comment 44 Robert Pollak 2014-01-29 14:57:46 PST
I also would often like to have the functionality of /page.cgi?id=linkify.html more easily available, for syntax tests and to check tooltips of bug numbers (since a comment is "write once read many").

What about having an immediate (javascripted) preview during typing, as on https://stackoverflow.com?
This could be user- and administrator-configurable as:
* always hidden
* hidden, but activated with a "Preview" checkbox
* always active.
Comment 45 Gervase Markham [:gerv] 2014-01-30 01:40:34 PST
(In reply to Robert Pollak from comment #44)
> What about having an immediate (javascripted) preview during typing, as on
> https://stackoverflow.com?

That would require either:

a) lots of API calls, and lag in updating the preview; or
b) the linkify code to be maintained both as Perl and as JS, and kept in sync

Neither of those options is awesomely appealing, particularly considering this is a function that can be hooked, and so for b), hook authors would also need to write their hook twice, once in each language.

Gerv
Comment 46 Byron Jones ‹:glob› 2014-01-30 23:07:01 PST
Created attachment 8368422 [details] [diff] [review]
40896_1.patch

i had this work lying around for a different project; just needed a little bit of polish.

this patch mirrors github's functionality by adding a 'preview' tab which calls back to the server to render the text as html.
Comment 47 Gervase Markham [:gerv] 2014-01-31 05:01:13 PST
Created attachment 8368507 [details]
Tabs in Standard skin
Comment 48 Gervase Markham [:gerv] 2014-01-31 05:02:25 PST
Created attachment 8368508 [details]
Tabs in Dusk skin
Comment 49 Frédéric Buclin 2014-01-31 12:30:31 PST
Comment on attachment 8368422 [details] [diff] [review]
40896_1.patch

>=== modified file 'Bugzilla/WebService/Bug.pm'

>+sub render_comment {

>+    my $bug = Bugzilla::Bug->check($params->{id});
>+    $bug->check_is_visible();

check() already calls check_is_visible().


>+B<Required> C<int> The ID of the bug to render the comment against.

The bug ID shouldn't be required. It's actually of no use if you are already viewing a bug report, i.e. href="#foo" will point to the same place as href="show_bug.cgi?id=$bug_id#foo". This would avoid a useless validation when previewing new comments.


Also, the preview feature isn't available when creating a new bug report.


I applied your patch, and it looks good visually. It's non-intrusive. I like it! :)
Comment 50 Gervase Markham [:gerv] 2014-02-01 00:05:58 PST
(In reply to Frédéric Buclin from comment #49)
> The bug ID shouldn't be required. 

The bug_format_comment hook takes it as a parameter. It's allowed to be undef if the text is "not on a bug" - whether that's true in this case could be argued both ways ;-)

Given that this is only a preview, I think it's OK not to pass it.

> Also, the preview feature isn't available when creating a new bug report.

That would be nice too :-)

Gerv
Comment 51 Byron Jones ‹:glob› 2014-02-10 21:41:07 PST
Created attachment 8373841 [details] [diff] [review]
40896_2.patch

this revision:
- the bug id is now optional
- adds comment preview to enter_bug as well as show_bug
- no longer visible if jsonrpc feature isn't available
- fixes the alignment of the tabs
Comment 52 Gervase Markham [:gerv] 2014-02-11 02:02:45 PST
Comment on attachment 8373841 [details] [diff] [review]
40896_2.patch

Review of attachment 8373841 [details] [diff] [review]:
-----------------------------------------------------------------

Looks and works well. A few comments, though (r- until we figure out the styling issues).

Gerv

::: Bugzilla/WebService/Bug.pm
@@ +323,5 @@
> +
> +    unless (defined $params->{text}) {
> +        ThrowCodeError('params_required',
> +                       { function => 'Bug.render_comment',
> +                         params   => [ 'text'] });

Nit: extraneous space.

::: skins/standard/global.css
@@ +678,4 @@
>      margin-left: -1px;
>  }
>  
> +.comment_tab {

At least on my copy, the style issues reported last time are still present. The preview has no border in the Classic skin, and the tabs are floating a couple of pixels above the textbox in both skins. Setting a 3px left margin on the tab container box fixes the horizontal alignment, and a -2px negative margin on the top of the comment box fixes the vertical alignment. But there may, of course, be other ways to do it.

This may be a Linux thing; I think it's something to do with the way the textbox is being rendered rather than the tabs themselves. (My textboxes have slightly rounded corners.) In Dusk, on the Preview tab, the alignment is fine.

::: template/en/default/global/comment.html.tmpl
@@ +29,5 @@
> +
> +[% IF feature_enabled('jsonrpc') %]
> +  <div id="comment_preview" class="bz_default_hidden bz_comment">
> +    <div id="comment_preview_loading" class="bz_default_hidden">Generating Preview...</div>
> +    <div id="comment_preview_error" class="bz_default_hidden">Empty</div>

Do we need the word "Empty" here?
Comment 53 Byron Jones ‹:glob› 2014-02-11 06:01:48 PST
(In reply to Gervase Markham [:gerv] from comment #52)
> At least on my copy, the style issues reported last time are still present.

you didn't report any style issues last time; you just attached 2 screenshots.

> The preview has no border in the Classic skin

that's because comments in the classic skin don't have a border.
i can add one just for the preview when using classic however.

> and the tabs are floating a
> couple of pixels above the textbox in both skins. Setting a 3px left margin
> on the tab container box fixes the horizontal alignment, and a -2px negative
> margin on the top of the comment box fixes the vertical alignment. But there
> may, of course, be other ways to do it.
> 
> This may be a Linux thing; I think it's something to do with the way the
> textbox is being rendered rather than the tabs themselves. (My textboxes
> have slightly rounded corners.) In Dusk, on the Preview tab, the alignment
> is fine.

ok, i'll mess around with your suggestions and the classic skin, but i'm not keen on pouring a lot of time into making things pixel-perfect with the classic skin.

> > +    <div id="comment_preview_error" class="bz_default_hidden">Empty</div>
> Do we need the word "Empty" here?

no, that's some debugging stuff which i forgot to remove :|
Comment 54 Gervase Markham [:gerv] 2014-02-11 06:09:03 PST
(In reply to Byron Jones ‹:glob› from comment #53)
> you didn't report any style issues last time; you just attached 2
> screenshots.

Hmm, weird. They were supposed to have an accompanying comment!

> that's because comments in the classic skin don't have a border.
> i can add one just for the preview when using classic however.

I think that would be good; it looks odd otherwise, because the text box has a border so when you switch tabs, the tabs seem to get "cut off" and float in mid-air.

> ok, i'll mess around with your suggestions and the classic skin, but i'm not
> keen on pouring a lot of time into making things pixel-perfect with the
> classic skin.

Well, as long as we ship it, we should make it work. Hopefully, it won't take long.

Gerv
Comment 55 Frédéric Buclin 2014-02-11 09:03:50 PST
Comment on attachment 8373841 [details] [diff] [review]
40896_2.patch

>=== modified file 'skins/standard/global.css'

>+.comment_tab {

Please move all this stuff into show_bug.css, which is called by default for all bug-related templates.



>=== added file 'template/en/default/global/comment.html.tmpl'

It should be named bug/comment.html.tmpl as it's a bug-only template called from bug-only templates.
Comment 56 Byron Jones ‹:glob› 2014-02-11 19:50:12 PST
(In reply to Frédéric Buclin from comment #55)
> Please move all this stuff into show_bug.css, which is called by default for
> all bug-related templates.

show_bug.css isn't included in enter_bug.
Comment 57 Byron Jones ‹:glob› 2014-02-11 23:34:25 PST
> At least on my copy, the style issues reported last time are still present.
> The preview has no border in the Classic skin, and the tabs are floating a
> couple of pixels above the textbox in both skins.
> This may be a Linux thing.

yeah; this looks like a linux thing .. i suspect your text field widget has built in padding.

i don't see any gaps on windows or mac, and the horizontal alignment looks perfect. there's currently 0px between those elements according to the box model, so introducing negative margins makes them overlap.
Comment 58 Byron Jones ‹:glob› 2014-02-11 23:44:50 PST
Created attachment 8374664 [details] [diff] [review]
40896_3.patch

- adds border to preview in classic skin
- moves global/comment.html.tmpl to bug/comment.html.tmpl
- fixes nits
Comment 59 Gervase Markham [:gerv] 2014-02-12 06:39:12 PST
Comment on attachment 8374664 [details] [diff] [review]
40896_3.patch

r=gerv.

Gerv
Comment 60 Dave Miller [:justdave] (justdave@bugzilla.org) 2014-02-12 15:32:46 PST
Wow, this only took forever.  Hooray!
Comment 61 Byron Jones ‹:glob› 2014-02-12 21:46:33 PST
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/WebService/Bug.pm
modified js/field.js
modified skins/standard/global.css
added template/en/default/bug/comment.html.tmpl
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/create/create.html.tmpl
Committed revision 8918.
Comment 62 Frédéric Buclin 2015-02-21 07:54:50 PST
Added to relnotes for 5.0rc1.

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