Bug 29466 - [2004-05-03] Nodes should render HTML / formatted display names
[2004-05-03] Nodes should render HTML / formatted display names
Status: VERIFIED FIXED
Product: platform
Classification: Unclassified
Component: Nodes
3.x
All All
: P4 (vote)
: 4.x
Assigned To: _ tboudreau
issues@platform
core-promod
: API, UMBRELLA
Depends on: 41955
Blocks: 37618 11783 16339 22679 22822 29080 36026 37478 38478 41415
  Show dependency treegraph
 
Reported: 2002-12-11 15:13 UTC by Jiri Kovalsky
Modified: 2008-12-22 18:37 UTC (History)
7 users (show)

See Also:
Issue Type: TASK
:


Attachments
HTML support for explorer patch (63.59 KB, patch)
2003-08-26 13:07 UTC, _ tboudreau
Details | Diff
Example patch for javacvs module to use HTML annotations (looks pretty!) (1.61 KB, patch)
2003-08-26 13:08 UTC, _ tboudreau
Details | Diff
Test comparing swing and our HTML rendering (4.28 KB, patch)
2003-08-28 12:08 UTC, Jaroslav Tulach
Details | Diff
diff against current trunk less the htmlrenderer class itself (135.95 KB, patch)
2004-03-09 23:40 UTC, _ tboudreau
Details | Diff
org.openide.awt.HtmlRenderer (86.19 KB, text/plain)
2004-03-09 23:41 UTC, _ tboudreau
Details
More-or-less final diff (203.16 KB, patch)
2004-04-28 18:31 UTC, _ tboudreau
Details | Diff
Diff incorporating suggested changes - now a factory instead of using a shared renderer instance for everything (679 bytes, patch)
2004-05-01 15:11 UTC, _ tboudreau
Details | Diff
final (?) dif (231.75 KB, patch)
2004-05-02 12:14 UTC, _ tboudreau
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Kovalsky 2002-12-11 15:13:42 UTC
Development build #200212110100 of NetBeans 4.0
Windows 2000 with FCS build #21 of JDK 1.4.1.

Description:
============
The look of original "Basic" node in "Variables
Editor" is really ugly. This is regression and
causes automatic tests to fail.

Steps to reproduce:
===================
1. Invoke "Versioning|Mount Version
Control|Generic VCS".
2. Select CVS profile from combo box and push
"Next >".
3. Click "Edit Variables..." button.
4. Look at the first tree subnode.
Comment 1 Martin Entlicher 2002-12-11 17:02:06 UTC
What happened with the ability of Nodes to interpret html??
Comment 2 _ ttran 2003-01-07 13:31:53 UTC
Please don't rely on Nodes to display names in html.  We don't want to
use html renderer in JTree for performance reasons.  Node's display
name must be plain text.  It can be used/displayed in many places not
only in a JTree with JLabel as the cell renderer.
Comment 3 Martin Entlicher 2003-01-07 14:11:14 UTC
So, Trung, does that mean that we will not be able to have nodes of
different color? That's a pity. Don't you plan to implement some hooks
how to change at least the color? (It would be nice to be able to set
a different font as well.)
Comment 4 _ ttran 2003-01-08 10:23:09 UTC
> So, Trung, does that mean that we will not be able to have nodes of
> different color?

No, we don't plan this feature
Comment 5 Jesse Glick 2003-01-08 15:36:51 UTC
There was a long discussion of this on nbdev... of interest:

- pnejedly knows about patch which disabled HTML display names
(regression, no plans to revert)

- tboudreau started thread proposing new lighter-weight API using some
kind of rendering hints

- jglick proposed curses-like escape sequences

