Opened 11 years ago

Last modified 10 years ago

#8345 closed

wxLogBuffer extensions

Reported by: frm Owned by:
Priority: low Milestone:
Component: Version:
Keywords: Cc: frm, vadz
Blocked By: Blocking:
Patch: yes

Description

This patch adds two new classes which I think are very useful.

I really found the need for them in some complex apps which use wxLogError/Warning in a lot of places.

The implementation of the wxLogArrayString class required to implement also wxArrayString::Join/Split functions which I also think are indeed very useful.

Attachments (2)

wxlogstring.patch download (5.1 KB) - added by frm 11 years ago.
wxlogstring2.patch download (6.4 KB) - added by frm 10 years ago.
updated version

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by vadz

Sorry, but I have a lot of problems with this patch and I really don't think we can apply it in its current state:

First of all, I don't quite see how is wxTrapOnLog better than just putting a breakpoint in wxLogError (as I usually do).

And while wxLogArrayString is useful I'd rather merge it with wxLogBuffer and not provide GetArray() method but just the accessor functions.

Finally, Join() and Split() are useful but I wouldn't make members of wxArray as they could also apply to wxList &c. And Split() doesn't seem to take care of escaping the separator which is really necessary.

But please let's discuss this on wx-dev before doing anything about it, thanks.

comment:2 Changed 11 years ago by frm

First of all, I don't quite see how is wxTrapOnLog better than just
putting a breakpoint in wxLogError (as I usually do).

Good point. I don't remember why I felt the need for such class since you must be running the debbugger anyhow. Probably just because wxLog functions are implemented through macros and I was lazy and didn't want to go and find the right file for placing the breakpoint.

However now that I reconsider it, I see that wxTrapOnLog is not very useful/sensed :)

while wxLogArrayString is useful I'd rather merge it with wxLogBuffer

ok

and not provide GetArray() method but just the accessor functions.

hmmm, that is provide only the GetString() accessor?
But then if the user wants the log messages as a wxArrayString, how could it get the array?

Join() and Split() are useful but I wouldn't make members of
wxArray as they could also apply to wxList &c

see the mail to the wx-dev

Split() doesn't seem to
take care of escaping the separator which is really necessary.

I don't understand this: why should Split() "escape" the separator?
And how and where should the separator be escaped?

But please let's discuss this on wx-dev before doing anything about it,
thanks.

done now.

Changed 11 years ago by frm

comment:3 Changed 11 years ago by frm

Reposted the patch with modifications discussed earlier. However now this patch depends on:

http://sourceforge.net/tracker/index.php?func=detail&aid=1638950&group_id=9863&atid=309863

for wxJoin() function.

File Added: wxlogstring.patch

comment:4 Changed 10 years ago by frm

Hi,

I now remember while I didn't merge wxLogArrayString with wxLogBuffer since the beginning: the problem is that when you want to collect the log messages and then, instead showing them to the user through wxMessageBox(), retrieve them from the log target as a wxString, you'll do something like:

wxLogBuffer *newl = new wxLogBuffer();
wxLog *oldl = wxLog::SetActiveTarget(newl);


/* here call some function which uses wxLog* functions */

wxLog::SetActiveTarget(oldl);
wxArrayString arr = newl->GetArray();

unfortunately if wxLogBuffer::Flush() methods shows the log messages to the user using wxMessageBox (as it currently does), the code above won't work wxLog::SetActiveTarget(oldl); will call Flush() on the old log target resulting in a messagebox shown to the user and an empty wxArrayString returned by GetArray() later.

I'd say the only solution is to:

1) keep two different wxLogArrayString and wxLogBuffer classes (the second should be private and undocumented however) - just as first version of the patch did.

The wxLogArrayString::Flush() function shouldn't do anything. And the user can clear the internal buffer using a wxLogArrayString::CleanBuffer() function.

2) provide in wxLogBuffer a CleanBuffer() function which can be used to clean the internal buffer without showing the contents to the user. Then the code above would become:

wxLogBuffer *newl = new wxLogBuffer();
wxLog *oldl = wxLog::SetActiveTarget(newl);


/* here call some function which uses wxLog* functions */

/* get logged msg and clean them (so that Flush()ing later

won't show them to the user) */

wxArrayString arr = newl->GetArray();
newl->CleanBuffer();

wxLog::SetActiveTarget(oldl);

however this second solution requires the user to know that wxLogBuffer::Flush() shows the messages using wxMessageBox... (ok, I agree this is not such big problem).

Changed 10 years ago by frm

updated version

comment:5 Changed 10 years ago by frm

I've attached a new version of the patch:

  • it does rename wxLogBuffer to wxLogOutputBest (since it uses wxMessageOutputBest to Flush messages) and reimplements the wxLogBuffer as a log target which logs messages to a wxArrayString (i.e. reimplemented to work just as wxLogArrayString - after all I think wxLogBuffer name is better for such class).
  • documents it

File Added: wxlogstring2.patch

