This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 218850 - GIT-/Mercurial show history: align user and date label to the right
Summary: GIT-/Mercurial show history: align user and date label to the right
Status: RESOLVED FIXED
Alias: None
Product: versioncontrol
Classification: Unclassified
Component: Code (show other bugs)
Version: 7.3
Hardware: PC Windows Vista
: P4 normal (vote)
Assignee: Ondrej Vrabec
URL:
Keywords: NETFIX
Depends on:
Blocks:
 
Reported: 2012-09-22 23:01 UTC by markiewb
Modified: 2012-11-01 02:40 UTC (History)
0 users

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
chaotic layout of names and dates (39.63 KB, image/png)
2012-09-22 23:02 UTC, markiewb
Details
Contributed patch / alignment of user and data (30.05 KB, patch)
2012-10-07 22:39 UTC, markiewb
Details | Diff
Screenshot showing the patch in action (120.97 KB, image/png)
2012-10-07 22:40 UTC, markiewb
Details
wrong behavior on Nimbus laf (58.58 KB, image/png)
2012-10-08 12:03 UTC, Ondrej Vrabec
Details
Contributed patch / alignment of user and data / with nimbus laf and reformat fix (35.38 KB, patch)
2012-10-10 19:42 UTC, markiewb
Details | Diff
Contributed patch / alignment of user and data / with nimbus laf and reformat fix / with bounding rect translation fix (47.12 KB, patch)
2012-10-26 16:06 UTC, markiewb
Details | Diff
patch proposal (43.91 KB, patch)
2012-10-29 14:04 UTC, Ondrej Vrabec
Details | Diff
Patch based on http://netbeans.org/bugzilla/attachment.cgi?id=126725 (42.96 KB, patch)
2012-10-29 19:33 UTC, markiewb
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description markiewb 2012-09-22 23:01:27 UTC
[ BUILD # : 201209210001 ]
[ JDK VERSION : 1.6.35 ]

When using the show history view with GIT/Mercurial in conjunction with 
branches and commiter name of different length the summary view looks very 
chaotic.

ACTUAL:
commiter name and dates are not aligned - see screenshot

EXPECTED:
Align the user label and date to the right (this way it looks more like a table

and it gives a better overview)
Comment 1 markiewb 2012-09-22 23:02:29 UTC
Created attachment 124758 [details]
chaotic layout of names and dates
Comment 2 markiewb 2012-10-07 22:39:08 UTC
Created attachment 125555 [details]
Contributed patch / alignment of user and data

I want to contribute this patch to fix the issue.

See the screenshot for the result.

I had to do some refactoring to achieve goal.
* split the one textpane into multiple textpanes
* introduced a pane "SummaryCellRenderer.RevisionItemCell" which layouts the textpanes for revision, comment, author, date
* introduced methods for style creation (create*Style)
* introduced methods for adding texts to separate textpanes (f.e. addRevision, addDate, addAuthor) 

(Another note for future development: the internal linkerSupport-API is very nasty to use in combination with JTextPanes. Any rewrite planed?)
Comment 3 markiewb 2012-10-07 22:40:14 UTC
Created attachment 125556 [details]
Screenshot showing the patch in action
Comment 4 Ondrej Vrabec 2012-10-08 12:03:21 UTC
Created attachment 125576 [details]
wrong behavior on Nimbus laf
Comment 5 Ondrej Vrabec 2012-10-08 12:15:30 UTC
thanks for looking into it. Several quick notes:
OV1: take a look at the attached screenshot. There's a problem with Nimbus laf, you removed the WA for Nimbus:
> textPane.setOpaque(false);
> textPane.setBackground(new Color(0, 0, 0, 0));
and forgot to set this for all the newly added text panes.

OV2: there is a wrong formatting in the patch. I can see unnecessary use of {} blocks, e.g. addRevision() plus you sometimes use more spaces for indentation (8 spaces instead of usual 4, see again addRevision() - its 3rd line). Please fix that

OV3: true, the linker support pseudo API is difficult to use and in your case it does not work. Download and install Team plugin (support for java.net and netbeans.org integration), register netbeans.org as a team server and open search history for a file from a netbeans clone. While clicking on revisions i'm getting plenty of exceptions:
> SEVERE [org.openide.util.Exceptions]
> javax.swing.text.BadLocationException: Position not represented by view
>         at javax.swing.text.CompositeView.modelToView(CompositeView.java:275)
>         at javax.swing.text.BoxView.modelToView(BoxView.java:484)
>         at javax.swing.plaf.basic.BasicTextUI$RootView.modelToView(BasicTextUI.java:1509)
>         at javax.swing.plaf.basic.BasicTextUI.modelToView(BasicTextUI.java:1047)
> [catch] at org.netbeans.modules.versioning.util.VCSHyperlinkSupport$AuthorLinker.computeBounds(VCSHyperlinkSupport.java:315)
>         at org.netbeans.modules.versioning.util.VCSHyperlinkSupport.computeBounds(VCSHyperlinkSupport.java:161)
>         at org.netbeans.modules.versioning.history.SummaryCellRenderer$RevisionRenderer.paint(SummaryCellRenderer.java:720)
>         at javax.swing.CellRendererPane.paintComponent(CellRendererPane.java:151)
Comment 6 Ondrej Vrabec 2012-10-08 12:17:09 UTC
> (Another note for future development: the internal linkerSupport-API is very
> nasty to use in combination with JTextPanes. Any rewrite planed?)
No rewrite planned, sorry. Not until it goes public (i guess it never will) or we realize it does not suit our needs.
Comment 7 markiewb 2012-10-10 19:42:21 UTC
Created attachment 125718 [details]
Contributed patch / alignment of user and data / with nimbus laf and reformat fix

(In reply to comment #5)
> thanks for looking into it. Several quick notes:
> OV1: take a look at the attached screenshot. There's a problem with Nimbus laf,
> you removed the WA for Nimbus:
> > textPane.setOpaque(false);
> > textPane.setBackground(new Color(0, 0, 0, 0));
> and forgot to set this for all the newly added text panes.

fixed.

> OV2: there is a wrong formatting in the patch. I can see unnecessary use of {}
> blocks, e.g. addRevision() plus you sometimes use more spaces for indentation
> (8 spaces instead of usual 4, see again addRevision() - its 3rd line). Please
> fix that

Reformatting done. There were refactoring artefacts.

> OV3: true, the linker support pseudo API is difficult to use and in your case
> it does not work. Download and install Team plugin (support for java.net and
> netbeans.org integration), register netbeans.org as a team server and open
> search history for a file from a netbeans clone. While clicking on revisions
> i'm getting plenty of exceptions:
> > SEVERE [org.openide.util.Exceptions]
> > javax.swing.text.BadLocationException: Position not represented by view
> >         at javax.swing.text.CompositeView.modelToView(CompositeView.java:275)
> >         at javax.swing.text.BoxView.modelToView(BoxView.java:484)
> >         at javax.swing.plaf.basic.BasicTextUI$RootView.modelToView(BasicTextUI.java:1509)
> >         at javax.swing.plaf.basic.BasicTextUI.modelToView(BasicTextUI.java:1047)
> > [catch] at org.netbeans.modules.versioning.util.VCSHyperlinkSupport$AuthorLinker.computeBounds(VCSHyperlinkSupport.java:315)
> >         at org.netbeans.modules.versioning.util.VCSHyperlinkSupport.computeBounds(VCSHyperlinkSupport.java:161)
> >         at org.netbeans.modules.versioning.history.SummaryCellRenderer$RevisionRenderer.paint(SummaryCellRenderer.java:720)
> >         at javax.swing.CellRendererPane.paintComponent(CellRendererPane.java:151)

Could not fix it yet. It is very hard to do because there is code which maps the mouse click location to the inner location model of the jtextpane. Now the textpanes are wrapped and positioned within panels. So the origin of the location is not clear. There must be some location translating regarding the topmost parent, but therefore i had to alter some interfaces which will break the API... arghh...
Comment 8 Ondrej Vrabec 2012-10-11 09:56:48 UTC
>> OV3
>Could not fix it yet. It is very hard to do because there is code which maps the mouse click location to the inner location model of the jtextpane. Now the textpanes >are wrapped and positioned within panels. So the origin of the location is not clear. There must be some location translating regarding the topmost parent, but >therefore i had to alter some interfaces which will break the API... arghh...
right, to correctly fix this a friend API change is required, computeBounds would require a relative component against which the positioned should be calculated as you're doing in ExpandMsgHyperlink.computeBounds. I don't see this as a big problem and could help you with changing the API, i know what modules use it and i could help you finish it. However i would rather change it in the next development cycle since now being in the stabilization period we could easily break something and this close to code freeze there wouldn't be enough time to test it.

Or you could solve it the same way as in ExpandMsgHyperlink. Extend IssueLinker and AuthorLinker, override computeBounds and use whatever logic to identify the correct position. Several protected final methods will need to be added to IssueLinker and AuthorLinker. Not nice but i guess it'll work.

OV4
MessageTooltip hyperlink is incorrectly positioned, should be handled the same way as ExpandMsgHyperlink - should be displayed only over the commit message
Comment 9 markiewb 2012-10-19 11:21:35 UTC
(In reply to comment #8)
> >> OV3
> >Could not fix it yet. It is very hard to do because there is code which maps the mouse click location to the inner location model of the jtextpane. Now the textpanes >are wrapped and positioned within panels. So the origin of the location is not clear. There must be some location translating regarding the topmost parent, but >therefore i had to alter some interfaces which will break the API... arghh...
> right, to correctly fix this a friend API change is required, computeBounds
> would require a relative component against which the positioned should be
> calculated as you're doing in ExpandMsgHyperlink.computeBounds. I don't see
> this as a big problem and could help you with changing the API, i know what
> modules use it and i could help you finish it. However i would rather change it
> in the next development cycle since now being in the stabilization period we
> could easily break something and this close to code freeze there wouldn't be
> enough time to test it.
> 
> Or you could solve it the same way as in ExpandMsgHyperlink. Extend IssueLinker
> and AuthorLinker, override computeBounds and use whatever logic to identify the
> correct position. Several protected final methods will need to be added to
> IssueLinker and AuthorLinker. Not nice but i guess it'll work.
> 
> OV4
> MessageTooltip hyperlink is incorrectly positioned, should be handled the same
> way as ExpandMsgHyperlink - should be displayed only over the commit message

I am currently working on this. It looks good until now.
But one question: How get the Issue-Hyperlinks activated? I could not get them activated.
Comment 10 Ondrej Vrabec 2012-10-19 11:48:38 UTC
(In reply to comment #9)
> But one question: How get the Issue-Hyperlinks activated? I could not get them
> activated.
IssueLinker highlights issue ids in a commit message and paints them as hyperlinks (in blue and underlined). As long as you have Jira or Bugzilla installed and enabled the hyperlinks should be highlighted by default. The usual issue id pattern is: issue #123456. So commit a file and add e.g. "Issue #123" to the commit message.
Or didn't i understand your question and you wanted to know something else?
Comment 11 markiewb 2012-10-26 16:06:45 UTC
Created attachment 126635 [details]
Contributed patch / alignment of user and data / with nimbus laf and reformat fix / with bounding rect translation fix

(In reply to comment #10)
> As long as you have Jira or Bugzilla
> installed and enabled the hyperlinks should be highlighted by default. The
> usual issue id pattern is: issue #123456. So commit a file and add e.g. "Issue
> #123" to the commit message.
> Or didn't i understand your question and you wanted to know something else?
Thank you. That was the answer to my question. Sorry for the delayed response - i was on vacation, but now another try/patch.

Changes to the previous patch:
a) to every linker a field of RevisionItemCell was added
b) every linker knows in which textpane it is nested (see the top of computedBounds())
c) the translation-logic (the calculation of the correct bounds for nested textpanes) is in RevisionItemCell.correctTranslation()

No public API-Changes are neccessary and the annotation bar tooltips (which also use the linkers) still work. The code is made backward-compatible via conditional checks for the reference component - see the linker classes.


Further enhancements: 
* shows the message tooltip, only if the commit message has more than one line AND if it is not fully visible (see org.netbeans.modules.versioning.history.SummaryCellRenderer.RevisionRenderer.addCommitMessage() - ca. line 534 if (nlc > 0 && !item.messageExpanded))
* the message tooltip is shown immediately (see AbstractSummaryView)
* removed flickering while hovering over a link (see AbstractSummaryView)

Open issue:
* FYI: i have not tried the IssueLinker linking (because the bugzilla plugin 1.30 in my current dev build cannot be enabled) - but the code is there

@ovrabec: Can you have a look at the new patch please!? 

Thank you, markiewb
Comment 12 Ondrej Vrabec 2012-10-29 13:55:18 UTC
> Further enhancements: 
> * shows the message tooltip, only if the commit message has more than one line AND if it is not fully visible (see org.netbeans.modules.versioning.history.SummaryCellRenderer.RevisionRenderer.addCommitMessage() - ca. line 534 if (nlc > 0 && !item.messageExpanded))
> * the message tooltip is shown immediately (see AbstractSummaryView)
> * removed flickering while hovering over a link (see AbstractSummaryView)
Let's stick to "one problem = one issue". Please remove these modifications from this patch, file another bug and attach the patch there. It is better when modifications deal with just one problem, it is easier to understand the fixes and changesets. Thanks.

> a) to every linker a field of RevisionItemCell was added
it is fine for linkers within SummaryCellRenderer, but i don't like it in VCSHyperlinkSupport class. The linker API is relatively independent and you're introducing a cyclic dependency on another package. No code in VCSHyperlinkSupport should depend on org.netbeans.modules.versioning.history.* classes. I think a better solution would be to define an interface inside VCSHyperlinkSupport (and implement it in RevisionItemCell). So what about:
public static interface BoundsTranslator {
    public Rectangle correctTranslation (Container startComponent, Rectangle r);
}
defined in VCSHyperlinkSupport and:
class RevisionItemCell implements BoundsTranslator { ... } in org.netbeans.modules.versioning.history.RevisionItemCell ?

> b) every linker knows in which textpane it is nested (see the top of computedBounds())
it is really confusing, linkers get a textPane as a parameter and then throw it away and start using a completely different instance. It is difficult to read and i doubt we will understand it when we go back to the code six month later. My suggestion is:
1) add another method to Author and Issue linkers that expects the final textpane (message, date, user etc.) and the translator and delegate the current method to it:
@Override
public void computeBounds(JTextPane textPane, BoundsTranslator translator) {

}

