Author Topic: wxFlatNotebook update + Patch (Read 21180 times)

MortenMacFly · « **on:** May 05, 2006, 08:31:21 am »

Dear devs,
since I'm using wxFlatNotebook in a project myself I realised that C::B uses a rather old version (0.90) of the library. I've updated my C::B to the current version (1.30) for quite some time now and I've also adopted the patch for C::B to the new version which I provide with this message.
So basically I'd like to say: On my machine wxFlatNotebook v1.30 works very well, the required patch is attached.
With regards, Morten.
Edit: Removed patch to save forum space. It has been applied already.

thomas · « **Reply #1 on:** May 05, 2006, 09:36:58 am »

Could you provide another patch please?

I tried applying this one with TortoiseMerge, which did not work.
Then I tried gnu patch, and all I got was:
patch: **** Only garbage was found in the patch input.
Editing patchfiles by hand is nasty business, especially when there are several files with several chunks each.

Just cd to the wxFlatNotebook folder and type svn diff > patchname.patch, that works out fine (or even easier, right-click onto the folder if you have Tortoise installed, and select "create patch").

MortenMacFly · « **Reply #2 on:** May 05, 2006, 11:13:22 am »

Quote from: thomas on May 05, 2006, 09:36:58 am

Could you provide another patch please?

Well, I guess I didn't made myself clear enough. The patch is not to be applied to the sources of C::B. This patch has to be applied to the sources of wxFlatNotebook, version 1.30. So to update C::B you would have to do the following:
1.) Download wxFlatNotebook v1.30
2.) Copy the sources into C::B overwriting the onces there (which are v0.90)
3.) Apply my patch to these sources.
Why did I do this? Because that's the way how the patch for v0.90 is provided in C::B SVN.
If I provide a file to patch the sources fom C::B SVN (v0.90) directly to v1.30_patched that this would be a huge file and noone could understand what changes actually were made to the original v1.30 version.
With regards, Morten.

Edit: Of course I could do that, too... give me some time... ;-)

MortenMacFly · « **Reply #3 on:** May 05, 2006, 11:28:32 am »

...here it comes: This patch should update the current SVN head to wxFlatNotebook v1.30_patched. This will also update the patch file so that in the end C::B is updated to v1.30, patched and the C::B patch from SVN is updated accordingly. This also updates the readme files in wxFlatNoteBook.
With regards, Morten.
Edit: Removed patch to save forum space. It has been applied already.

thomas · « **Reply #4 on:** May 05, 2006, 12:26:03 pm »

Applies cleanly, and works fine under Windows, thanks.

Let's see what the Don finds out under Linux

tiger683 · « **Reply #5 on:** May 05, 2006, 12:54:09 pm »

It fixed a nasty bug for me, when accidentally dragging a notebook-panel or a marked text itself inside the notebook area
would freeze the mouse pointer on button-release (could move it, but rendering it unable to click anything, even outside C::B window)

Thanks!

EDIT: It's gentoo Linux unicode build, wxGTK 2.6.3_p2, C::B revision 2411, gcc-4.1/glibc-2.4

thomas · « **Reply #6 on:** May 05, 2006, 01:04:37 pm »

Update: It does not work so nicely after all.

When closing the application without having a project open, I get a null pointer exception...

thomas · « **Reply #7 on:** May 05, 2006, 01:22:01 pm »

The reason for this is that EditorManager destroys its notebook page on exit. wxFNB 1.3 destroys the notebook page again, which causes the crash.

The modification for EditorManager is in svn.

tiger683 · « **Reply #8 on:** May 05, 2006, 01:59:47 pm »

Quote from: thomas on May 05, 2006, 01:22:01 pm

The modification for EditorManager is in svn.

Thanks!

Didn't notice since the "Something went wrong inside C::B" window disappeared so fast

MortenMacFly · « **Reply #9 on:** May 05, 2006, 02:16:25 pm »

Quote from: thomas on May 05, 2006, 01:04:37 pm

When closing the application without having a project open, I get a null pointer exception...

I obvoisly never did that... which is surpising me... :shock: - but I can confirm this issue. It also happens when you just close the start page after a fresh startup of C::B.

Quote from: thomas on May 05, 2006, 01:22:01 pm

The modification for EditorManager is in svn.

Luckily I can confirm this, too. Thanks, Thomas!
With regards, Morten.

killerbot · « **Reply #10 on:** May 05, 2006, 03:23:36 pm »

editormanager adjust is in svn, should wxFNB also not be updated then in svn ?
We might have a leak without the destroy and the old wxFNB ?

mandrav · « **Reply #11 on:** May 05, 2006, 03:25:48 pm »

Quote from: killerbot on May 05, 2006, 03:23:36 pm

editormanager adjust is in svn, should wxFNB also not be updated then in svn ?

Not until I test it under Linux...

Quote from: killerbot on May 05, 2006, 03:23:36 pm

We might have a leak without the destroy and the old wxFNB ?

We can live with it for a few hours, I guess