comment:6 Changed 10 years ago by vadz

Looking at this anew I can't help wondering what is the point of this patch, in fact?

AFAICS it just adds GetArray() accessor to wxLogBuffer and there are no other changes, is this right? Why do we need to split wxLogBuffer in 2 classes?

Also, I have a problem with changing GetBuffer() return type: this silently breaks existing code doing something like this:

const wxChar *p = logbuf.GetBuffer();
puts(p);

as the temporary object is now destroyed before "p" is used. I'm afraid we really need to keep wxString inside this class.

comment:7 Changed 10 years ago by frm

Looking at this anew I can't help wondering what is the point of this
patch, in fact?

to be able to retrieve as wxArrayString or as wxString (the only two reasonable types which can be used for this scope) the log of the messages.

AFAICS it just adds GetArray() accessor to wxLogBuffer and there are no
other changes, is this right?

well, not completely: it also adds a different wxLogBuffer::Flush() implementation

Why do we need to split wxLogBuffer in 2

classes?

one is for retrieving the logged messages as wxString/wxArrayString, the other is for logging them using wxMessageOutputBest...

Also, I have a problem with changing GetBuffer() return type: this
silently breaks existing code doing something like this:
...

is this code in wxCVS?

IIRC I tested the patch and it worked well (i.e. did not break compilation)...
OTOH if the "broken" code lives outside of wx, then it should not be a big issue since wxLogBuffer was undocumented and thus used only by very few peoples presumably...

In conclusion I think: currently there's no way (unless rewriting wxLogBuffer obviously) to get logged messages in form of wxArrayString or wxString (theis last is because wxLogBuffer was not documented)... this patch fix this hole :D

comment:8 Changed 10 years ago by vadz

I'm reluctant to change the type of wxLogBuffer::GetBuffer() precisely because it does *not* break compilation. This is the worst sort of change as the existing code will compile but then will crash at run-time.

I also dislike the Flush() which doesn't flush but just clears the buffer -- what's wrong with adding a separate Clear() method for this? Losing the log messages in Flush() is a bad idea.

So let me suggest this:

  1. Add new class wxLogBufferedOutput with a pure virtual wxString GetBufferContents() method and Flush() implemented using wxMessageOutputBest::Output(GetBufferContents())
  1. Derive wxLogBuffer from it and keep its GetBuffer() as is otherwise
  1. Derive wxLogArray (as you initially wanted) from it too

I think this should make everybody happy, do you agree?

comment:9 Changed 10 years ago by frm

Hmm, I think your proposal is not perfect because:

1) wxLogBuffer is a bad name for something which actually outputs the messages to the user using wxMessageOutputBest (and not simply storing it internally as the name would suggest)

2) wxLogBufferedOutput seems a bad name, too. In fact, the presence of the "Flush" function in the wxLog base class means that any wxLog implementation is buffered, isn't it? In fact a Flush() function on a non-buffered class wouldn't make much sense... isn't it?

3) Why would be the change to wxLogBuffer::GetBuffer() backward incompatible? With last patch it now returns "wxString" instead of "const wxString&". Thus it should be backward compatible.

Also I don't think backward compatibility should be respected at all since wxLogBuffer is undocumented.

comment:10 Changed 10 years ago by vadz

  1. But this is how it already is. So the only alternative is to change it in incompatible way and I don't think it's a good idea. Of course, I don't think it's such a bad name personally as for me it should be understood as "log target which stores messages in a buffer until it can be output using wxMessageOutput"
  1. Flush() for non-buffered stream wouldn't do anything. Anyhow, as this would be a new class I'd take another name if you can propose anything better too.
  1. As I explained, this would *silently* break existing code doing "const wxChar *p = log.GetBuffer()". This is the worst type of breakage and I really don't see why do we have to break it when there are perfectly good ways to avoid it. I'm ready to break compatibility when it's worth it and it can't be avoided but here it can, and hence it should, be avoided.

comment:11 Changed 10 years ago by frm

  1. this would *silently* break existing code doing "const

wxChar *p = log.GetBuffer()"

I've tried to add in minimal sample the following (after applying last version of the patch):

wxLog::SetActiveTarget(new wxLogBuffer);
wxLogMessage(wxT("hello world"));

const wxChar *p = NULL;
{

p = wx_static_cast(wxLogBuffer*, wxLog::GetActiveTarget())->GetBuffer();

}
printf("%s", p);

and it works (i.e. shows "hello world" on the console) so it seems that

const wxChar *p = log.GetBuffer()

is not broken by this patch... even if I sincerely don't know why :/

comment:12 Changed 10 years ago by vadz

Well, it doesn't *have* to not work -- the behaviour of this program is undefined (which means that it only fails when you don't expect it). The p pointer still points to a buffer belonging to an already destroyed temporary object and the fact that it works sometimes doesn't prove anything...

comment:13 Changed 10 years ago by sf-robot

This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).

Note: See TracTickets for help on using tickets.