@Override
public void computeBounds(JTextPane textPane) {
    computeBounds(textPane, null);
}

2) the caller of computeBounds should determine the correct textpane to use and call the appropriate method:
RevisionRenderer class {
@Override
public void paint(Graphics g) {
    super.paint(g);
    AuthorLinker author = linkerSupport.getLinker(AuthorLinker.class, id);
    if (author != null) {
        author.computeBounds(revisionCell.getAuthorControl(), revisionCell);
    }
    IssueLinker issue = linkerSupport.getLinker(IssueLinker.class, id);
    if (issue != null) {
        issue.computeBounds(revisionCell.getCommitMessageControl(), revisionCell); 
    }
    ExpandMsgHyperlink expandMsg = linkerSupport.getLinker(ExpandMsgHyperlink.class, id);
    if (expandMsg != null) {
        expandMsg.computeBounds(revisionCell.getCommitMessageControl(), revisionCell);
    }
    ...
}
Comment 13 Ondrej Vrabec 2012-10-29 14:04:54 UTC
Created attachment 126725 [details]
patch proposal
Comment 14 Ondrej Vrabec 2012-10-29 14:05:56 UTC
> Created attachment 126725 [details]
> patch proposal
- demonstrates usage of coputeBounds(textPane, boundsTranslator)
- not finished, still requires change in MsgTooltip and ExpandMessage linkers
Comment 15 markiewb 2012-10-29 16:45:09 UTC
(In reply to comment #14)
> > Created attachment 126725 [details]
> > patch proposal
> - demonstrates usage of coputeBounds(textPane, boundsTranslator)
> - not finished, still requires change in MsgTooltip and ExpandMessage linkers

@ovrabec: Do you want me to continue the work? Or do you want to?
Comment 16 Ondrej Vrabec 2012-10-29 16:47:59 UTC
(In reply to comment #15)
> @ovrabec: Do you want me to continue the work? Or do you want to?
You should continue, i just committed a modified version of your patch to demonstrate my ideas in comment #12.
Comment 17 markiewb 2012-10-29 19:33:44 UTC
Created attachment 126747 [details]
Patch based on http://netbeans.org/bugzilla/attachment.cgi?id=126725

(In reply to comment #16)
> (In reply to comment #15)
> > @ovrabec: Do you want me to continue the work? Or do you want to?
> You should continue, i just committed a modified version of your patch to
> demonstrate my ideas in comment #12.

Done. 
* Removed the "enhancements".
* added logic for the MsgTooltipLinker/ExpandMessageLinker (most work was already done by you, not much left to do for me)

Please have a look at patch now.
Comment 18 Ondrej Vrabec 2012-10-30 11:45:51 UTC
thanks for the patch, it seems fine now
Comment 19 Ondrej Vrabec 2012-10-30 11:52:18 UTC
fixed: http://hg.netbeans.org/core-main/rev/c62c6e876673
Comment 20 Quality Engineering 2012-11-01 02:40:03 UTC
Integrated into 'main-golden', will be available in build *201211010001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/c62c6e876673
User: Benno Markiewicz <markiewb@netbeans.org>
Log: Issue #218850 - GIT-/Mercurial show history: align user and date label to the right