[rfc][icedtea-web] rewritten java console

[Andrew Azores]

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!
>
> J.
>
> On 11/25/2013 12:22 PM, Jiri Vanek wrote:
>> No brave soul to check the concept? tss :)
>>
>> Dont blame me later ;)
>>
>> J.
>>
>> On 11/21/2013 07:17 PM, Jiri Vanek wrote:
>>> Hi all, after week of work there is complete rewrote of java console.
>>> The motivation was that it was ugly and useless.
>>> With new logging, when plugin is logging to its file, java aprt is 
>>> logging into its file, when one cen easily redirect stderr/out to 
>>> separate chanels and when one can set verbosity and headers in 
>>> itw-settings or by environment variable.
>>> With system logging on the way (although I did not yet decided how 
>>> to do it), I believe java console should have some additional value. 
>>> Maybe we can hunt applets developers with advertisement "we have 
>>> better console" :)
>>>
>>> What I was misisng most, were missing plugin messages. So most of 
>>> this patch is focused on including them. Other parts are focused on 
>>> colour and setupable, powerfull console.
>>> In meanwhile I stumbled over few bugs or incomplete features, so 
>>> they are also included in this patch. I know i crossed the limits of 
>>> review, and so after some general comments I will push in smaller 
>>> steps.
>>>
>>> Generally in this patch:
>>> - java console can receive plugin messages
>>> - this was implemented by *new* pipe at the end. I tried various 
>>> approaches (file, sockets, shared pipe) and this one had the best 
>>> performance
>>> - new pipe is now third parameter to jvm lunch
>>> - console itself have highlight, and search and filtering capabilities
>>> - advantage is, that when its hidden, or shown and debug of, then it 
>>> have nearly no overhead. Debug on and visibel one can have some 
>>> overhead, but nothing crucial (unless you set some terrible 
>>> filters/settings)
>>> - plugin si now logging to file
>>> - log dir can be set in itw-settings (the deployment key was already 
>>> in itw, so Ijust added textfield)
>>> - the logs now have human readable name
>>>
>>> Small bugfixes and improvements
>>> - the pies are now in XDG_RUNTIME_DIR - or in temp as they were if 
>>> it is not setup
>>> - headers (for c) are generated by shared function
>>> - teh tmp path search is does in separate function
>>> - headers in java are represented by object, and changed to string 
>>> when it have sense
>>> - fixed few newline terminators in PLUGINDEBUG/ERROR macros call
>>>
>>> and unluckily much more :)
>>>
>>>
>>>
>>> the "changelog" is like this:
>>>
>>> * netx/net/sourceforge/jnlp/controlpanel/DebuggingPanel.java:
>>> - added jtextfield and button "reset to default" to set 
>>> KEY_USER_LOG_DIR
>>>
>>> * netx/net/sourceforge/jnlp/resources/Messages.properties:
>>> - fixed and added few entries, but whole ConsoleOutputPane is 
>>> waiting to be added
>>>
>>> *netx/net/sourceforge/jnlp/util/logging/ConsoleOutputPane.java:
>>> - is repalcement for old "twoplain text stdout/err" panels
>>> - it have higlighlt, can sort, can filter, can search.. well apply 
>>> and see;)
>>> - is folowing observer/obsevable patter, and is updating smoothly
>>> -- code hint --
>>> It is updating the lines, not regenrating all of them (unless 
>>> necessary), however there is still an performance critical part, I 
>>> do joptionpane.setText(srting) in all ways, and imho the parsing of 
>>> html is here the worst.
>>> I was trying to add text or whatever.. but no imprvement. Maybe 
>>> because underlying document.insertString is creating new string 
>>> anyway :)
>>> Maybe I will swap to jtextpane in next iterations
>>> -------------
>>> - most of the code are adding some listeners or layout fighting, so 
>>> dont be scared ;)
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/FileLog.java:
>>> - created human readable, alphabet-sortable name
>>> - much better constructor handling
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/JavaConsole.java:
>>> - removed old palintext stdout/err textareas
>>> - added reader of new pipe, started only if pipe is presented
>>> - messages from pluginare parsed to header and message
>>> - java messages are already recieved as header and body
>>> instead of remove textares, it now contains one (by default, can 
>>> have endelss of them) ConsoleOutputPane
>>> - update of ConsoleOutputPane is doen by observable,observer pattern
>>> - all ConsoleOutputPane updates are disabled when console is hidden
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/LogConfig.java:
>>> - config made accessible , ConsoleOutputPane is setting default 
>>> appearence based on them
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/OutputController.java
>>> - fixed isOutput, isError aproach (from static tomember), added 
>>> isWarnig,isDebug,isNotDebug
>>> - adapted to work with object Header instead of string header
>>> - console is reciving headers always (but is showing them on demand)
>>> - header code moved to new class (Header:)
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/headers/Header.java:
>>> - simple obkject representtig header
>>> - is requesting
>>> - String getMessage();
>>> - Header getHeader();
>>>
>>> netx/net/sourceforge/jnlp/util/logging/headers/MessageWithHeader.java
>>> - shared interface for JavaMessage and PluginMessage
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/headers/JavaMessage.java:
>>> - wrapper around Header and string message body
>>>
>>>
>>> * 
>>> netx/net/sourceforge/jnlp/util/logging/headers/ObservableMessagesProvider.java:
>>> - abstraction for data source for ConsoleOutputPane
>>> - the only implementation is JavaConsole
>>> - enforcing
>>> - List<MessageWithHeader> getData();
>>> - Observable getObservable();
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/headers/PluginHeader.java
>>> - extending Header
>>> - adding some plugin specific constants, preintint mark, and addindg 
>>> original timestamp and preinti flag
>>>
>>> * netx/net/sourceforge/jnlp/util/logging/headers/PluginMessage.java
>>> - second mplementation of MessageWithHeader
>>> - have factory method to parse source string (parts of header and 
>>> message)
>>>
>>>
>>> * plugin/icedteanp/IcedTeaNPPlugin.cc:
>>> - adding new pipe for debug messages->java
>>> - gchar* debug_pipe_name
>>> - the messages are buffered in FIFO (declared in 
>>> IcedTeaPluginUtils.cc), but access to this fifo ave to be 
>>> sinchronised - pthread_mutex_t debug_pipe_lock
>>> - GIOChannel* debug_to_appletviewer - channel around pipe
>>> - bool plugin_debug_to_console = true; - flag initialised form 
>>> deployment-properties, log is used to decide whther create pipe, and 
>>> whether to write to it,whether to initalisated buffer, and so on...
>>> - creating (and cleaning in case of failure) logic in ititialisation 
>>> - if plugin_debug_to_console is true, then pipe for debug messages 
>>> is intialised. A bit later the thread reading buffer is stared
>>> - if debug to console is true, path to debug pipe is passed to jvm
>>> - two mwthods
>>> - plugin_send_message_to_appletviewer_console and 
>>> flush_plugin_send_message_to_appletviewer_console to opearte the pipe.
>>> - they do not log to prevent deadlock
>>> - adding logging into file possibility
>>> - std::string plugin_file_log_name - name of file to save the 
>>> plugin's part messages/*errors
>>> - FILE * plugin_file_log; - stream to write to file
>>> - eg the getMimeDesc or similar, are called *without* plugin 
>>> initialisation. Or as separate nstance orhow to say it. So each call 
>>> to them (mostly one or two per browser run) is creating a log file. 
>>> But I would call it feature :)
>>> - clenup of pipe and log fiel in shutdown time
>>> - various fixes of new line at end of _MESSAGES_
>>> - generation of tmp dir path moved to std::string 
>>> IcedTeaPluginUtilities::getTmpPath() (and used by std::string 
>>> IcedTeaPluginUtilities::getRuntimePath())
>>>
>>>
>>> * plugin/icedteanp/IcedTeaNPPlugin.h:
>>> declared extern public definitions of pthread_mutex_t 
>>> debug_pipe_lock, plugin_debug_to_console; plugin_file_log; 
>>> plugin_file_log_name;jvm_up; 
>>> plugin_send_message_to_appletviewer_console and 
>>> flush_plugin_send_message_to_appletviewer_console
>>>
>>> * plugin/icedteanp/IcedTeaParseProperties.cc:
>>> - added logic to read deployment.console.startup.mode and 
>>> eployment.user.logdir (or to provide default) (each read only once 
>>> during initialisation)
>>>
>>> * plugin/icedteanp/IcedTeaParseProperties.h
>>> - declared above
>>>
>>> * plugin/icedteanp/IcedTeaPluginUtils.cc
>>> - declared buffer to store preinit messages and to buffer runtime 
>>> messages - std::queue<std::string> pre_jvm_message
>>> - flush_pre_init_messages body of thread which is poping from queuq
>>> - push_pre_init_messages - sinchronised push to queuq
>>> -- code hint --
>>> I "synced" the thread via :
>>> + struct timespec ts;
>>> + ts.tv_sec = 1;
>>> + ts.tv_nsec = 0;
>>> + nanosleep(&ts ,0);
>>> afaikit is not good :( But behave greate for me. But I have little 
>>> experience withc synchroisatin i c/c++ :(
>>> ----------------
>>> - IcedTeaPluginUtilities::initFileLog method to create path to file 
>>> elog and open the stream
>>> - initConsoleFile (and initErrFile with initOutFile) relicts from 
>>> time when plugin messages were sen tto java side via files :)
>>> - IcedTeaPluginUtilities::generateLogFileName - generating human 
>>> redable log name - same as javaside, just with different prefix
>>> - IcedTeaPluginUtilities::printDebugStatus() - method to log current 
>>> status of debug settngs
>>> - IcedTeaPluginUtilities::getTmpPath() :)
>>> - IcedTeaPluginUtilities::getRuntimePath( returning value of 
>>> XDG_RUNTIME_DIR ot getTmpPath if XDG_RUNTIME_DIR is not set. USed to 
>>> create pipes destinations
>>>
>>>
>>> * plugin/icedteanp/IcedTeaPluginUtils.h:
>>> - nicer initialisation of log subsystems
>>> - CREATE_HEADER jsut cosmetic changes
>>> - PLUGIN_DEBU and ERROR are reusing the header, added psh to buffer 
>>> and log to file
>>> - declared new IcedTeaPluginUtilities methods
>>>
>>> * plugin/icedteanp/java/sun/applet/PluginMain.java:
>>> - printed out imput arguments
>>> - handled third one (debug pipe name)
>>>
>>> * plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java*
>>> omented out repeated endless "woken consumer" message - it was still 
>>> spamming the outputs
>>>
>>> * tests/cpp-unit-tests/IcedTeaPluginUtilsTest.cc :
>>> - partially considered existence of console logs
>>>
>>> * 
>>> tests/netx/unit/net/sourceforge/jnlp/util/logging/JavaConsoleTest.java:
>>> - added some tests for parsing the plugin message
>>
>

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.

- ConsoleOutputPane(ObservableMessagesProvider) constructor has a 
variable named "mathces", maybe this should be "matches"?
- ConsoleOutputPane#refreshPane and ConsoleOutputPane#update, 
formatting/bad indentation
- 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.
- ConsoleOutputPane#importList, variable naming could be improved again. 
"l" -> "messageList", "m" -> "message" ? Also, maybe extract the hex 
colour codes out into constants?
- JavaConsole#createPluginReader, indentation again, also there's a typo 
in an OutputController.log() statment ("conosle")
- IcedTeaPluginUtils.cc - IcedTeaPluginUtilities::initFileLog is using a 
flag "O_WRONLY". Should this be "O_WRONGLY" or something?
- ConsoleOutputPane#initComponents - can you make a factory method for 
ActionListeners or something? 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.
- 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

Thanks,

-- 
Andrew A