New log manager since rev. 4606

[thomas]

I'm afraid that I probably won't be able to fix this. I'm unable to follow the modifications to the original code and the reasoning behind them. Not only are there a lot of them, but since half of the code was moved to another file, I can't use diff or blame to see which lines were modified either, so that makes it tedious.

Some of the differences I found are:
SetupGUILogging() is now called before the plugins are loaded. Logger controls are no longer maintained by the application, but are created by hand, either by sending messages or even worse by calling CreateControl manually (see TODO plugin). The behaviour of that function has been changed too (don't know if that is a problem, but it's different).

Trying to find out where the debug log has gone, I went stepping in the debugger. CreateControl() on the Logger in slot 3 (debug log) returns a null pointer, which causes the application to (correctly) ignore it. I don't know why this happens, but apparently somewhere during startup, the TextContolLog is replaced with a NullLog?

[mandrav]

I'm afraid that I probably won't be able to fix this. I'm unable to follow the modifications to the original code and the reasoning behind them. Not only are there a lot of them, but since half of the code was moved to another file, I can't use diff or blame to see which lines were modified either, so that makes it tedious.

Code has not been changed.
And the move to a different file was done to avoid recompilation of the entire repository each time we want to add a comma or a space.

Some of the differences I found are:
SetupGUILogging() is now called before the plugins are loaded. Logger controls are no longer maintained by the application, but are created by hand, either by sending messages or even worse by calling CreateControl manually (see TODO plugin). The behaviour of that function has been changed too (don't know if that is a problem, but it's different).

SetupGUILogging moved there because in the place it was in the original code, no startup log was retained (logs were empty).
I don't understand "controls are not maintained by application". They are. But SDK messages were added so plugins can add/remove loggers at run time (in contrary to only being able to do this at init time).
Also, first step was to make everything compile properly. Old code will slowly be replaced by newer more compact code in time. So the single hack you noticed in todo list is not worth the mention.

Trying to find out where the debug log has gone, I went stepping in the debugger. CreateControl() on the Logger in slot 3 (debug log) returns a null pointer, which causes the application to (correctly) ignore it. I don't know why this happens, but apparently somewhere during startup, the TextContolLog is replaced with a NullLog?

I found out why, although I don't understand it.
Seems like Manager::Get()->GetLogManager() and LogManager::Get() return two different instances...
Specifically, in Mgr<MgrT>::Get(), a new instance is created the first time the two above calls are made (totalling 2 instances).

[thomas]

Code has not been changed.
And the move to a different file was done to avoid recompilation of the entire repository each time we want to add a comma or a space.

Code has been changed. I found an if(!control) and was sure that I had not put it there. Doing a manual compare with a revision a week old indeed showed that it wasn't there. Now, I don't know if it really matters, but it changes the behaviour of the function from "always create" to "create only if not exist". The reason why I had no such check in the original code was that it should remove one constraint (pay attention to not create duplicate objects) from people who contribute Loggers, and move that constraint into the application. Very likely it works the same now, but the behaviour is different.
I am aware why you moved the code from a header to a source file, and that is perfectly ok... but... since you did this together with some other changes, one cannot see what those changes were, other than by comparing line by line manually. svn diff doesn't show anything useful in that case.

I don't understand "controls are not maintained by application". They are. But SDK messages were added so plugins can add/remove loggers at run time (in contrary to only being able to do this at init time).

What I was saying was that now a dozen different components decide when they want to add a log, instead of this being decided by the application. There is no harm if this is only done by the application in my opinion.
The worst thing to happen is that a user won't have a log after manually loading a plugin until he starts the application for the next time (it will still work in every other respect). That "missing" log is a very minor issue (actually none at all) but it removes a huge deal of complexity and possible errors from the whole system, and makes it a lot easier to follow (for me, at least). But... never mind that.

Seems like Manager::Get()->GetLogManager() and LogManager::Get() return two different instances...
Specifically, in Mgr<MgrT>::Get(), a new instance is created the first time the two above calls are made

Oh, it's worse... it looks like that's the case with all managers, only so far nobody ever noticed
I don't know why it happens either, it should not be possible, but it is apparently the case.

[thomas]

This is an interesting problem.

We have
type* f() { return g(); };
and we have
f()  != g().

This is beyond my understanding of C++ (or mathematics, or anything). I don't see g() return anything but the exact same thing every time.

[mandrav]

This is an interesting problem.

We have
type* f() { return g(); };
and we have
f()  != g().

This is beyond my understanding of C++ (or mathematics, or anything). I don't see g() return anything but the exact same thing every time.

I know why, because it happened to me before (in a different project).
You see, you instantiate the template's "instance" variable in the template header. But this means that different compilation units will create different static vars (because templates are instantiated in the calling unit).

The solution is to remove the static vars instantiation from manager.h and move them in each manager's compilation unit (as template specializations):

Code: "from manager.h"

// remove those:

template<class MgrT>MgrT* Mgr<MgrT>::instance = 0;
template<class MgrT>bool  Mgr<MgrT>::isShutdown = false;

Code: "for example, top of logmanager.cpp"

template<> LogManager* Mgr<LogManager>::instance = 0;
template<>bool Mgr<LogManager>::isShutdown = false;

Doing this will make everything work as expected. You can try it, if you want .

[MortenMacFly]

Doing this will make everything work as expected. You can try it, if you want .

I can't believe it... Yiannis, you solved a major issue in a software I'm maintaining here at work. I had a singleton class as member variable inside another singleton class. The object got never initialised properly and I did an ugly hack to get it to work. But I was facing *exactly* that issue. You are the hero.
(Although it's really weired that nobody out there noticed this with all those millions of singleton template example code...)

[thomas]

Hmm... can we simply move the instance into a .cpp file (one)? If I understand the problem right, that should work.

[mandrav]

Hmm... can we simply move the instance into a .cpp file (one)? If I understand the problem right, that should work.

It probably would work, yes. But what would be the point? You 'd still have to #include all the managers there. So better to put each one in its own compilation unit (the appropriate manager's .cpp). It's just two lines of code.

I have already created the patch and sent it over to Morten for testing on windows. If all goes well, I 'll commit it.

[mandrav]

I have already created the patch and sent it over to Morten for testing on windows. If all goes well, I 'll commit it.

Tests were successful and changes committed .

[dje]

Hi all !

Feature request 3879 :
I need my log window to be removed from the logger without being destroyed.

In InfoPane class, we have:

Code

int InfoPane::AddLogger(Logger* logger, wxWindow* p, const wxString& title, wxBitmap* icon)
int InfoPane::AddNonLogger(wxWindow* p, const wxString& title, wxBitmap* icon)
bool InfoPane::DeleteLogger(Logger* l)
bool InfoPane::DeleteNonLogger(wxWindow* p)
bool InfoPane::RemoveNonLogger(wxWindow* p)

User benefit from the following events for adding/removing logs:

Code

EVT_ADD_LOG_WINDOW
EVT_REMOVE_LOG_WINDOW

I'd like:
- bool InfoPane::RemoveLogger(wxWindow* p) to be implemented
- the EVT_DELETE_LOG_WINDOW to be implemented
- the EVT_REMOVE_LOG_WINDOW event call InfoPane::RemoveLogger instead of InfoPane::DeleteLogger
- the EVT_DELETE_LOG_WINDOW event call InfoPane::DeleteLogger

I can do it if accepted.

Dje

[thomas]

If you want a function to hide a log tab, then this could be implemented in InfoPane, if you can give a good reason.
InfoPane does have a Show() member already to allow you to bring a tab into the foreground, if something crucial really has to be shown and put in the foreground (for example a critical error, or search results after a lengthy search). A Hide() member was deliberately omitted because it is generally not your decision what logs are shown, but the user's (selectable from the popup menu).

If you want to do word by word what you said ("my log window to be removed from the logger") then it gets complicated.
LogManager takes Loggers (with GUI or without), and owns them.
InfoPane asks (on behalf of LogManager) all Loggers to provide a GUI control (which they can do, or not). If a Logger does provide a control, InfoPane takes ownership and adds it to the notebook.
Giving up ownership on either of the two may be problematic, and I don't see a real urgent need for it.

You probably want this functionality for the ThreadSearch plugin, right? If so, then using a Logger is not the best solution anyway. You are not logging anything, so you shouldn't be using a Logger.

InfoPane has a function to add non-loggers to its tab list, too. This function takes anything derived from wxWindow, it does not require a Logger, nor does it own a Logger or install one into LogManager, nor does it delete one. That function was added to give the possibility to add something into the log area which actually does not belong there (as it is no log), but still belongs there somehow (as people are used to find it there).

[dje]

if you can give a good reason

Well, we'll have to discuss what makes a reason to be good  but:
Previous MessageManager worked like that.
No access to InfoPane is provided (and I think it's a better design choice).
I'd like to transfer ownership of my control from Messages notebook to the Layout on the fly. From a user point of view, I find very BORING the "Change will take place at next application start". And I find more pleasant being able to use Messages notebook or Layout management for the plugin without restarting the application.

That is why I need a removal without deletion.

You probably want this functionality for the ThreadSearch plugin, right? If so, then using a Logger is not the best solution anyway. You are not logging anything, so you shouldn't be using a Logger.

Yes, I need it for this plugin to keep the same behaviour. If it is not the right place for it, is it the place for the "Search results" tab ?
That's the same feature except there is a preview window and a non blocking search.

InfoPane has a function to add non-loggers to its tab list, too. This function takes anything derived from wxWindow, it does not require a Logger, nor does it own a Logger or install one into LogManager, nor does it delete one. That function was added to give the possibility to add something into the log area which actually does not belong there (as it is no log), but still belongs there somehow (as people are useNonLogger,d to find it there).

I saw it and I agree but no events exist to access it. And I would need a RemoveNonLogger, not a DeleteNonLogger call.
I tried to reparent my ThreadSearchView to avoid the parent window to destroy my wxWindow but as the wxWindow is referenced in a structure and an explicit Destroy is called, there is no workaround.

Nevertheless, I find that, in the present implementation, EVT_REMOVE_LOG_WINDOW is confusing.
That is reenforced by InfoPane implementation with its Remove and Delete methods.

Dje

[thomas]

No access to InfoPane is provided (and I think it's a better design choice).

There is no real access to InfoPane, at least not one that is to be used without good reason. The idea behind the new LogManager and the InfoPane is that LogManager runs regardless of any GUI capabilities (in fact, without any capabilitites at all, not even console I/O or file access). InfoPane is a GUI class that builds a bridge to a GUI, in this case the notebook window that we see on the bottom. If InfoPane isn't there, LogManager still works the same (as for example when doing a batch build). Thus, no special branches are needed for this, everything works the same.
I think this is a better design, but surely opinions can differ on that.

For you, LogManager, Logger, and anything related to them actually does not matter. You don't really log anything. Thus, whatever happens with Loggers really isn't so important, since you would really want to add a non-Logger.

And I find more pleasant being able to use Messages notebook or Layout management for the plugin without restarting the application.  That is why I need a removal without deletion.

That is no good reason. You should be using a non-Logger (even if you derive your panel from a Logger-subclass, you can still add it as non-Logger).

I saw it and I agree but no events exist to access it.

Yiannis probably just forgot to add those, since there are very few valid uses for them anyway. But... it's not like it isn't supported

And I would need a RemoveNonLogger, not a DeleteNonLogger call.

This exists.

[dje]

I totally agree on the fact I should use a non-logger but it is impossible for now as there no access  to the infopane (direct or events).
That's why I chose the logger way that makes me add useless code to comply with available interfaces.
Whatever (non)logger it is, it is systematically destroyed and that is my real problem.

I totally agree this evolution makes software design cleaner but something is broken or missing because i can not do any more what was possible.

Before, there was a mix between LogManager and MessagesManager.
First step is reached, the log part is OK.
Second step is partially reached because developer looses a lot of possibilities.

As there is a wxWindow* Logger::CreateControl method which is triggered for window creation, it could be nice to have a OnRemoveControl which could veto or tell if wxWindow must be removed or destroyed. It could be an alternative solution to a new event type.

As you wrote, lots of what I need is available but some bridges are missing to link them.
So, what, who, when ?

Dje

[dje]

Hi all !

In sdk_events.cpp

Code

CodeBlocksLogEvent::CodeBlocksLogEvent(wxEventType commandType, wxWindow* window, const wxString& title, wxBitmap *icon)
	: wxEvent(wxID_ANY, commandType),
	logger(0), logIndex(logIndex), icon(icon), title(title), window(window)
{
	logger = Manager::Get()->GetLogManager()->Slot(logIndex).GetLogger();
}

logIndex auto affectation gives out of range value (98443884 according to GDB).

my code:

Code

CodeBlocksLogEvent evt(cbEVT_REMOVE_LOG_WINDOW, m_pThreadSearchView);
Manager::Get()->ProcessEvent(evt);

Crash

BerliOS bug : 12700

Thanks for non deletion of non logger window in cbEVT_REMOVE_LOG_WINDOW event management in 2nd December build !

Dje