Re: [path] refactored ReplPane.enter() / cleanups

[Per Bothner <per at bothner dot com>]

On 04/11/2011 10:01 AM, Charles Turner wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> To get acquainted with the Kawa development cycle, I thought I'd submit
> a patch. I'll apologise in advance if this is the wrong format.
>
> The attached patch addresses a FIXME in the ReplPane class, to move the
> the enter() method over to ReplDocument. In addition, the focusGained()
> method of ReplDocument and the ReplPane constructor have been modified.
> The change message for Subversion should probably be "refactored enter()".

Thanks!  Some comments:

The most important thing missing is a ChangeLog entry.  I usually write that
myself when I get a contribution, but in principle it should be included
with the patch.  (Actually, to avoid merge failures, it's better to have it
separate, but in practice it's unlikely to an issue either way.)

The @Eoverride annotations for inner classes seems indented wrong - it is
certainly different from what emacs (23.2.1) does.

The comment "implement a volatile variable that keeps track of when the
thread should stop" doesn't really seem like a solution: What if you're
executing an infinite loop?  Generating a test of a volatile variable
at each "sequence point" (e.g. before looping back) seems like an
unacceptable performance killer.

I don't think changing 'kind == ReplPane.ViewableElementName' to
'kind.equals(ReplPane.ViewableElementName)' is needed unless Swing
behind-the-scenes makes a copy of the incoming String.  I guess in
theory Swing might do that (perhaps internally Swing might use UTF-8
byte arrays - like javac does), and the code is not performance-critical,
so it may be reasonable to use equals.
--
	--Per Bothner
per@bothner.com   http://per.bothner.com/