Created attachment 20794 [details]
The changes in core to delegate to Logger

Not only this patch shall allow implementation of issue 35067, but it shall speedup 
isLoggable as well, as the implementation of isLoggable in logging is very likely 
better than the one we have in NbErrorManager. 

Use of "utf-8" in Main.java might not be right... better to use a PrintWriter to
begin with, then no need to translate char -> byte -> char.

I wonder if NbEvents could use java.util.logging.* directly, in the intended
way, with a special formatter plugin to translate the "events" into the same
format we see now. Would be a good exercise.

Created attachment 23126 [details]
Updated to current version of core

s/NbFormater/NbFormatter/g


new File(String) should use File.separatorChar.


ErrorManager.UNKNOWN severity ought not map to anything: it is not a legal value
for the actual level of any exception or message. It just means that the default
severity should be used; or, when annotating, not to change the current level.
At least that is how I understood it.


Would the revised logger still add [catch] markers as now?


The NbTopManager.java patch looks bogus.


Why is NbErrorManagerTest.testLog broken? Anything important?


NbEvents should use the formatter the way it was intended. I.e. rather than

logger.log(Level.INFO, NbBundle.getMessage(NbEvents.class, "TEXT_patch",
f.getAbsolutePath()));

use

logger.log(Level.INFO, NbBundle.getMessage(NbEvents.class, "TEXT_patch"),
f.getAbsolutePath());

which would delay the formatting until (and unless) the message is actually printed.


We might also consider making the formatter automatically set the ResourceBundle
on LogRecord's it sends to formatMessage to be NbBundle.getBundle(name), where
name is somehow attached by the Logger according to its own name. Not sure
exactly how it should work. (Properly, the NbErrorManager instance should set
the Logger's ResourceBundle when it is *created*, but it seems that the Logger
factory methods only permit you to set a name to pass to
ResourceBundle.loadBundle, whereas we need to ask NbBundle - maybe an API bug to
be reported.) This would let us write e.g.

logger.log(Level.INFO, "TEXT_patch", f.getAbsolutePath());

which would be even nicer. Note however that the current impl of formatMessage
uses MissingResourceException for control flow, which is not good (and perhaps
also a bug to report).


If someone uses the Logging API directly, will it do any of our nice stuff like
[catch] etc.? I guess not. Basically this patch means that usage of ErrorManager
will delegate to the JDK's logging impl, but not vice versa, right? Maybe we
should consider a different approach: do the EM -> Logger delegation in
ErrorManager.java (in the default openide impl), remove NbErrorManager (so the
default ErrorManager impl is used in NB unless someone registers a different
one), and register a special Handler from core. (Or a LogManager? I am a bit
unclear on how the logging SPI works.) That way

- from unit tests or lib-based programs, you could use either Logger or
ErrorManager interchangeably, and get the default JDK output formatting

- from inside the complete NB app (i.e. core loaded), you could also use either
Logger or ErrorManager interchangeably, but get enhanced output (messages.log,
[catch], automatic NbBundle localization, whatever)

- we might need to copy some concepts from the JDK logger to configure log
levels for different components, etc.; or just keep our current system property
configuration, rather than use logging.properties

Does this make any sense?

More work needed. 

Created attachment 27903 [details]
Sketch of new approach - instead of System.err we log using Logger & NbErrorManager is going to implement such logger

It is priority for me to get this into codebase as that will "standardize" our   
logging APIs and also allow change in NbTestCase to support collecting of logs   
from failed tests which is too painful right now and nobody except me knows  
how to do it.   
 
The previous patch shows how things are going to look like. If there is no 
error manager in lookup to delegate to we are going to use logger. The plan is 
that NbErrorManager is no longer going to implement error manager, instead it 
will implement java.util.logging.Handler and collects logs from both sources - 
error manager as well as directly thru logging API. 
 
The NbTestCase is going to have new method protected boolean collectLogs() 
{ return false; } that will be called from runTest and if true, the test 
registers a Logger that will print info into NbTestCase.getLog(). 

Excellent.

apichanges.xml: <summary> typo.

Double-check usage of Level.INFO vs. Level.FINE; I am guessing that our
ErrorManager.INFORMATIONAL ~ Level.FINE. (Should not be displayed unless
explicitly requested.) But not sure; check default settings of Logger.

Would probably also want to deprecate the ErrorManager constructor to alert
anyone subclassing it that they should no longer do so (and check NB code base
incl. unit tests for anyone doing it).