- tball proposed ability to override a paint method for label
Comment 6 _ ttran 2003-01-08 15:59:50 UTC
I didn't say we don't want to do it, just stated the fact that I don't
know when this will happen
Comment 7 Petr Hrebejk 2003-01-28 13:21:02 UTC
Please don't expect NetBeans to interpret html code in the Controls.
Especially for JTree (TreeView) in explorer this has significant
disadvantages in the area of performance. (The same is valid for
changing font). In the future the JTree should use LargeModel (in
order to prevent creation of all subnodes on node's expand). Using
large model requires having all items (nodes) fixed height, which is
impossible when changing fonts or rendering HTML. Some API for
changing colors would be possible but is not planned and would require
all views to interpret this.

(The bug in VCS has to be solved different way)

Changing this bug to a RFE.
Comment 8 Jesse Glick 2003-01-28 19:42:00 UTC
Please note that color changes, as well as <b>, <i>, etc., require no
change in font height and are thus compatible with a fixed label
height. I don't think anyone ever *wanted* tree node labels with
nonstandard heights. Is there no straightforward way to permit typical
HTML to be rendered but to fix the line height to that of the base
font (i.e. not support any markup which would change line height)?
Comment 9 Jesse Glick 2003-06-24 17:37:15 UTC
Probably should be reassigned to Tim, mark STARTED for 4.0.
Comment 10 _ tboudreau 2003-08-24 13:31:09 UTC
Status update:

HTML branch exists, but is fragmented - 
cvs update -f html_may2003

I will put together a patch containing its contents, making
sure it's buildable against the current trunk.

Open issue (easily resolvable, I think): How to indicate html content.
 IMO there are two solutions:

1.  Require html tag at beginning of string
2.  Provide separate method, for nodes, et. al.,
getHTMLDisplay name.  Returns null if no html version
needed.

IMO solution 2 is much more desirable.  Reasons:
 - HTML tags don't need to be included.  This is useful
for composing strings (as a composite Look will want to
do) without having to manage opening/closing HTML tags.
 - Code that can't deal with HTML doesn't have to (logging,
process output, any non-UI-oriented use case)
 - Code that *wants* to use raw HTML/XML tags (such as an
XML tree) doesn't need to do any special escaping of them
(which will cause other problems when used for process
output, etc.)

Unacceptable solution:  Just assume strings are HTML.  This
can fail in too many ways, cause problems in that display
names are not just used for UI display (a better method name
for Node.getDisplayName() might be 
Node.getHumanReadableName(), which would make the 
distinction clear.

Reasons for disliking using opening HTML tags:
 - Still requires escaping by any object that manipulates
the display name but doesn't want HTML
 - The idea of spending cpu cycles (even a few) generating
HTMLized text, and further down the pipeline, spending 
more cycles stripping those tags out is evil.  An assembly
programmer would be shot for such code;  sometimes it's
better to think like an assembly programmer.

Hrebejk, important question for you:  How should this be
handled in Looks?  

Other question:  Choice of method name?
  - getHTMLDisplayName()
or
 - getFormattedDisplayName()

I think I like getFormattedDisplayName better.  Makes it
clearer that the full HTML spec is not necessarily 
supported.
Comment 11 _ tboudreau 2003-08-24 14:18:04 UTC
Note also, new lightweight html renderer is already used in
two places: 
 - New winsys - for displaying tabs (so things like font
   face of tabs can be manipulated in order to indicate
   status)
 - Navigator module (private copy I'd like to delete)

Comment 12 _ tboudreau 2003-08-26 13:06:50 UTC
Attaching a diff with pretty much final HTML rendering code.

One thing:  The rendering methods in Utilities have 
commented out assertions to make sure they aren't called
off the AWT thread.  I couldn't rid of 1.4 source warnings
and get it to compile in openide - either we're not using
source=1.4, or there's some ant property I couldn't track
down.  When/if we can compile with assertions, these lines
should be uncommented.
Comment 13 _ tboudreau 2003-08-26 13:07:42 UTC
Created attachment 11431 [details]
HTML support for explorer patch
Comment 14 _ tboudreau 2003-08-26 13:08:34 UTC
Created attachment 11432 [details]
Example patch for javacvs module to use HTML annotations (looks pretty!)
Comment 15 Jesse Glick 2003-08-26 17:36:02 UTC
You can freely include assertions in openide code. I just added
source="1.4" to some <javac> tasks in the build script yesterday.
Comment 16 Jesse Glick 2003-08-26 18:47:11 UTC
Tim, cvs diff -u please! Contextless diffs are unpleasant to read and
unsafe to apply.

Request that the new Utilities methods be moved to a separate static
class. Yarda can give his opinion here - it seems that Utilities is
getting more and more cluttered with all sorts of unrelated stuff, and
it is very difficult to deprecate or refactor things when it is all
mashed in the same class. If there is a block of conceptually related
utility methods, they should go into their own class.

DataNode.getFormattedDisplayName says "@since 1.73" which is nonsense
- (1) should use the API spec version number, not some CVS revision;
(2) DataNode is now in the Loaders API, not the general Open APIs,
therefore there should be an independent spec version incr for
openide-loaders.jar as well as an independent entry in the change list
for that API.

Ditto FileSystem, etc. - @since tags should refer to API spec versions.

DataNode.gFDN Javadoc mentions U.renderString; should be renderHTML
(right?). At least the @see tag says rHTML.

There are outstanding "XXX" comments. Either fix whatever they refer
to, or write what is actually still broken. Don't let future
generations of programmers have to guess what is wrong with this code.

VisualizerNode.getFDN() is a new API method, right? Should have
@since, mention in apichanges. Ditto FilterNode, Node.PROP_F_D_N.
Utilities methods and constants are missing @since too - though if you
put all this stuff into one rendering utils class as I suggest, it
suffices to have just one @since in the class Javadoc.

Assertions may be uncommented, as I mentioned. Any method requiring
AWT-only access should start with

assert EventQueue.isEventDispatchThread();

Some parameters in Utilities methods missing Javadoc tags. Please run
PMD, or Tasklist's suggestions, or AutoComment, or whatever, and check
for violations.

[Javadoc only?] Numeric character entities: "&#8822;" not "&8822;".

System property 'netbeans.lwhtml.strict': (1) better to use
ErrorManager-compatible logging I think; (2) this prop must be
documented in some */arch/arch-*.xml file.

Re.:

---%<---
ErrorManager.getDefault().log(ErrorManager.WARNING,  msg);
System.err.println(msg); //Also print to stdout - ErrorManager warning
will be cut off after first /n
---%<---

Huh? ErrorManager logs are not cut off after newlines. Please delete
the serr; should not be there in production code.

Re.

String out = "Could not find color identifier in font declaration";

- need NOI18N decl. (In various other places, too.)

Use of IllegalArgumentException - fine, but mention it in the throws
clause of relevant methods, and include it in @throws tags in the
Javadoc, describing when it may be thrown.

<class package="org.openide.filesystems.FileSystem" name="HTMLStatus"/>

is forbidden syntax for apichanges (bogus package); use

<class package="org.openide.filesystems" name="FileSystem"/>

(there is no support for listing changes in nested classes).
Comment 17 Jaroslav Tulach 2003-08-28 12:08:22 UTC
Created attachment 11462 [details]
Test comparing swing and our HTML rendering
Comment 18 _ tboudreau 2003-11-20 21:09:09 UTC
Reassigning this issue to myself (mainly because I'm tired of having
to search for it when I need to update it), setting umbrella keyword
and changing to type TASK for consistency with similar issues.

It looks like we'll want a similar getHTMLDisplayName() for property
objects - they are used in places (like dialog titles) where they
shouldn't be html-ized.  Propertysheet now disables HTML for the same
reasons as Explorer.

One thought:  We seem to have a lot of patterns for ways to get a
human-readable description of a property.  To wit:
 - Node.getName() - well, not really human readable, but descriptive
 - Node.getDisplayName() - human readable name
 - Node.getHTMLDisplayName() - part of the branch
 - Node.getShortDescription()
 - Some hint (I don't remember what) for properties to supply a longer
display name for dialogs
 - Other code to get (possibly html-ized) titles for editor tabs
 - similar methods for Property objects

We have a proliferation of contexts in which different textual
descriptions of an object are required, and a corresponding
proliferation of methods to get those descriptions.

I'm wondering if it might be better to simply have some well defined
keys and a describeThyself() method, i.e.
Node.getDescription(CONSTANT_INDICATING_DISPLAY_NAME_OR_WHATEVER)
etc.  It keeps the whole thing more flexible for the long term, and
IMO is much less cluttered (and you can have reasonable fallback
mechanisms without worrying that some subclass will, say, mangle
displayName and result in a mangled HTMLDisplayName if they don't
override that).

What do you think?
Comment 19 Tomas Pavek 2003-11-25 15:22:50 UTC
I agree that we need to handle this consistently for all nodes and
properties - and that a new method should be used for providing html,
not  misusing getDisplayName() (see e.g. my last comments in issue
11783). Having parametrized getDescription method is IMO good idea.
Comment 20 _ gtzabari 2003-12-03 16:15:52 UTC
As of today this issue blocks 7 other issues. Seeing how we're
redesigning the UI, it might be a good idea to take a look at this
(especially since this issue is over a year old).

Please consider upgrading its priority.
Comment 21 _ ttran 2003-12-03 17:16:58 UTC
increasing priority doensn't help much.  It's about resource
allocation.    Please consider contributing
Comment 22 _ tboudreau 2004-01-01 23:35:26 UTC
FYI, I just created the following branch:

html_yet_again 

for core openide java vcscore vcscvs form

Will try to respond to Jesse and Jarda's comments, and get everything
pretty and working there.
Comment 23 _ tboudreau 2004-01-04 04:58:06 UTC
FYI, the branch is now functional, and there's a patch to VCSCore
which uses the controlShadow color for status annotations.  I've
migrated all of the explorer views to use it, as well as the property
sheet name and set renderers, the property sheet combo box cell editor
and the new window system tabs and popup tab selection dialog.  A
border or two may be a pixel or two off, but it's mostly tweaking left
to do - the major stuff is there.

The open question is the nature of the API changes involved.  I'm
adding two classes to the APIS:

org.openide.awt.HTMLRenderer - has the same static html painting
methods as in earlier revisions, but is also a JLabel subclass which
implements TableCellRenderer/TreeCellRenderer/ListCellRenderer, so
that anyone wanting HTML rendering for such a control can simply
override getDefaultRenderer() to return HTMLRenderer.sharedInstance()
(each call to sharedInstance reinitializes the state).

org.openide.util.Describable - a generic interface for requesting a
human readable description.  Provides one relevant method -
getDescription (int key), and defines a few constants for plain/html
description, etc., and bitmasks for indicating plaintext vs. html. 
There are three reasons to do this:

 - We don't want to be adding getHTMLDisplayName(),
getHTMLShortDescription(), then maybe later
getWeightedHTMLDisplayName, getEBDICHtmlDisplayName(), ... and
generate a lot of clutter...

 - I do think we should leave the door open for other types of
annotated/formatted strings.  In particular, this would be the right
mechanism for doing Navigator-style abbreviations - the client would
weight characters by importance, the view would figure out what it
could display and display that.  Also, we have various magical hints
like "longerDisplayName" and assorted gunk that would be better served
by a more understandable API.

 - Provides an unambiguous way to indicate exactly what the client wants.


How exactly we go about implementing it is manageable; the current
branch implements Describable directly on Node, Node.PropertySet and
Node.Property.  I'm anticipating Jarda or someone will object to that
- hence there is Describable.Provider (sort of a 1-method Look), which
could just as easily be substituted and passed one of our myriad
FeatureDescriptor subclasses.

So, it'd be good to get some feedback on the API stuff as soon as
possible.

BTW, I still need to test it to make IconView works correctly - does
anything use that these days?

Comment 24 _ tboudreau 2004-01-07 23:14:23 UTC
Well, after the interlude of issue 38412, the branch is functional
again, and is modified to reflect the feedback from nbdev, but is
postponed until after 3.6.

Comment 25 _ tboudreau 2004-01-11 16:01:17 UTC
Since there were tons of changes this week because of feature freeze,
I've updated the html branch from the trunk, so everything is up to
date and happy once again.

I've added treefs to the branch, so HTML annotations are propagated
from underlying CVS filesystems.
Comment 26 _ tboudreau 2004-03-09 23:37:18 UTC
I just brought the changes up to date with the current trunk.  I seem to have gotten some 
gunk committed to the html_yet_again branch, and my attempts to weed the gunk out ran 
smack into pretty massive "flexible build" changes in the trunk, so I'll just attach a diff with 
the current changes that I assembled against a clean check-out.

Two things need further work:

1.  (easy to fix) - The look for cell renderers is not exactly the same because the 
background extends under the icon, which is not what happens in trees in the trunk.

2.  There is an issue if a node uses HTML, but the color specified in HTML does not 
contrast well with the selection color for trees, etc.  In practice, windows classic L&F is the 
only place I know it will be a problem, since it uses a very dark blue with white text for 
selection, and black on white otherwise.  Several ways to solve this:
 - Sidestep the look and feel for all trees/lists/tables and simply make sure the selection 
color is always one close enough in brightness to the default window background that this 
will never be a problem.  
 - Extend logical colors specifiable in HTML to be something that is interpreted on the fly 
depending on the selected state of the renderer.  This means we would need colors usable 
in HTML that have special logical meaning, i.e., say, font color=d1...d4 where 1-4 are 
varying levels of deemphasis, and do something similar for other logical uses of it.

Comments?
Comment 27 _ tboudreau 2004-03-09 23:40:17 UTC
Created attachment 13915 [details]
diff against current trunk less the htmlrenderer class itself
Comment 28 _ tboudreau 2004-03-09 23:41:39 UTC
Created attachment 13916 [details]
org.openide.awt.HtmlRenderer
Comment 29 _ tboudreau 2004-03-29 00:23:52 UTC
Okay, since the old branch was fairly mangled, I'm creating a new branch - 

html_renderer  - base tag is BLD200403281800.  With any luck and some reading, 
perhaps my branch management skills have improved.  

I'll check some stuff into in the next day or so, once I resolve any issues resulting from the 
Stavbicka merge.
Comment 30 _ tboudreau 2004-04-11 16:19:26 UTC
Okay, I've improved the API of it to deal with some past objections, and gotten rid of any 
cases of subclassing.  Here's a skeleton of how it looks now.  The actual code can be 
found on the branch ui_cleanup_with_html (a subbranch merging html_renderer and 
ui_cleanup2), and soon on the html_renderer branch):

public final class HtmlRenderer {
   /** Returns a shared cell renderer instance */
   public static final Renderer sharedRenderer();

   /** Actually returns the same thing as sharedRenderer(), but as a JLabel - we have a few
      * embedded renderer cases that need this */
   public static final JLabel sharedLabel();

   public interface Renderer extends TreeCellRenderer, TableCellRenderer, ListCellRenderer 
{
        /** Indicate that the component being rendered has keyboard focus.  NetBeans 
requires that a different
         * selection color be used depending on whether the view has focus.
         *
         * @param parentFocused Whether or not the focused selection color should be used
         */
        void setParentFocused (boolean parentFocused);

        /**
         * Indicate that the text should be painted centered below the icon.  This is primarily 
used
         * by org.openide.explorer.view.IconView
         *
         * @param centered Whether or not centered painting should be used.
         */
        void setCentered (boolean centered);

        /**
         * Set a number of pixels the icon and text should be indented.  Used by ChoiceView 
and ListView to
         * fake tree-style nesting.  This value has no effect if <code>setCentered(true)</
code> has been called.
         *
         * @param pixels The number of pixels to indent
         */
        void setIndent (int pixels);

        /**
         * Explicitly tell the renderer it is going to receive HTML markup, or it is not.  If the 
renderer should
         * check the string for opening HTML tags to determine this, don't call this method.  
If you <strong>know</strong>
         * the string will be compliant HTML, it is preferable to call this method with true; if 
you want to intentionally
         * render HTML markup literally, call this method with false.
         *
         * @param val
         */
        void setHtml (boolean val);

        /**
         * Set the rendering style - this can be JLabel-style truncated-with-elipsis (...) text, or 
clipped text.
         * The default is STYLE_CLIP.
         *
         * @param style The text style
         */
        void setRenderStyle (int style);

        /** Set the icon to be used for painting
         *
         * @param icon An icon or null
         */
        void setIcon (Icon icon);

        /** Clear any stale data from previous use by other components,
         * clearing the icon, text, disabled state and other customizations, returning the 
component
         * to its initialized state.  This is done automatically when get*CellRenderer() is called.
         */
        void reset();

        /** Set the text to be displayed.  Use this if the object being rendered's toString() 
does not
         * return a real user-displayable string, after calling get**CellRenderer().  Typically 
after calling
         * this one calls setHtml() if the text is known to either be or not be HTML markup.
         *
         * @param txt The text that should be displayed
         */
        void setText (String txt);

        /**
         * Convenience method to set the gap between the icon and text.
         *
         * @param gap an integer number of pixels
         */
        void setIconTextGap (int gap);
   }

   public static String renderString (...); //same as before
   public static String renderHtml (...); //same as before
}
Comment 31 _ tboudreau 2004-04-28 18:31:43 UTC
Created attachment 14607 [details]
More-or-less final diff
Comment 32 Jaroslav Tulach 2004-04-29 06:20:26 UTC
Why this commit contains code that is "FOR DEMO PURPOSES ONLY!". I do
not like it.

org.openide.awt.HtmlLabelUI was not in original plans or am I wrong?

This should not be in the diff I guess:
+/*
+ * HtmlRenderer.java
+ *
+ * Created on January 2, 2004, 12:49 AM
+ */

Please point me to the list of usecases that justify existence of
HtmlRenderer.Renderer - btw. my desire is to eliminate that interface
so usecase with direct usage of it are not that useful, I am searching
for some higher level interfaces.

Why org.openide.awt.HtmlRendererImpl is public?

There are no tests neither for FilterNode, DataNode vs. HtmlStatus.


Comment 33 _ tboudreau 2004-04-29 09:47:03 UTC
That code will not be enabled unless a line switch is set.  It's for using font/color instead 
of * and [read only] in tab titles.  A picture being worth a thousand words, it's far better for 
someone to be able to try it than for me to tell them about it.  So what's the difference if I 
commit it now or later so people can try it?  It will have no effect unless you run with a line 
switch (I may have done the diff while it was still set automatically to true because I was 
testing it - that's exactly what was the "more-or-less" I said when I attached it).

HtmlRendererImpl should not be public.  I'll change it.

Use cases for HtmlRenderer.Renderer - why not read the diff for NodeRenderer?  The point 
of it is:

We need to supply a component which can render html, and it is typically used in tree/
table/list/combo boxes.  All of these have slightly different one method interfaces in the 
JDK.

HtmlRenderer.Renderer aggregates TreeCellRenderer, ListCellRenderer, TableCellRenderer, 
etc. into a single interface.

But there are also some things you need to do sometimes that involve manipulating 
component properties of the renderer - the icon, the icon text gap, indentation.  Two ways 
to solve this:

1.  Have three methods on HtmlRenderer - say - asTableCellRenderer() 
asTreeCellRenderer() asListCellRenderer() - and document that the result is safe to cast as 
a JLabel if you want to manipulate it as one.  That means 
   - more work for the user of it
   - it must always *be* a JLabel, but this will not be enforced by the compiler
   - the entire API of JLabel is open for the caller to abuse on the shared renderer instance

2.  Have HtmlRenderer.Renderer, and expose those things that are useful in a reasonable 
interface, and keep the flexibility to change how they are implemented or what concrete 
implementation is actually returned.


HtmlLabelUI was not in the original plan; its purpose is that if any platform comes up with 
industrial strength fast html rendering in their UI, switching over to *not* use our renderer 
is as simple as using turning off use of our custom html rendering UI delegate and using 
the default.


There will be tests for filter node before monday - that's another part of "more-or-less".
Comment 34 _ tboudreau 2004-04-29 09:49:52 UTC
BTW, one other thing this patch enables - we will finally be able to "grey out" nodes that 
have been cut to the clipboard but not yet pasted (checking each node as it's painted to 
see if it's on the clipboard is not the way - possibly something using 
FeatureDescriptor.putValue() would work fast, if done carefully, maybe there's a better 
way).  Still have to work out the mechanics of this, but it shouldn't be too hard.  Ideas 
welcomed.
Comment 35 Jaroslav Tulach 2004-04-29 12:46:08 UTC
All good except:

> But there are also some things you need to do sometimes that involve
manipulating 
> component properties of the renderer - the icon, the icon text gap,
indentation.  Two ways 
> to solve this:
> 
> 1.  Have three methods on HtmlRenderer - say - asTableCellRenderer() 
> asTreeCellRenderer() asListCellRenderer() - and document that the
result is safe to cast as 
> a JLabel if you want to manipulate it as one.  That means 
>    - more work for the user of it
>    - it must always *be* a JLabel, but this will not be enforced by
the compiler
>    - the entire API of JLabel is open for the caller to abuse on the
shared renderer instance
> 
> 2.  Have HtmlRenderer.Renderer, and expose those things that are
useful in a reasonable 
> interface, and keep the flexibility to change how they are
implemented or what concrete 
> implementation is actually returned.
> 

3. A lot of static methods, no type casting:
public static TableCellRenderer asTableCellRenderer ();
public static TableCellRenderer asTableCellRenderer (Icon icon);
public static TableCellRenderer asTableCellRenderer (..., ..., ..., ..);
I am affraid for having shared instance where one can call set methods
which might suggest that the usage is to keep reference to the
instance and call one setter on it, do something other, another
setter, etc. Which might result in random bugs due to certain foreign
code calling the setters meanwhile. While encourage the usage of the
#3 would be:
public TableCellRenderer getCellRenderer () {
  Icon icon = ...;
  String name = ...;
  return asTableCellRenderer (icon, name, ...):
}
that would imho prevent the probability of interference with foreign code.
Comment 36 _ tboudreau 2004-04-29 12:57:10 UTC
"I am affraid for having shared instance where one can call set methods
which might suggest that the usage is to keep reference to the
instance and call one setter on it, do something other, another
setter, etc. Which might result in random bugs due to certain foreign
code calling the setters meanwhile. While encourage the usage of the
#3 would be:"

The HTML renderer is restricted to use on the AWT thread; so nobody can change its state 
mid-process without accessing it from another thread, which is easy to defend against.  
Basically, you take it, you paint with it, and you're done with it.

One of the reasons for the small set of methods on Renderer is that any value set on them 
by a previous use will be reset to defaults on any call to getNNNCellRendererComponent, 
and the set of things that may be accessed are well defined, so it is easy to do this reset.
Comment 37 Jaroslav Tulach 2004-04-29 13:23:56 UTC
AWT thread restriction does not help. If you do:
rend = sharedRenderer ();
rend.setText (node.getName ());
rend.setIcon (node.getIcon ());
you are calling into foreing code and the node.getIcon() can in fact
call sharedRenderer and reset the previous value of setText. This
cannot happen in #3. Of course you can eliminate this in #2 by
gathering the info first and then calling all the setters without
using foreign code, but the API does not encourage that. That is why
the only semantically correct way how to deal with calls to foreign
code is:

// gather all the info
String name = node.getName();
String icon = node.getIcon();

// set it 
rend = sharedRenderer ();
rend.setName (name);
rend.setIcon (icon);

// return it
return rend;

which is far more complex than

return asTableCellRenderer (node.getName(), node.getIcon());

the semantically safe alternative of this in #3.
Comment 38 _ tboudreau 2004-04-29 15:38:52 UTC
And how do you then set the icon-text-gap, possibly background color, etc.  I'm afraid 
you end up with:

asTreeCellRenderer (String text, Icon icon, Boolean isHtml, int margin, int iconTextGap, 
Color background, Color foreground, Border border...)

which may be semantically safe, but not straightforward to work with, much less keep 
straight in one's head.  The current API allows you to simply call getNNNCellRenderer, as 
defined in the JDK's API and use the result.  The only time you need to intervene and set 
something is if:

 - The value object does not implement toString() to return the string you want to use

 - You want to use some different background color or something like that (by default it 
will use the tree's selection background color if selected, etc. - all the same stuff you get 
for free from DefaultTreeCellRenderer)

 - You want to adjust margin or icon text gap (we do both of these for things like ListView, 
which for whatever reason displays a tree-like indented view as a flat list)

all of which is also true for DefaultNNNCellRenderer.

I would much rather provide an easy-to-use API (by default you can just call, say, 
JTable.setDefaultRenderer (HtmlRenderer.sharedInstance()) and never think about it again 
unless you have any of the problems enumerated above).

The odds of a Node (of all things) needing a table cell renderer in order to calculate its 
display name are so astronomical that the simple answer to anyone doing that is "What the 
hell are you thinking?!"  Can you describe (I know you are imaginative!) some situation in 
which a Node would need to do that?  


Comment 39 _ tboudreau 2004-04-29 17:26:05 UTC
Note one interesting issue - tell me if this is a blocker or not.  Once this is integrated, it 
will be impossible to run the constructor of any explorer view on any thread except the 
event thread.  I can fix the test failures this causes for openide and core - no idea if there 
are other modules out there whose tests will do this.

Now, IMO, (as well expressed elsewhere), running Swing constructors on any thread but 
the EQ is just a recipe for trouble.  But finding all of the places in code that something 
might do this (I've fixed a few - mainly old actions that show a dialog with an explorer 
view, which don't override asynchronous() to return false as they should) may be hard.

I could get rid of sharedRenderer(), and let people simply get an instance of 
HtmlRenderer.Renderer, and eliminate the handful of static vars that make the rendering 
code itself not thread-safe.

Or we could treat it as welcome enforcement of reasonable practices.  What do you think?
Comment 40 Jaroslav Tulach 2004-04-29 17:30:52 UTC
The method asTableCellRenderer with so many arguments is very similar
to already existing HtmlRenderer.render (...) which takes 12. And as
it is semantically correct I do not see reason why not use it.

Imho the danger of two completely unrelated components spoiling its
paint is so big that I would like to file that as tcr, but as I seem
to be only one who cares, I'll leave it here as advice. If anybody
else wants to support my opinion, file tcr otherwise keep it as
minority opinion.

I've been just talking to Petr Hrebejk and he proposed another way how
to fix this: to have one Renderer per component. So instead of shared
instance, have factory method and keep reference to the Renderer in
the component. That would be solution to my concerns as well.
Comment 41 _ tboudreau 2004-04-29 18:10:35 UTC
Note one option for the thread issue is simply to override getPreferredSize() to return 
some meaningless value if called on a non-EQ thread, in the various explorer view classes.  

The reason this is a problem is because BasicTreeUI calls instance methods on JTree when 
called from the superclass constructor, including getPreferredSize() - so whetever thread 
it's called on, getPreferredSize() will get called in the constructor, which in turn uses the 
cell renderer to determine its preferred size (as if it would know at that point).

Comment 42 _ tboudreau 2004-04-29 20:24:24 UTC
"Imho the danger of two completely unrelated components spoiling its
paint is so big" - the only way I can think of to do this is if you, say, embed one explorer 
view as a cell renderer of another which will first configure the renderer, and *then* paint 
the other explorer views, and then try to paint the renderer.  Which is not how trees or 
tables in swing work.

A compromise that I'd find acceptable:
Keep the Renderer interface, but make it a factory interface instead.  The sharedLabel() 
method will remain, as it is a straightforward way to do a few cell renderer like things, in 
particular, calculate the dimensions required by a string as rendered by the html renderer 
without needing it to be added to a parent to do so.
Comment 43 Jesse Glick 2004-05-01 03:49:08 UTC
IMHO it is fine to limit explorer view construction to EQ if that is
the most straightforward fix; should not be hard to check for (put in
a Thread.dumpStack when it is violated and collect the bug reports).
Comment 44 _ tboudreau 2004-05-01 04:59:12 UTC
The main problem is the fairly vast number of unit tests I'd need to fix by Monday...
Comment 45 _ tboudreau 2004-05-01 15:11:27 UTC
Created attachment 14656 [details]
Diff incorporating suggested changes - now a factory instead of using a shared renderer instance for everything
Comment 46 Jesse Glick 2004-05-01 15:20:01 UTC
"The main problem is the fairly vast number of unit tests I'd need to
fix" - to run in EQ? No problem, just add

   protected boolean runInEQ() {return true;} // override NbTestCase

to each.
Comment 47 Jaroslav Tulach 2004-05-02 06:58:53 UTC
What kind of diff the last one is? I see just change in build.xml!

I am not sure about Jesse's comment "limit explorer view construction
to EQ", but it does not address my concerns at all.

This Tim's comment is much better:
> "Imho the danger of two completely unrelated components 
> spoiling its
> paint is so big" - the only way I can think of to do this 
> is if you, say, embed one explorer 
> view as a cell renderer of another which will first configure 
> the renderer, and *then* paint 
> the other explorer views, and then try to paint the renderer.  Which
> is not how trees or 
> tables in swing work.

Exactly! This is one case how things will get broken. But here another
one which is more obvious:

public String MyNode.getDisplayName () {
  return "myNode";
}
public Image getIcon (...) {
  try {
     return computeIcon (...);
  } catch (IOException ex) {
     StatusDisplayer.getDefault().setStatusText ("<html>No <b>icon</b>");
     return null;
  }
}
If the view is calling setText and then setIcon and the displayer is
using setText and you keep the broken shared instance design this will
render the node name as "No icon" and not "myNode". 


Comment 48 _ tboudreau 2004-05-02 09:39:58 UTC
Well, ignore the last diff, there will be another.  Something I did in converting it to be a 
factory and misc small changes introduced a massive performance degredation.  
Comment 49 _ tboudreau 2004-05-02 11:39:24 UTC
I'm attaching a final (?) diff.  Changes:

 - Went through the old comments, added correct @since tags, increment version of 
loaders and added apichange there, dealt with a couple other of Jesse's earlier comments 
that were still relevant

 - Html rendering is now thread safe, so explorer views can be constructed on other 
threads (this was true in the last diff as well)

 - Overrode getHtmlDisplayName() on a bunch of the filternodes displayed in the options 
tree for performance (no need to look up if they override getDisplayName())

 - VisualizerNode now caches the html display name

 - Explorer tree now handles -fontsize() correctly again (this code seems to have just 
disappeared between 3.5.1 and present), as does TreeTableView

 - Corrected a rounding error in preferred size of tree cells that only becomes visible at 
around -fontsize 30

 - Overrode addNotify() / removeNotify() in HtmlRendererImpl to do nothing (this was the 
performance problem's root)

 - HtmlRendererImpl will now parse out our custom UI manager syntax and return legal 
html if its UI is not an HtmlLabelUI, so running with Swing HTML painting really works (not 
that we want to do it)

 - A couple not directly relevant changes because of things I ran into while digging around 
for the performance problem (I know, I know, should be in a separate commit):

    - Fixed a blatant memory leak in TreeTableView (add a listener to the parent component 
in addNotify() and just leave it there)

    - Fixed the horizontal scrollbar problem in the options window

    - Fixed the background color of the property sheet description area (got mangled in the 
GTK merge)



Comment 50 _ tboudreau 2004-05-02 12:14:59 UTC
Created attachment 14659 [details]
final (?) dif
Comment 51 Jaroslav Tulach 2004-05-02 18:49:06 UTC
HtmlLabelUI is still public. createRenderer addresses my issues with
various views sharing the same component. Fine. sharedLabel does not.
Not fine from my point of view, but treat it as minority opinion. This
comment goes thru whole your code. It may seem efficient, but it is
dangerous. NodeRenderer shares static instances of renderer again, I
really do not understand why allocating new instances makes such a big
deal to compromise execution safety. Again minority opinion.

Usually overideXYZ like overrideGetDisplayName are cached, at least I
think. 

Otherwise fine, commit it so we can enjoy those little nice colored
cvs statuses...

Comment 52 _ tboudreau 2004-05-02 22:48:31 UTC
NodeRenderer was sharing static instances of the renderer before - I just changed ListView 
to be consistent with the rest of the universe;  I don't really care, just as easy to change all 
the Explorer views to keep an instance of it.

Re sharedLabel(), it's uses are extremely limited, but when it's useful it's useful.  
HtmlLabelUI should not be public, will fix.

Re caching overridesGetDisplayName, maybe a good idea; but we are caching the result of 
getHtmlDisplayName() in VisualizerNode, which means it shouldn't be called more than 
once for the lifetiime of a visualizer node (well, unless the display name changes);  that 
saves the call to FileSystem.HtmlStatus.annotateNameHtml() which would otherwise 
happen every on every paint as well;  probably that's enough unless things other than 
explorer views start calling getHtmlDisplayName.
Comment 53 _ tboudreau 2004-05-03 14:49:16 UTC
Merged.

before merge tag: trunk_before_html_merge - core openide form projects vcscore 
after merge tag: trunuk_after_html_merge - core openide form projects vcscore 
Comment 54 Marian Mirilovic 2005-12-20 15:49:21 UTC
This issue was solved long time ago. Because nobody has reopened it neither
added comments, we are verifying/closing it now. 
If you are still able to reproduce the problem, please reopen. Thanks in advance.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo