[rfc][icedtea-web] rewritten java console

[Jiri Vanek]

On 12/06/2013 10:01 PM, Andrew Azores wrote:
> On 12/03/2013 08:08 AM, Jiri Vanek wrote:
>> As one-before-last "make java console aware of plugin messages" is on review, I think I can send
>> also the new console to parallel review (applied on top of it)
>> As advertisement there is screenshot attached. (by default jsut one, not filtered output is shown)
>>
>> Main change is new file ConsoleOutputPane.java with its logic in ConsoleOutputPaneModel.java -
>> which together are fast and powerful and *nice* new console.
>> JavaConsole.java - got rid of it original outputs, and instead of them it have  one (with possible
>> multiplication) ConsoleOutputPane. Individual messages are no longer strings, but list of
>> messgae+header. The underlying logic is actualised on observer/observable
>> JavaConsole.java is still responsible for static methods like showing/hiding/factories or entrance
>> methods like addMessage or processPluginMessage.
>>
>> Lets it serve well!
>>
...
>>>
>>
>
> Okay, took me quite a while to review all this, and I think it really does deserve a double review
> anyway. Rather than trying to put my comments inline with the patch, which I think would be pretty
> difficult to read, I'm just going to copy the notes I took into here, and specify class/method
> names, etc.

thanx!

>

ConsoleOutputPane x ConsoleOutputModel  - was messed together during review. I hope I had assigned 
issues correctly (mostly 100% sure)

> - ConsoleOutputPane(ObservableMessagesProvider) constructor has a variable named "mathces", maybe
> this should be "matches"?

yes :( fixed
> - ConsoleOutputPane#refreshPane and ConsoleOutputPane#update, formatting/bad indentation

hopefully reformatted.

> - ConsoleOutputPane#preSort - need more descriptive variable names than "l". Also, could there be
> some kind of refactoring done here? There's a big case statement and each case is almost identical
> to the last, just accessing a different field on a variable.

yy, refactored

> - ConsoleOutputPane#importList, variable naming could be improved again. "l" -> "messageList", "m"
> -> "message" ? Also, maybe extract the hex colour codes out into constants?

done

> - JavaConsole#createPluginReader, indentation again, also there's a typo in an
> OutputController.log() statment ("conosle")

This goes to different patch which ptisnovs is reviving

> - IcedTeaPluginUtils.cc - IcedTeaPluginUtilities::initFileLog is using a flag "O_WRONLY". Should
> this be "O_WRONGLY" or something?

nn!! it is Write ONLY and so is correct.

> - ConsoleOutputPane#initComponents - can you make a factory method for ActionListeners or something?

hehe. Nice catch!

> There are a *lot* of identical anonymous inner classes being used here. Also, it seems to me that at
> line 615 of rewrittenConsole.patch, we have one incredibly long line of code... :)
>
> General comments:
> - Can the ConsoleOutputPane not be subdivided into more components? It feels very large and
> monolithic as it is. I'd hate for this to become JNLPClassLoader 2.0.

oh no..... I'm not fan of gathered components. What is bloated here is just gui composition. I'm in 
favour to let it as it is.  Also it looks like you reviwe the merged patch - sorry for confusing 
you! In newest, there is separated model and gui.

> - I kind of gave up on nitpicking about formatting and spelling when I was taking notes :( there
> were quite a few places where indentation seemed wrong, or member variables were all being defined
> at the bottom of a class, etc., and it's just inconsistent.
> - Some of the C++ work I wasn't entirely sure of what was going on, so I refrained from commenting
> on that too much

Ptisnovs is on C part. So should be ok.

This is only java part  -The C waits to ptisnovs, but this can go in separately.

Thanx for check!
   J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rewrittenConsole3.patch
Type: text/x-patch
Size: 72850 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131209/6b6385fe/rewrittenConsole3-0001.patch