Created attachment 27910 [details]
Addition of NbTestCase.logLevel() to simplify capturing of LogRecords

Created attachment 27921 [details]
ErrorManager, NbErrorManager, NbTestCase jumbo patch

> apichanges.xml: <summary> typo. 
 
Done. 
 
> Double-check usage of Level.INFO vs. Level.FINE; I am guessing that our 
> ErrorManager.INFORMATIONAL ~ Level.FINE. (Should not be displayed unless 
> explicitly requested.) But not sure; check default settings of Logger. 
 
I rewrote NbErrorManagerTest and from it it seems that INFORMATIONAL 
definitely maps to FINE for log messages. However for exceptions it does not, 
as those need to be logged all the time. Right now I map them to CONFIG (just 
handy value, no philosophical reason to do so). 
 
 
> Would probably also want to deprecate the ErrorManager constructor to alert 
> anyone subclassing it that  
 
 
I've deprecated the whole class. The last necessary method, useful from 
ErrorManager was annotate(Throwable, localizedString). Actually LogRecord has 
support for localizations so it was just a matter of attaching LogRecord and a 
Throwable. Done by Lookup.Provider. Works. Maybe a support method to annotate 
a throwable needed somewhere, probably in Utilities, the contract is not the 
nicest one to expose it to module writers. It would make them laugh (or cry). 
 
> they should no longer do so (and check NB code 
> base incl. unit tests for anyone doing it). 
 
I can check the code, but tests use ErrorManager a lot and I have no great 
interest in rewriting them until we really decide to cleanup all usages of 
ErrorManager in all NetBeans sources. A lot of work. 
 
 
So I think I am ready to integrate. The current date is Dec31,2005 as 
apichanges do not allow year 2006 yet. Patch sort of works. I'll just need to 
document two additional APIs:  
1. throwables that implement Lookup.Provider which can contain LogRecords 
2. layer folder Logging which can contain a configuration property files which 
can configure logging priorities - the old System.getProperties() API is hard 
to support with JDK's logging.  
 
Comments? 

Please don't rush with this issue.  We don't know what and when the next release
will be.  Refrain from any API changes until we'll be done with 5.0 and know
better about the plan for 5.1.

Rather than do "the first step" I'd like to attempt to solve issue 35067 in full
for the next release.  Full requirement and API review required.

Thanks

Right, exceptions (notify) and messages (log) do not really use EM levels the
same way - hence the (notorious?) -J-DErrorManager.minimum=17. Historical oddity.

NbErrorManager.java - needs to close InputStream's once opened (or does
SequenceInputStream do this?).

Is there any reason to order children under Logging/? If so, need to use
DataFolder.gC().

I'm not sure removing system properties as a configuration mechanism is a good
idea. It's often helpful to enable logging on a particular component during
development, unit tests, or when diagnosing a bug (including asking a user to
attach a log with certain diagnostics enabled). It's pretty easy to turn logging
today with -J-Dxxx=0 - in fact easier than with the JDK's logging, since there
is no need to create a special config file - but I don't see an easy way to
temporarily turn on logging using the proposed layer entries. What do you think
we should do about this? Is it possible to e.g. have
LogManager.readConfiguration also load some kind of config from system
properties, e.g.

-J-Dxxx.level=100

?

Re. "needs to close InputStream" - SequenceIS does that. But anyway in issue   
35067 I've just admited that properties can be used and the layer folder may   
not be needed at all.   
   
To Trung: Yes, you are right, the last patch indeed needs full review. I've   
just provided it to show that the previous patch for ErrorManager is in the   
right direction. I've turned the issue 35067 into the API review request.   
   
However I have few open bugs that depend on an improved support for logging in   
unit tests. I do not like open bugs and would like to integrate the changes   
into NbTestCase without waiting. Could that stay the fasttrack? It does not   
influence the behaviour of running NetBeans application at all. I am talking  
about this patch:  
http://www.netbeans.org/nonav/issues/showattachment.cgi/27910/LoggingInNbTestCase.diff  
  
Re-adding fast track for this last change in NbTestCase. Remove again, if that 
is not acceptable.  
 

Please wait. I don't want *any* API changes in the trunk before we know better about the plan for 5.5 and 
6.0.  This should happen first half of January.

(I removed "_FAST")

Created attachment 29553 [details]
Using jackpot rule + few fix imports to fix usage of err for logging in AutoUpdate

Created attachment 29602 [details]
commit log

Integrated.