Preliminary support for dartdoc hovers. Future CLs will give us richer hover information and styl...

Preliminary support for dartdoc hovers. Future CLs will give us richer hover information and styling.
Committed: https://code.google.com/p/dart/source/detail?r=2134

[devoncarew, 2011-12-06 06:50:05]

From talking w/ Steve, this or future CLs would benefit from using
DartCompilerUtilities - a cache of ASTs. I believe that hyperlinking would also
see a performance enhancement from changing to it.

[messick, 2011-12-06 15:11:04]

lgtm

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/text/DartSourceViewerConfiguration.java
(right):

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/text/DartSourceViewerConfiguration.java:708:
public boolean isShownInText(Annotation annotation) {
What's the purpose of this method?

[Brian Wilkerson, 2011-12-06 15:15:39]

LGTM, but I think you forgot to add the interface DartDocumentable.

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java
(right):

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java:1:
package com.google.dart.tools.ui.internal.text.dartdoc;
There is nothing UI specific in this class. Please move it into the model
(unless you expect to have UI specific code in here in the future).

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java:10:
public class DartDocUtils {
nit: I like to make utility classes "final" to make the intent clear.

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java
(right):

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java:50:
DartUnit unit = editor.getAST();
Everything in this try statement below the access of the AST could be move into
DartDocUtils so that it can be accessed from other places.

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java:58:
if (element != null && element instanceof DartDocumentable) {
nit: You don't need to test for null if you're doing an instanceof test.

[devoncarew, 2011-12-06 17:55:09]

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java
(right):

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java:1:
package com.google.dart.tools.ui.internal.text.dartdoc;
On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> There is nothing UI specific in this class. Please move it into the model
> (unless you expect to have UI specific code in here in the future).

Done.

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java:10:
public class DartDocUtils {
On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> nit: I like to make utility classes "final" to make the intent clear.

Done.

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java
(right):

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java:50:
DartUnit unit = editor.getAST();
On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> Everything in this try statement below the access of the AST could be move
into
> DartDocUtils so that it can be accessed from other places.

Done.

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java:58:
if (element != null && element instanceof DartDocumentable) {
On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> nit: You don't need to test for null if you're doing an instanceof test.

Done.

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/text/DartSourceViewerConfiguration.java
(right):

http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/text/DartSourceViewerConfiguration.java:708:
public boolean isShownInText(Annotation annotation) {
On 2011/12/06 15:11:04, messick wrote:
> What's the purpose of this method?

There are several types of annotations - like DiffChanges - that are not meant
to be shown to the user. If you look at the parent's implementation you can see
how that visibility decision is made.

[messick, 2011-12-06 18:08:03]

On 2011/12/06 17:55:09, devoncarew wrote:
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
> File
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java
> (right):
> 
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java:1:
> package com.google.dart.tools.ui.internal.text.dartdoc;
> On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> > There is nothing UI specific in this class. Please move it into the model
> > (unless you expect to have UI specific code in here in the future).
> 
> Done.
> 
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dartdoc/DartDocUtils.java:10:
> public class DartDocUtils {
> On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> > nit: I like to make utility classes "final" to make the intent clear.
> 
> Done.
> 
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
> File
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java
> (right):
> 
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java:50:
> DartUnit unit = editor.getAST();
> On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> > Everything in this try statement below the access of the AST could be move
> into
> > DartDocUtils so that it can be accessed from other places.
> 
> Done.
> 
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartTextHover.java:58:
> if (element != null && element instanceof DartDocumentable) {
> On 2011/12/06 15:15:39, Brian Wilkerson wrote:
> > nit: You don't need to test for null if you're doing an instanceof test.
> 
> Done.
> 
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
> File
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/text/DartSourceViewerConfiguration.java
> (right):
> 
>
http://codereview.chromium.org/8817009/diff/1020/editor/tools/plugins/com.goo...
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/text/DartSourceViewerConfiguration.java:708:
> public boolean isShownInText(Annotation annotation) {
> On 2011/12/06 15:11:04, messick wrote:
> > What's the purpose of this method?
> 
> There are several types of annotations - like DiffChanges - that are not meant
> to be shown to the user. If you look at the parent's implementation you can
see
> how that visibility decision is made.

The new comment helps. Thanks!