sethjackson · « **Reply #12 on:** May 05, 2006, 05:41:04 pm »

Umm wouldn't this be more appropriate in the EditorManager destructor?

Code: cpp

if (m_pNotebook)
    m_pNotebook->Destroy();

Please pardon my ignorance if this is bad code...... :lol:

Der Meister · « **Reply #13 on:** May 05, 2006, 06:13:24 pm »

If I understand thomas correctly this would not solve the problem because the editormagager would destroy the notebook as the pointer is valid (and it should be valid as the old line didn't crash) and *after that* wxFND would try to destroy this notebook again and this will crash - no matter if you check the m_pNotebook pointer in editormanager or not.

yop · « **Reply #14 on:** May 05, 2006, 06:20:08 pm »

Also it's totaly legal to "delete" a null pointer. All the problems arise from the fact that including myself many forget to set the pointer to null after deletion.
From: http://www.cplusplus.com/doc/tutorial/dynamic.html

Quote

The value passed as argument to delete must be either a pointer to a memory block previously allocated with new, or a null pointer (in the case of a null pointer, delete produces no effect).

mandrav · « **Reply #15 on:** May 05, 2006, 06:22:10 pm »

As I expected, the patch doesn't work correctly. That's why I wanted to test it myself.
Steps to reproduce the bug:

1) Right-click on "Build messages".
2) Select "Show/hide->Build log".

The build log hides.

3) Right-click on "Build messages".
4) Select "Show/hide->Build log".

The build log shows again and, seemingly, gets the focus. But still the "Build messages" is displayed.

5) Left-click on "Build messages" and enjoy.

thomas · « **Reply #16 on:** May 05, 2006, 07:26:24 pm »

Quote from: yop on May 05, 2006, 06:20:08 pm

Also it's totaly legal to "delete" a null pointer. All the problems arise from the fact that including myself many forget to set the pointer to null after deletion.

Right. Actually everybody should use something like this all the time:

Code

template<typename T> void Delete(T *p) {delete p; p = 0;};

Hmm... I really wonder why nobody does that. Something must be fundamentally wrong with this, that would be just too simple... :?:

One could use reference counted smart pointers to avoid above problem, too, of course.
If fact, I believe an object having a Destroy() function should necessarily be reference counted - if it only wraps around a plain normal delete, then it is actually not of much use to have such a function.

yop · « **Reply #17 on:** May 05, 2006, 08:04:52 pm »

Quote from: thomas on May 05, 2006, 07:26:24 pm

...Something must be fundamentally wrong with this, that would be just too simple... :?:

Well there is http://public.research.att.com/~bs/bs_faq2.html#delete-zero but still deleting and setting to 0 should cover 99.99% of the cases.

MortenMacFly · « **Reply #18 on:** May 06, 2006, 05:33:53 pm »

Quote from: mandrav on May 05, 2006, 06:22:10 pm

Steps to reproduce the bug:

...confirmed. But: How strange is that? I don't even understand what's happening (to be honest) - where is this space coming from suddenly? :shock:
Anyway: I expected that testing is required - that's why I did not file a patch to BerliOS in the first place. Forgive me, but for my "daily work" it really works properly. But I have to admit that the only thing I'm closing/opening are code files - the lower layout I keep as it is normally.
With regards, Morten.

MortenMacFly · « **Reply #19 on:** May 06, 2006, 11:19:04 pm »

...alright, I've tried to track down this issue nearly the whole day without success. In my humble opinion this is a very weird bug and from the changes done from 0.90 to 1.30 I really don't understand why and where this bug was introduced. Anyway, I'm not an expert in wxWidgets which makes things for me even worse. If anybody could at least give me a hint... that would be really helpful. I'm closing for today (tonight ;-)), maybe I seee more clear tomorrow... :|
With regards, Morten.

MortenMacFly · « **Reply #20 on:** May 07, 2006, 11:18:23 am »

Quote from: mandrav on May 05, 2006, 06:22:10 pm

As I expected, the patch doesn't work correctly. That's why I wanted to test it myself.

mandrav: Correct me, if I'm wrong, but could it be that this bug also appears in the nightlys? Starting from v0.90 (C::B pathhed as from SVN) I tried to apply the changes done up to revision 1.30 to see when the issue appears. But when I compiled C::B again using the SVN version the bug did already appear. I thought it might have been a missing deletion of any file, did a re-build... again. Then I downloaded the latest nightly available and renamed "my" C::B folder. But when I run the nightly and do the steps you described the same thing happens. Are you aware of that?
If so I got you wrong because I thought this issue was introduced somewhere in between v0.90 and v1.30...?!
With regards, Morten.

PChris · « **Reply #21 on:** May 07, 2006, 11:29:11 am »

Quote from: MortenMacFly on May 07, 2006, 11:18:23 am

