Bug 345847 - [api] create UI for private comments
Summary: [api] create UI for private comments
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.6   Edit
Assignee: Frank Becker CLA
QA Contact: Frank Becker CLA
URL:
Whiteboard:
Keywords: noteworthy, plan
Depends on:
Blocks: 284026 349620
  Show dependency tree
 
Reported: 2011-05-15 11:17 EDT by Frank Becker CLA
Modified: 2011-07-18 15:14 EDT (History)
2 users (show)

See Also:


Attachments
patch V1 (11.56 KB, patch)
2011-05-16 15:15 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (18.73 KB, application/octet-stream)
2011-05-16 15:15 EDT, Frank Becker CLA
no flags Details
patch V2 (21.75 KB, patch)
2011-05-22 02:42 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (18.95 KB, application/octet-stream)
2011-05-22 02:42 EDT, Frank Becker CLA
no flags Details
fix for comment font (2.09 KB, patch)
2011-05-30 17:56 EDT, Steffen Pingel CLA
no flags Details | Diff
screenshot of the comment section (41.12 KB, image/jpeg)
2011-05-30 23:52 EDT, Frank Becker CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Becker CLA 2011-05-15 11:17:34 EDT
we need a way to show that an comment is private.

Now I do the background work and show the comments in BOLD and ITALIC Font until we did an UI review and come up with an better way to present this to the user.
Comment 1 Steffen Pingel CLA 2011-05-15 12:08:22 EDT
We shouldn't modify the comment text style itself as that will interfere with the WikiText rendering. Could we decorate the header somehow, e.g. by showing an extra icon?
Comment 2 Frank Becker CLA 2011-05-15 13:18:46 EDT
Steffen,

I only change the hyperlink text style. Is this OK?

BTW I need to change CommentViewer and this is a private class in TaskEditorCommentPart.

Should I refactor this so it is possible to override some methods in a subclass for the bugzilla package?
Comment 3 Steffen Pingel CLA 2011-05-15 13:42:49 EDT
(In reply to comment #2)
> I only change the hyperlink text style. Is this OK?

I'm not sure I understand how that is done?

> BTW I need to change CommentViewer and this is a private class in
> TaskEditorCommentPart.
> 
> Should I refactor this so it is possible to override some methods in a subclass
> for the bugzilla package?

No, I would prefer if we found a solution that didn't require modification or exposure of these classes. We want to make the comment viewers extensible but it's a larger undertaking that I don't want to commit to for 3.6.

Couldn't we simply decorate the person icon in the comment header to show that a comment is private?
Comment 4 Frank Becker CLA 2011-05-15 14:03:58 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > I only change the hyperlink text style. Is this OK?
> 
> I'm not sure I understand how that is done?
> 
> > BTW I need to change CommentViewer and this is a private class in
> > TaskEditorCommentPart.
> > 
> > Should I refactor this so it is possible to override some methods in a subclass
> > for the bugzilla package?
> 
> No, I would prefer if we found a solution that didn't require modification or
> exposure of these classes. We want to make the comment viewers extensible but
> it's a larger undertaking that I don't want to commit to for 3.6.

OK!

> 
> Couldn't we simply decorate the person icon in the comment header to show that
> a comment is private?

If we want to do this we need the bugzilla specific attribute for hold the private value in 
CommentViewer.createTitleHyperLink. 

We need something like:
TaskAttribute isprivate = taskComment.getTaskAttribute().getAttribute("isprivate");

is this OK?
Comment 5 Steffen Pingel CLA 2011-05-15 16:03:33 EDT
Yes, adding a new attribute for the private flag seems very reasonable to me.
Comment 6 Frank Becker CLA 2011-05-16 15:15:20 EDT
Created attachment 195783 [details]
patch V1

Steffen,

can you please review.

Can we do this for 3.6?
Comment 7 Frank Becker CLA 2011-05-16 15:15:23 EDT
Created attachment 195784 [details]
mylyn/context/zip
Comment 8 Steffen Pingel CLA 2011-05-16 17:18:22 EDT
The concept of the patch looks fine to me. Here are a few comments:

* The isPrivate flag should be added to ITaskComment and TaskCommentMapper to expose it in the API for other connectors.
* The font instance in createTitleHyperLink() needs to be disposed when the editor is closed. It's better to only create the font once per comment part.

Can you clean the patch a bit, e.g. revert changes from private to protected, remove commented out code, and post another iteration?
Comment 9 Steffen Pingel CLA 2011-05-16 17:19:08 EDT
One more thought, would it make sense to add a tooltip to indicate that it's a private comment?
Comment 10 Frank Becker CLA 2011-05-22 02:42:02 EDT
Created attachment 196286 [details]
patch V2

(In reply to comment #8)
> The concept of the patch looks fine to me. Here are a few comments:
> 
> * The isPrivate flag should be added to ITaskComment and TaskCommentMapper to
> expose it in the API for other connectors.
> * The font instance in createTitleHyperLink() needs to be disposed when the
> editor is closed. It's better to only create the font once per comment part.
> 
> Can you clean the patch a bit, e.g. revert changes from private to protected,
> remove commented out code, and post another iteration?

Here we go.
(In reply to comment #9)
> One more thought, would it make sense to add a tooltip to indicate that it's a
> private comment?
ToolTip is now "Private comment from {0}
Comment 11 Frank Becker CLA 2011-05-22 02:42:05 EDT
Created attachment 196287 [details]
mylyn/context/zip
Comment 12 Frank Becker CLA 2011-05-29 02:32:28 EDT
Steffen,

should i commit patch v2 for 3.6 or are we to late for this?
Comment 13 Steffen Pingel CLA 2011-05-29 21:44:55 EDT
+1 looks good to me. Please feel fee to commit.
Comment 14 Frank Becker CLA 2011-05-29 23:57:05 EDT
(In reply to comment #13)
> +1 looks good to me. Please feel fee to commit.

Done!
Comment 15 Steffen Pingel CLA 2011-05-30 17:56:09 EDT
Created attachment 196930 [details]
fix for comment font
Comment 16 Steffen Pingel CLA 2011-05-30 18:07:37 EDT
Thanks! I have committed a small fix to set the font properly on non-private comments.

Can you attach a screenshot of a task editor that shows a private comment so I can include it in the New & Noteworthy? Anything else left to do here?

The summary of the bug report states that private attachments should also be supported. Was that part of the patch? If not we should create a separate bug.
Comment 17 Frank Becker CLA 2011-05-30 23:52:39 EDT
Created attachment 196955 [details]
screenshot of the comment section
Comment 18 Frank Becker CLA 2011-05-30 23:59:29 EDT
I create bug#347718 for attachments, so we can close this!