Quote from: mandrav on May 05, 2006, 06:22:10 pm
As I expected, the patch doesn't work correctly. That's why I wanted to test it myself.
mandrav: Correct me, if I'm wrong, but could it be that this bug also appears in the nightlys? Starting from v0.90 (C::B pathhed as from SVN) I tried to apply the changes done up to revision 1.30 to see when the issue appears. But when I compiled C::B again using the SVN version the bug did already appear. I thought it might have been a missing deletion of any file, did a re-build... again. Then I downloaded the latest nightly available and renamed "my" C::B folder. But when I run the nightly and do the steps you described the same thing happens. Are you aware of that?
If so I got you wrong because I thought this issue was introduced somewhere in between v0.90 and v1.30...?!
With regards, Morten.

I can confirm this too...
This bug is also present on the old version.

Der Meister · « **Reply #22 on:** May 07, 2006, 11:31:37 am »

Confirmed even on Linux. Revision 2417, compiled with gcc 3.4.5 and wxGTK 2.6.2 (unicode).

TheTuxKeeper · « **Reply #23 on:** May 07, 2006, 12:12:56 pm »

Same with rev2411 on Fedora (wxGTK 2.6.2 unicode + gcc 4.02). If you repeat these steps more than once the space is getting few pixels larger with each repetition of these steps until only few pixels of the headline are visible. Even the vertical scrollbar is gone :shock:

I forgot to save the last screenshot, but this one is only few times before the space isn't getting bigger anymore.

Der Meister · « **Reply #24 on:** May 07, 2006, 12:36:21 pm »

I'm not quite sure what behaviour is intended. But this problem can be fixed with this little patch:

Code

Index: src/sdk/messagemanager.cpp
===================================================================
--- src/sdk/messagemanager.cpp  (revision 2417)
+++ src/sdk/messagemanager.cpp  (working copy)
@@ -332,7 +332,7 @@
     if (show && !ls->visible)
     {
         // show
-        m_pNotebook->InsertPage(id, log, ls->title, false);
+        m_pNotebook->InsertPage(id, log, ls->title, true);
 
         SetLogImage(id, ls->bitmap);
         ls->visible = true;

This way the tab that will be shown again gets selected. I think this is not the originally intended behaviour but it at least works without the problem described here.

thomas · « **Reply #25 on:** May 07, 2006, 02:54:29 pm »

Is "selected" synonymous to "given focus"?

I guess if "selected" only means "becomes topmost tab" then it is quite harmless (and even intuitive).

Der Meister · « **Reply #26 on:** May 07, 2006, 04:33:07 pm »

Yes, it has the focus, it is displayed and is the topmost tab. Whether this is intuitive or not is another question. I would even prefer that the tab that was selected before stays selected and the "new one" is only placed between the other inactive tabs. I guess that was the originally intended behaviour.
But with a quick look at the roadmap I would say we should apply this little patch (even if the behaviour is not as intuitive than before - that is my opionion, maybe the others think different) as a workaround for this problem rather than spending hours or even days on searching and fixing the real problem.

mandrav · « **Reply #27 on:** May 08, 2006, 10:36:21 pm »

HEAD updated to wxFNB 1.30.

sethjackson · « **Reply #28 on:** May 08, 2006, 10:40:44 pm »

Quote from: mandrav on May 08, 2006, 10:36:21 pm

HEAD updated to wxFNB 1.30.

Yay!

Aww bugs....

Do this.

1. Right click on a messages tab.
2. Show/Hide a visible tab.
3. Show/Hide the same tab (making it visible).
4. Not good.

Fix.

Code: diff

Index: src/sdk/messagemanager.cpp
===================================================================
--- src/sdk/messagemanager.cpp	(revision 2421)
+++ src/sdk/messagemanager.cpp	(working copy)
@@ -337,8 +337,6 @@
         SetLogImage(id, ls->bitmap);
         ls->visible = true;
 
-        log->Show(false);
-
         if (id == m_DebugLog)
             cfg->Write(_T("/has_debug_log"), (bool)true);
     }
@@ -350,8 +348,6 @@
             m_pNotebook->RemovePage(id);
         ls->visible = false;
 
-        log->Show(false);
-
         if (id == m_DebugLog)
             cfg->Write(_T("/has_debug_log"), (bool)false);
     }

Posted to the tracker.

https://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=1038&group_id=5358

MortenMacFly · « **Reply #29 on:** May 09, 2006, 11:20:27 am »

Well... there is something wrong with how this is applied in SVN. Currently the cb_wxfn.patch holds the patch how to update the C::B sources to wxFlatNotebook 1.30-patched. My intention was to provide the patch how to change the original wxFNB sources (v1.30) to the way they are used in SVN.
I've attached a patch to this post (to be applied to the SVN sources) that will:
1.) Update the cb_wxfn.patch file to really have the changes applied within C::B in comparision to wxFNB v1.30
2.) Update the readme file
3.) Apply one minor change to wxFlatNotebook I've missed
With regards, Morten.
Edit: Removed patch to save forum space. It has been applied already.

mandrav · « **Reply #30 on:** May 09, 2006, 01:43:13 pm »

Thanks, commited.

News:

Author Topic: wxFlatNotebook update + Patch (Read 21180 times)

sethjackson

PChris

sethjackson