Opened 7 years ago

Last modified 13 months ago

#8945 confirmed enhancement

Doc/View framework for wxAUI MDI

Reported by: ru10 Owned by:
Priority: normal Milestone: 3.2.0
Component: wxAui Version: stable-latest
Keywords: wxAuiDocMDI Cc: ru10, biwillia76, erudix@…, florent.teichteil@…, bpetty, kinaou.herve@…
Blocked By: #15118 Blocking: #10939
Patch: yes

Description

Here is my patch for including Doc/View framework into wxAUI MDI concept.

I created the classes wxAuiDocMDIParent/ChildFrame which derives from the corresponding wxAuiMDI classes. I took back the wxDocMDI code and adapted it to the wxAui needs.

I thus added the files tabdocmdi.h/cpp and modified the VS project of wxAUI and wxWidgets to take them in care.
I created a sample app but you should probably create de folder 'samples\auidocvwmdi' before applying the patch.
Moreover, I created the related doc in tex format. The doc makes reference to the wxAuiMDI classes which do not exist at this time.

It seem to work fine under Windows XP sp2, I do not test it under other platforms.

I'm waiting for any remark, critic, idea...

Attachments (13)

wxAuiDocView_2007_11_23_patch.zip download (65.4 KB) - added by ru10 7 years ago.
Doc/View framework for wxAUI MDI
wxAuiDocMDI.patch download (28.0 KB) - added by ninjanl 6 years ago.
wxAuiDocView_2009_03_27.patch download (72.5 KB) - added by R.U.10 6 years ago.
Original AuiDocMDI code upgraded with the last SVN sources --without the sample--
wxAuiDocView_wx290_2009_04_17.patch download (72.9 KB) - added by R.U.10 6 years ago.
Doc/View framework for wxAUI and wxWidgets 2.9.0 svn
wxAuiDocView_wx29.patch download (80.8 KB) - added by R.U.10 5 years ago.
Doc/View framework for wxAUI and wxWidgets 2.9 SVN trunk
wxAuiDocView_wx29_auidocview_and_sample.2.patch download (17.2 KB) - added by R.U.10 5 years ago.
wxAuiDocView_wx29_refactored_base_classes.patch download (18.0 KB) - added by R.U.10 4 years ago.
Refactored base classes for Aui Doc/View integration
wxAuiDocView_wx29_QuickAndDirtyFixForLinkingIssue.patch download (5.9 KB) - added by R.U.10 4 years ago.
Quick an dirty fix for linking issue in DLL mode (if possible do not use it...)
wxAuiDocView_wx29_auidocview_and_sample.patch download (46.4 KB) - added by R.U.10 4 years ago.
Aui Doc/View implementation and modified sample
wxAuiDocView_wx29_dlllink.patch download (680 bytes) - added by mahae 4 years ago.
Other fix for dll linking issue
wxAuiDocView_wx29_refactored_base_classes_r67931.patch download (19.4 KB) - added by vadz 3 years ago.
The same patch against the current svn HEAD
wxAuiDocView_wx29_refactored_base_classes_traits.patch download (44.4 KB) - added by R.U.10 13 months ago.
Refactored base classes based on traits for Aui Doc/View integration
wxAuiDocView_wx29_auidocview_and_sample_traits.patch download (65.2 KB) - added by R.U.10 13 months ago.
Aui Doc/View implementation based on traits and modified sample

Download all attachments as: .zip

Change History (79)

Changed 7 years ago by ru10

Doc/View framework for wxAUI MDI

comment:1 Changed 7 years ago by wxsite

  • Status changed from assigned to confirmed

transitioning old 'assigned' status to new 'confirmed' status

comment:2 Changed 7 years ago by wojdyr

  • Type set to enhancement

comment:3 Changed 6 years ago by vadz

  • Status changed from confirmed to infoneeded_new

First, sorry for taking so long to look at this.

Second now that I did look at it the main problem I have is that the code in src/aui/tabdocmdi.cpp doesn't seem to be at all AUI-specific and duplicates a lot of code already existing e.g. in src/common/docmdi.cpp.

And it's even worse with the sample: while it's a very good idea to provide changes to the sample illustrating the new functionality I really don't want to end up with the third copy of this example (there are already two in samples/docview and docmdi). I think it would be better to have a compilation switch in docmdi sample to allow compiling it with AUI MDI support instead of the native one.

If you could find a way to implement these classes without duplicating too much code, it would be really great.

And, if you resubmit this patch, a few things:

  1. Please don't submit the generated files such as Makefile.in and other makefile.* in the patches. All these files are created from .bkl file and just the .bkl is enough (run bakefile to produce all the makefiles from it).
  2. Please don't submit your patches as ZIPs (of course, this was a consequence of the above point as the patch became huge because of all the generated files in it). This is the main explanation why nobody looked at it for so long.
  3. Since this patch submission the documentation system has changed and now its files are under interface/wx

Thanks!

Changed 6 years ago by ninjanl

comment:4 Changed 6 years ago by ninjanl

  • Owner biwillia76 deleted
  • Status changed from infoneeded_new to new

I would like to pursue this topic if I may.

I have attached a patch against SVN-HEAD.

I intend to address the problem of wxAui documentation in a separate patch, if that is OK.

This patch simply replicates the work done in the prior patch, against, now against svn head.

To see the new classes in action, simply set the __USE_WXAUI__ value to 1 and compile.

The wxAui library also needs rebuilding and including in the sample libraries.

Hopefully this patch satisfies at least some of the criteria for allowing these classes to be included.

Tested on Vista/VS2005

comment:5 Changed 6 years ago by biwillia76

This looks great-- nice work. Does anybody see any barriers preventing this from getting applied?

Taking care of the documentation in a separate patch is fine. Is it possible you can test this on wxGTK?

All the best,
Ben

comment:6 Changed 6 years ago by ninjanl

I can probably give it a go on both Ubuntu and PuppyLinux tomorrow. Will this be sufficient for your purposes? I don't have access to any other distro at the moment.

comment:7 Changed 6 years ago by vadz

  • Status changed from new to infoneeded_new

Unfortunately I do have a few comments about this patch:

  1. We really want to have just one version of the docview sample which was just cleaned up to show all possible different docview modes. If it can't be done without conditional compilation, then we must have a separate sample instead of uglifying this one with #ifdef __USE_WXAUI__. But IMO it should be possible to just add a command line "aui" option which would be clearly preferable. BTW notice that docview.bkl needs to be modified to include <wx-lib>aui</wx-lib> line in this case.
  2. There seems to be a lot of duplicate code in the new files apparently stemming from the fact that wxAuiMDIParentFrame doesn't derive from wxMDIParentFrameBase and so can't reuse the code there. It would be really great to avoid copying the code around and refactor it properly to make this unnecessary. I'm afraid I can't agree to adding more code duplication, especially, again, after just spending some effort in attempts to reduce it.
  3. As just one of the problems exposed by this copy-and-paste approach, overriding ProcessEvent() is wrong and doesn't work correctly with pushed event handlers. The new virtual TryBefore() function should be used instead.
  4. Finally there are the usual style issues, e.g. things like naming a static variable ActiveEvent instead of s_activeEvent as per wx coding standards and so on.

IOW I don't think the patch can be applied in its current state, sorry. And I'd really prefer to have documentation together with the main patch, this is the best way to ensure it's not forgotten later.

Thanks!

comment:8 Changed 6 years ago by ninjanl

  • Status changed from infoneeded_new to new

Thanks for the comments Vadim.

  1. I can rework the sample to use a command line switch, and of course add some documentation to the next patch.
  2. Forgive my ignorance (since I didn't write any of this myself and am just reworking work done by others) but are you saying that I should be reworking the wxAuiMDIParentFrame (initially) to derive it from wxMDIParentFrameBase? The wxAuiMDIParentFrame was written by the original authors of wxAui and I am a little reticent to hack away at their code, but will with gusto if you think that this is the way to go (unless Ben decides to jump in and do this for me)
  3. I understand your comments about ProcessEvent etc. and will attempt to correct these faults as I work through the code.
  4. I will endeavor to follow these as best I can.
  5. As a possible comment on the wxWidgets styles, does anyone have an aStyle config file for the wxWidgets styles?

best regards
Mal

comment:9 Changed 6 years ago by ninjanl

Interestingly, it looks as though the current mdi sample crashes if an attempt is done to build with the generic mdi interface.

Best Wishes

Mal

comment:10 follow-up: Changed 6 years ago by vadz

Mal, thanks for your replies, I'm not sure about the point (2) neither but I think that you should indeed hack (well, we usually call this "refactor" :-) the existing AUI code as it seems like there is scope to reduce code duplication in it too. I might be missing something though, I don't know this code well at all.

As for the crash, how can I reproduce it? In particular I think I tested the sample under wxX11 (which does use the generic implementation), which port did you test it with?

Thanks!

comment:11 Changed 6 years ago by R.U.10

  • Patch unset

Hello,

Unfortunately I have not had time to rework my code...
The main problem I saw to make the wxAuiMDIParentFrame derive of the wxWidgets version is the ClientWindow they manage. The wxAuiMDIParentFrame manages a wxAuiMDIClientWindow which is a wxAuiNotebook, while the wxMDIParentFrame manages a wxMDIClientWindow which is a wxWindow.
Except that, the classes appearance is really similar and I suspect a ugly copy/past since inception (and I did not make better myself...).
The solution I propose but I have not had time to experiment is to put the ClientWindow in a separate interface for both AUI and wxWidgets MDI classes (eg. wxIMDIClientWindow and wxIAuiMDIClientWindow - wxI... for Interface). The wxMDIParentFrame could be renamed to wxMDIParentFrameBase (for example) and "amputated" of its wxMDIClientWindow. A class named wxMDIParentFrame could derive of wxMDIParentFrameBase and wxIMDIClientWindow to retrieve the standard wxWidgets behavior. Probably some functions should become virtual.
This solution allows the wxAuiMDIParentFrame to derive of wxMDIParentFrameBase (and get back a lot of the wxWidgets features of the class without the wxMDIClientWindow) and wxIAuiMDIClientWindow (to recover the use of the wxAuiMDIClientWindow).
I also propose an interface for Doc part. The interface wxIDocMDIParentFrame could contain the functions needed for the Doc/View specialization. So the wxDocMDIParentFrame and the wxAuiDocMDIParentFrame classes would derive of the wxIDocMDIParentFrame and their respective MDIParentFrame.

Look at the class diagram.

I thought something similar for the MDIClientFrame classes.

It implies to modify the sources of wxWidgets and wxAui, so you will understand easily that I have not yet taken time to experiment it ;) (especially since the ugly copy/past works well). But I am available to discuss on it.

comment:12 Changed 6 years ago by ninjanl

Vadim,

As far as the crash is concerned, I simply built the sample under Vista/wxMSW/vs2005, changed the #if 0
text to #if 1, compiled and ran.

It fails because of the call to

windowMenu->SetLabel(wxID_MDI_WINDOW_TILE_HORZ, "&Tile horizontally\tCtrl-Shift-H");

on or about line 148 of \samples\mdi\mdi.cpp (and apparently other calls to SetLabel too)

After investigation it looks as though the constructor of the generic MDI frame is (possibly) incorrect.

Compare the generic MDI Parent Frame

bool wxGenericMDIParentFrame::Create(wxWindow *parent,
wxWindowID id,
const wxString& title,
const wxPoint& pos,
const wxSize& size,
long style,
const wxString& name)
{
this style can be used to prevent a window from having the standard MDI
"Window" menu
if ( !(style & wxFRAME_NO_WINDOW_MENU) )
{
#if wxUSE_MENUS
m_windowMenu = new wxMenu;

m_windowMenu->Append(wxWINDOWCLOSE, _("Cl&ose"));
m_windowMenu->Append(wxWINDOWCLOSEALL, _("Close All"));
m_windowMenu->AppendSeparator();
m_windowMenu->Append(wxWINDOWNEXT, _("&Next"));
m_windowMenu->Append(wxWINDOWPREV, _("&Previous"));
#endif wxUSE_MENUS
}

with the MSW MDI Parent Frame

bool wxMDIParentFrame::Create(wxWindow *parent,
wxWindowID id,
const wxString& title,
const wxPoint& pos,
const wxSize& size,
long style,
const wxString& name)
{
this style can be used to prevent a window from having the standard MDI
"Window" menu
if ( !(style & wxFRAME_NO_WINDOW_MENU) )
{
normal case: we have the window menu, so construct it
m_windowMenu = new wxMenu;

m_windowMenu->Append(wxID_MDI_WINDOW_CASCADE, _("&Cascade"));
m_windowMenu->Append(wxID_MDI_WINDOW_TILE_HORZ, _("Tile &Horizontally"));
m_windowMenu->Append(wxID_MDI_WINDOW_TILE_VERT, _("Tile &Vertically"));
m_windowMenu->AppendSeparator();
m_windowMenu->Append(wxID_MDI_WINDOW_ARRANGE_ICONS, _("&Arrange Icons"));
m_windowMenu->Append(wxID_MDI_WINDOW_NEXT, _("&Next"));
m_windowMenu->Append(wxID_MDI_WINDOW_PREV, _("&Previous"));
}

It is clear to see where the problem lies.

The program fails at

void wxMenuBase::SetLabel( int id, const wxString &label )
{
wxMenuItem *item = FindItem(id);

wxCHECK_RET( item, wxT("wxMenu::SetLabel: no such item") );

item->SetItemLabel(label);
}

I eventually get an unhandled exception message box.

HTH

Mal

comment:13 Changed 6 years ago by ninjanl

r.u.10

I already have a compile line effected docview sample. I can forward this to you should you wish to save yourself some work.

Best wishes

Mal

comment:14 in reply to: ↑ 10 Changed 6 years ago by ninjanl

Replying to vadz:

Further I have succeded in building svn with Ubuntu, however the mdi sample still crashes when I set the #if 0 text to #if 1

Under the terminal window I see

Trace/breakpoint trap, and then Segmentation fault. This is with a release build of the library.

I hope this helps a little.

Could you enlighten me as to your setup? I might very well have done something wrong, although I have successfully managed to build other samples.

Best regards

Mal

Mal, thanks for your replies, I'm not sure about the point (2) neither but I think that you should indeed hack (well, we usually call this "refactor" :-) the existing AUI code as it seems like there is scope to reduce code duplication in it too. I might be missing something though, I don't know this code well at all. As for the crash, how can I reproduce it? In particular I think I tested the sample under wxX11 (which does use the generic implementation), which port did you test it with? Thanks!

Changed 6 years ago by R.U.10

Original AuiDocMDI code upgraded with the last SVN sources --without the sample--

comment:15 Changed 6 years ago by R.U.10

  • Milestone set to 2.9.0
  • Patch set
  • Version set to 2.9-svn

I finally reviewed all the needed classes to avoid copy/paste of code. I started from wxWidgets 2.9.0 svn.
I could not preserve the wxDocChild/ParentFrameAny template classes because of linking issues at the compilation between aui and core DLL.

Here is the new inheritance graph:

/-> wxDocMDIParentFrameAnyBase -> wxMDIParentFrameBase -> wxFrame

wxAuiDocMDIParentFrame

\-> wxAuiMDIParentFrame

I also modified the docview sample but I have not written the documentation yet.

Consider this patch as beta version, I'm waiting for your feed back.

comment:16 Changed 6 years ago by R.U.10

                        /-> wx[Aui]MDIParent/ChildFrame -> wxMDIParent/ChildFrameBase -> wxFrame
wx[Aui]DocMDIParent/ChildFrame
                        \-> wxDocMDIParent/ChildFrameAnyBase


                    /-> wxMDIClientWindowBase
wxAuiMDIClientWindow
                    \-> wxAuiNotebook


                 /-> wxMDIClientWindowBase
wxMDIClientWindow
                 \-> wxWindow

Changed 6 years ago by R.U.10

Doc/View framework for wxAUI and wxWidgets 2.9.0 svn

comment:17 Changed 6 years ago by vadz

  • Milestone 2.9.0 deleted

2.9.0 milestone is already past

Changed 5 years ago by R.U.10

Doc/View framework for wxAUI and wxWidgets 2.9 SVN trunk

comment:18 Changed 5 years ago by R.U.10

  • Cc erudix@… added
  • Keywords wxAuiDocMDI added

Patch regenerated for the latest SVN trunk revision (no new implementation).

comment:19 Changed 5 years ago by ninjanl

Could someone tell me what the status of adding Doc/View to wxAui is?

R.U.10 reworked his patch to cater for the then latest SVN (4 weeks ago) code.

It looks as though it does do what Vadim wanted, although he would obviously be best suited to commenting on this. 

Further it does look to me like a suitable addition to the current wxAui implementation (we already have a wxAui tabmdi implementation, this simply adds doc/view to that).

I know it looks like I am pressuring you all, and this is not my intention, but these look to be useful classes, which I could personally use in a project of mine. I know I could work with a patched version of the code, but I don't really want to maintain a patched tree.

If someone could examine the code and yea or nay the patch, then we could move on. Probably to other areas of improving wxAui (probably by submitting individual patches for other improvements) or reworking the patch to meet the demands of the wxWidgets developers.

Any result would be an advancement.

Best regards

Malcolm

comment:20 Changed 5 years ago by vadz

  • Milestone set to 2.9.1
  • Status changed from new to confirmed

It globally looks good but the patch is huge and I didn't have time to really review it yet. I see some small things here and there which would need to be fixed (e.g. we need to use Connect() and not Bind() inside wx itself) but globally it indeed looks good. I'd appreciate if somebody else could reread it too though and also share any experiences of using it.

comment:21 Changed 5 years ago by biwillia76

I reviewed this patch and it looks like everything has been done correctly. It looks great. Hopefully I'll get a chance here to give it a spin.

Yep the patch is a bit huge. I'd probably split it into three parts, *Base class work, the aui portion, and the samples portion.

comment:22 Changed 5 years ago by vadz

I don't think it's worth splitting out the samples part, it doesn't touch the same files after all. But I'd definitely like to separate the refactoring of the existing classes from the introduction of the new ones. This would make it easier to both review and find any regressions later.

comment:23 Changed 5 years ago by R.U.10

I generated a separated patch for:

  • the refactored base classes
  • the adding of the Doc/View framework in wxAui and the sample adaptation.

I did not modified the use of the Bind functions. It could be a minor modification before committing the code(?)

comment:24 Changed 5 years ago by R.U.10

  • Milestone 2.9.1 deleted

What new about that?

My ticket celebrates its two years opening...

comment:25 Changed 5 years ago by R.U.10

I am going to leave my current post in few months and I'm not sure to have enough time to bring modifications on the code after that. So I would really like to see my patch applied If you consider it acceptable.

comment:26 follow-up: Changed 5 years ago by vadz

  • Milestone set to 2.9.2

Has anybody (except the author) tested this patch? Any feedback about it?

I'd love to have this but I simply don't have time to review nor test it properly...

comment:27 in reply to: ↑ 26 ; follow-up: Changed 5 years ago by ninjanl

Replying to vadz:

Has anybody (except the author) tested this patch? Any feedback about it?

I'd love to have this but I simply don't have time to review nor test it properly...

I just applied the patch to a clean svn checkout.

I am not quite sure how accurate the patch application is within svn so I am not sure if the patch is 100% correctly applied.

That said (and bearing in mind that I WANT these classes in wxWidgets myself), I did notice a couple of bugs.

Firstly the required wx..._aui.lib is not listed in the libraries required of the sample. A slight oversight but nonetheless needed.

Secondly when the views are changed, the menu does not reflect this. IOW when the view has been changed the "File" should change to reflect this, offering Save and/or Save As... options. These remain disabled in the sample.

This is possibly a small oversight since when closing a changed view we are presented with a "Do You Want to Save Changes" message box.

Notice that I said "views". If there is only a single view, then this changed view correctly enables the Save and Save As... menu options.

These are just a couple of things I noticed initially.

Build setup: Windows 7, M$ VS2008 Express, Debug build.

Regards
Mal

comment:28 in reply to: ↑ 27 Changed 5 years ago by R.U.10

Firstly the required wx..._aui.lib is not listed in the libraries required of the sample.

You should add the AUI library file in the dependencies of the sample project while the patch will not be applied definitely and the project files not regenerated (if I well understood).

You should also add the tabdocmdi.h/cpp files in the AUI project before building the wxWidgets libraries.

Secondly when the views are changed, the menu does not reflect this.

Fixed

I uploaded the new patch files over the old files (the file wx...sample.2.patch is there by mistake but I can't delete it).

comment:29 Changed 5 years ago by biwillia76

Hello Vadim,

I've reviewed this patch line-for-line, and I'm very much in favor of it getting applied. Is there anything blocking this still?

Thanks.

All the best,
Ben

comment:30 Changed 5 years ago by VZ

(In [64295]) Refactor wxDocParentFrame and wxDocMDIParentFrame to share common base class.

Use the same approach as for the child frames: add a base template class which
allows wxDocParentFrame to inherit from wxFrame and wxDocMDIParentFrame from
wxMDIParentFrame while still allowing to reuse the common code.

This reduces code duplication and should make implementing parent AUI document
frame easier as well, see #8945.

comment:31 Changed 5 years ago by vadz

I didn't look at the AUI-specific (i.e. interesting) parts of the patch yet but I really don't understand what did it [try to] do with the doc view classes. The removal of wxDocChildFrameAny definitely isn't the right thing to do, in fact the commit referenced above just introduced wxDocParentFrameAny and we can't avoid code duplication without it.

It seems clear that wxAuiMDIParentFrame must derive from wxDocMDIParentFrame (and not wxMDIParentFrameBase) and wxAuiMDIChildFrame -- from wxDocChildFrameAny<wxPanel, wxAuiMDIParentFrame>. And maybe we need something similar for wxAuiMDIClientWindow. But please let me know if I'm missing anything.

In any case, the patch can't be applied as is...

comment:32 follow-up: Changed 5 years ago by R.U.10

The class wxAuiMDIParentFrame is quite similar of wxMDIParentFrame but it manages a wxAuiNotebook instead of wxNotebook. I cannot make wxAuiMDIParentFrame derive from wxDocMDIParentFrame for two reasons:

  • wxDocMDIParentFrame derives from wxMDIParentFrame and I do not want to inherit of the wxNotebook as "Client Window"
  • As is, wxAuiMDIParentFrame can be used without the Doc/View framework (which is probably the behavior used for Strata). It will not be the case any more if I follow your solution.

As I reported here last year, I experimented some linking problem with the template form of wxDocChildFrameAny. This is comprehensible because it needs to know his given typenames. So if I make wxAuiMDIChildFrame derive from wxDocChildFrameAny<wxPanel, wxAuiMDIParentFrame>, the core library will need a dependency relation with the aui library which is, I think, unwanted because of scalability reasons.

Of course, I stay listening to your comments or suggestions. So if you insist, I can try again to use the template form of wxDocChildFrameAny to give you a precise report. And if the conceptual approach given here is not the good one for you, we can think together to define a better one which serve our common imperatives.

comment:33 in reply to: ↑ 32 Changed 5 years ago by vadz

Replying to R.U.10:

The class wxAuiMDIParentFrame is quite similar of wxMDIParentFrame but it manages a wxAuiNotebook instead of wxNotebook. I cannot make wxAuiMDIParentFrame derive from wxDocMDIParentFrame for two reasons:

  • wxDocMDIParentFrame derives from wxMDIParentFrame and I do not want to inherit of the wxNotebook as "Client Window"
  • As is, wxAuiMDIParentFrame can be used without the Doc/View framework (which is probably the behavior used for Strata). It will not be the case any more if I follow your solution.

Sorry, I probably confused MDI and doc-view MDI versions of the classes here. I guess it's ok to have wxAuiMDIParentFrame. But the point is that wxDocAUIParentFrame should still derive from wxDocParentFrameAny<wxAuiMDIParentFrame> to avoid duplicating the code of this class.

As I reported here last year, I experimented some linking problem with the template form of wxDocChildFrameAny. This is comprehensible because it needs to know his given typenames.

Sorry, I don't comprehend this at all. I missed the comment:15 but anyhow there were no details about the problems you ran into, what were they?

So if I make wxAuiMDIChildFrame derive from wxDocChildFrameAny<wxPanel, wxAuiMDIParentFrame>, the core library will need a dependency relation with the aui library which is, I think, unwanted because of scalability reasons.

It's unwanted but AFAICS isn't a problem because wxAuiMDIChildFrame will be in aui library and not core. I really don't see what the problem is here.

Of course, I stay listening to your comments or suggestions. So if you insist, I can try again to use the template form of wxDocChildFrameAny to give you a precise report. And if the conceptual approach given here is not the good one for you, we can think together to define a better one which serve our common imperatives.

I think it would be best to discuss this on wx-dev, which is more convenient than Trac for discussions. But the diagram of comment:16 looks mostly fine to me, it's just the doc-view classes which are not right. AFAICS you should be able to keep most of the patch unchanged and change just the doc-view part. In fact I wonder if we couldn't apply the patch adding "AUI MDI" stuff right now and then add doc view support for it later. Again, I might be missing something, but if you have AUI classes emulating MDI ones closely, doc-view support should be completely trivial, just look at the latest include/wx/docmdi.h (and src/common/docmdi.cpp is empty).

comment:34 Changed 4 years ago by R.U.10

Ok, I made the expected modifications and updated the patch. It seems to work fine in LIB mode.

But please try to compile wxWidgets in DLL mode. I have these errors while compiling the new tabdocmdi.cpp file:

4>------ Build started: Project: aui, Configuration: DLL Debug Win32 ------
4>Compiling...
4>tabdocmdi.cpp
4>..\..\src\aui\tabdocmdi.cpp(150) : warning C4273: 'ms_classInfo' : inconsistent dll linkage
4>        c:\hk8\Devel\WX_2_9_TRUNK\include\wx/aui/tabdocmdi.h(109) : see previous definition of 'public: static wxClassInfo wxAuiDocMDIChildFrame::ms_classInfo'
4>..\..\src\aui\tabdocmdi.cpp(150) : error C2491: 'wxAuiDocMDIChildFrame::ms_classInfo' : definition of dllimport static data member not allowed
4>..\..\src\aui\tabdocmdi.cpp(150) : warning C4273: 'wxAuiDocMDIChildFrame::GetClassInfo' : inconsistent dll linkage
4>        c:\hk8\Devel\WX_2_9_TRUNK\include\wx/aui/tabdocmdi.h(109) : see previous definition of 'GetClassInfo'
4>..\..\src\aui\tabdocmdi.cpp(161) : warning C4273: 'wxAuiDocMDIChildFrame::wxAuiDocMDIChildFrame' : inconsistent dll linkage
4>        c:\hk8\Devel\WX_2_9_TRUNK\include\wx/aui/tabdocmdi.h(85) : see previous definition of '{ctor}'
4>..\..\src\aui\tabdocmdi.cpp(174) : warning C4273: 'wxAuiDocMDIChildFrame::Create' : inconsistent dll linkage
4>        c:\hk8\Devel\WX_2_9_TRUNK\include\wx/aui/tabdocmdi.h(95) : see previous definition of 'Create'
4>Build log was saved at "file://c:\hk8\Devel\WX_2_9_TRUNK\build\msw\vc_mswuddll\aui\BuildLog.htm"
4>aui - 1 error(s), 4 warning(s)

If you see something wrong...

By what means do I access the wx-dev channel?

comment:35 Changed 4 years ago by vadz

Thanks, I'll look at this patch in details soon but I already see that it uses dynamic_cast<> extensively which is for me a big warning sign. Why do we need this?

The DLL errors are probably due to some missing or mismatching WXDLIMPEXP_XXX somewhere.

wx-dev is a mailing list, please see http://www.wxwidgets.org/support/maillst2.htm#dev for instructions about subscribing and posting to it.

Thanks!

comment:36 follow-up: Changed 4 years ago by fteicht

  • Cc florent.teichteil@… added

Hi all!
I am very interested in the wxAUI doc/view feature. I looked at the wxWidgets 2.9 SVN trunk but could not find the tabdocmdi.h/cpp files in the repository. Is there any chance that this important feature be included in the official wxWidgets sources one day?
Thanks,
Florent

Changed 4 years ago by R.U.10

Refactored base classes for Aui Doc/View integration

comment:37 Changed 4 years ago by R.U.10

Updated the patches:

  • Removed duplicated event methods
  • Prevented an unexpected call to the virtual method DoActivate during destruction

comment:38 in reply to: ↑ 36 Changed 4 years ago by R.U.10

Replying to fteicht:

I am very interested in the wxAUI doc/view feature.

Great, one more ;)

I looked at the wxWidgets 2.9 SVN trunk but could not find the tabdocmdi.h/cpp files in the repository.

It's because the patch is not yet applied. I am waiting for code review by vadz to resolve a linking problem in DLL mode.

Is there any chance that this important feature be included in the official wxWidgets sources one day?

Yes, there is a good chance but there is quite a lot of inertia...

Regards

Changed 4 years ago by R.U.10

Quick an dirty fix for linking issue in DLL mode (if possible do not use it...)

comment:39 Changed 4 years ago by vadz

  • Blocking 10939 added

(In #10939) This should probably be applied after dealing with #8945. We also need to be able to test this code somehow, would be great to have some (even if very simple) non-interactive unit tests for this and/or (but preferably "and") a patch to the xrc sample showing this in action.

P.S. The patch doesn't quite apply any more, it's not difficult to apply manually but if you work to enhance it further, could you please redo it against the latest svn?

P.P.S. I wonder if any VC6 users could check if it compiles with this compiler?

Changed 4 years ago by R.U.10

Aui Doc/View implementation and modified sample

comment:40 Changed 4 years ago by triton

What's the status of this one? Seems like an useful addition to AUI.

comment:41 Changed 4 years ago by R.U.10

There is a linking issue between core and aui libraries during DLL compilation. I hope vadz will have a little bit of time to study the problem to finalize the implementation asap.

Meanwhile you can apply the patches to benefit from new features.
Apply them in this order:
1/ wxAuiDocView_wx29_refactored_base_classes.patch
2/ wxAuiDocView_wx29_auidocview_and_sample.patch
3/ for DLL buid: wxAuiDocView_wx29_QuickAndDirtyFixForLinkingIssue.patch

The code is fully functional.

comment:42 Changed 4 years ago by bpetty

  • Cc bpetty added

Changed 4 years ago by mahae

Other fix for dll linking issue

comment:43 Changed 4 years ago by mahae

I am using the wxAuiDocView_wx29_dlllink.patch file for solving the dll linking problem. This is working with Visual Studio 2010.

comment:44 Changed 4 years ago by R.U.10

Great! It's a much better solution than my "QuickAndDirtyFix" and it works also with VS 2005.

comment:45 follow-up: Changed 3 years ago by vadz

  • Milestone changed from 2.9.2 to 3.0

I've finally started looking at this patch (better late than never...).

Unfortunately even the first, supposedly trivial, part, i.e. wxAuiDocView_wx29_refactored_base_classes.patch is already problematic because it breaks compilation of non-MSW ports by incompatible changes to the base MDI classes, e.g. wxGTK simply doesn't compile at all with these changes (and neither does wxOSX). While this could be fixed by updating wxGTK and wxOSX sources, these changes would also break user code. For example, currently you can write

wxMDIChildFrame *child = mdiParent->GetActiveChild();

and a lot of code does exactly this, in fact, but this code won't compile any more. This alone seems to clearly indicate that we should actually preserve the name wxMDIChildFrame for the common base class for all the different MDI children, am I missing something? The same also applies to wxMDIParentFrame and wxMDIClientWindow too.

Also, why do we need to add wxMDIParentFrame *m_pMdiParent field to wxMSW wxMDIChildFrame? It really ought to reuse the base class m_mdiParent field, if necessary please just add a function upcasting it to the right type. But having m_mdiParent and m_pMdiParent fields in the same class is very confusing and error-prone.

TIA!

comment:46 Changed 3 years ago by vadz

P.S. I attach a version of the patch applying cleanly to the current svn but unchanged otherwise.

Changed 3 years ago by vadz

The same patch against the current svn HEAD

comment:47 in reply to: ↑ 45 ; follow-up: Changed 3 years ago by R.U.10

Replying to vadz:

I agree with you. But you have expressed reservations about the use of `dynamic_cast<>` so I found a solution to avoid the use of wxDynamicCast also. But I'm clearly favorable to add a specific method in each specialized classes to upcast the base attribute. However, this solution will not allow the use of the code wxMDIChildFrame *child = mdiParent->GetActiveChild(); 'as is', but rather wxMDIChildFrame *child = GetMDIParentFrame()->GetActiveChild().

IMHO, wxMDIChildFrame cannot be the base class for all the MDI children because it contains methods which are undesirable in specialized classes and I am not sure about what will happen if (for example) a downcasted wxAuiMDIChildFrame or wxAuiMDIClientWindow object is given to a wxMDIParentFrame.

Anyway, thank you for having started looking at this patch :)

comment:48 in reply to: ↑ 47 ; follow-up: Changed 3 years ago by vadz

Replying to R.U.10:

Replying to vadz:

I agree with you. But you have expressed reservations about the use of `dynamic_cast<>` so I found a solution to avoid the use of wxDynamicCast also. But I'm clearly favorable to add a specific method in each specialized classes to upcast the base attribute.

It would be great if we could avoid the use of dynamic casts but if we must use them to keep the existing code compiling/working, then we should use them (although I don't really see how would it help to be honest).

However, this solution will not allow the use of the code wxMDIChildFrame *child = mdiParent->GetActiveChild(); 'as is', but rather wxMDIChildFrame *child = GetMDIParentFrame()->GetActiveChild().

Sorry, what's the difference? For me mdiParent is of type wxMDIParentFrame * and GetMDIParentFrame() should presumably continue to return the value of this type.

IMHO, wxMDIChildFrame cannot be the base class for all the MDI children because it contains methods which are undesirable in specialized classes and I am not sure about what will happen if (for example) a downcasted wxAuiMDIChildFrame or wxAuiMDIClientWindow object is given to a wxMDIParentFrame.

We must make this work though, we simply can't break all the existing code using these classes. If it means having some virtual methods not making sense for the derived classes in the base class, so be it. They'd just be stubbed out to throw an assert in the derived classes.

Of course this is not ideal but it's the only alternative I can see to completely breaking compatibility. And it's not unprecedented, to say the least, to have methods in a base class that don't apply to a derived one in (badly designed for exactly this reason) wx API, just try to call wxWindow::SetScrollbar() on a wxButton...

Anyway, thank you for having started looking at this patch :)

Thanks a lot for continuing to work on this and not losing hope since all this time, I realize that it must not be easy to keep the motivation. And I want to reassure you that I still would like very much the goal of this patch to be realized. It's just that we need to do this without breaking the existing code and this is actually the hardest part.

comment:49 in reply to: ↑ 48 Changed 3 years ago by R.U.10

Replying to vadz:

Replying to R.U.10:
Sorry, what's the difference? For me mdiParent is of type wxMDIParentFrame * and GetMDIParentFrame() should presumably continue to return the value of this type.

Because in my mind the attributes should remain of the base type (wxMDIParentFrameBase) but...

IMHO, wxMDIChildFrame cannot be the base class(...)

We must make this work though

...if you insist I will keep them in their current type. However I'm really worried about the class wxAuiMDIClientWindow which will inherit of wxWindow twice:

Before:

class wxMDIClientWindowBase : public wxWindow
class wxMDIClientWindow : public wxMDIClientWindowBase
class wxAuiMDIClientWindow : public wxAuiNotebook

In my patch:

class wxMDIClientWindowBase
class wxMDIClientWindow : public wxMDIClientWindowBase, public wxWindow
class wxAuiMDIClientWindow : public wxMDIClientWindowBase, public wxAuiNotebook

After your recommendations:

class wxMDIClientWindowBase : public wxWindow
class wxMDIClientWindow : public wxMDIClientWindowBase
class wxAuiMDIClientWindow : public wxMDIClientWindow

comment:50 Changed 3 years ago by vadz

It's not that I insist but we really can't break the compilation of all existing code using MDI classes by this patch. Please keep this in mind while working on it.

As for deriving from wxWindow twice: of course this would be a bad thing and can't be allowed. The simplest way to solve this would be by not making wxAuiMDIClientWindow derive from wxAuiNotebook but only from wxMDIClientWindowBase and then creating wxAuiNotebook as its child.

There might be a better, albeit much more complex, way: I think that the cleanest solution would be to make all MDI classes templates parametrized on the kind of windows they use. The compatibility would be preserved because e.g. wxMDIClientWindow would be typedef'd as (or inherit from)wxMDIAnyClientWindow<wxMDIDefaultTraits> and would have exactly the same interface as now. But we'd also have wxAuiMDIClientWindow that would be typedef'd as wxMDIAnyClientWindow<wxAuiMDITraits>. And we'd have

struct wxMDIDefaultTraits
{
    typedef wxWindow ClientWindow;
    typedef wxFrame ChildWindow;
    typedef wxFrame ParentWindow;
};

struct wxMDITabTraits
{
    typedef wxNotebook ClientWindow;
    typedef wxWindow ClientWindow;
    typedef wxFrame ParentWindow;
};

struct wxMDIAuiTraits
{
    typedef wxAuiNotebook ClientWindow;
    typedef wxPanel ClientWindow;
    typedef wxFrame ParentWindow;
};

struct 
template <class Traits>
class wxMDIAnyClientWindow : public Traits::ClientWindow
{
    virtual bool CreateClient(Traits::ParentWindow *parent,
                              long style = wxVSCROLL | wxHSCROLL) = 0;
};

and so on...

I don't know if you want to investigate this approach further but I think it should result in the best API for the user and might even be simpler to implement than the alternatives...

comment:51 follow-up: Changed 3 years ago by R.U.10

Here is a patch which implements your excellent proposal based on traits's classes.
I implemented and tested only on MSW (XP+VS2005).
I'm waiting for your feedback about the compilation on GTK...

I did not used the struct wxMDITabTraits, perhaps you should implement it at the place you thought.

In the aui docview sample there is a resizing problem with the internal window of each document which appears tiny on the top left of the view. It seems that I'm not responsible of that because the issue did not exist some revisions before. And since it appeared it impacts also the sample with the previous version of my patch...

comment:52 in reply to: ↑ 51 ; follow-up: Changed 3 years ago by ninjanl

Replying to R.U.10:

Here is a patch which implements your excellent proposal based on traits's classes.
I implemented and tested only on MSW (XP+VS2005).
I'm waiting for your feedback about the compilation on GTK...

Using MinGW on Win7 with SVN HEAD, the build fails with the latest patches.

In file included from ../../src/msw/frame.cpp:37:0:
..\..\include/wx/mdi.h: In member function 'virtual typename Traits::MDIClient* wxMDIAnyParentWindow<Traits>::OnCreateClient()':
..\..\include/wx/mdi.h:142:22: error: expected type-specifier
..\..\include/wx/mdi.h:142:22: error: expected ';'
..\..\include/wx/mdi.h: In member function 'typename Traits::MDIClient* wxMDIAnyParentWindow<Traits>::OnCreateClient() [with Traits = wxMDIDefaultTraits, typename Traits::MDIClient = wxMDIClientWindow]':
../../src/msw/frame.cpp:995:1:   instantiated from here
..\..\include/wx/mdi.h:142:22: error: cannot convert 'int*' to 'wxMDIDefaultTraits::MDIClient* {aka wxMDIClientWindow*}' in return
..\..\include/wx/mdi.h:142:22: error: dependent-name 'Traits:: MDIClient' is parsed as a non-type, but instantiation yields a type
..\..\include/wx/mdi.h:142:22: note: say 'typename Traits:: MDIClient' if a type is meant
..\..\include/wx/mdi.h:142:41: warning: control reaches end of non-void function [-Wreturn-type]
mingw32-make: *** [gcc_mswu\monolib_frame.o] Error 1

I am assuming that I only need to perform a clean checkout and apply the last two patches. Am I missing something?

Regards
mal

comment:53 in reply to: ↑ 52 ; follow-up: Changed 3 years ago by R.U.10

Replying to ninjanl:

Using MinGW on Win7 with SVN HEAD, the build fails with the latest patches.

I corrected the patch which now compiles on Cygwin.
Please let me know if it corrects your problem.

Regards,
KH

comment:54 in reply to: ↑ 53 ; follow-up: Changed 3 years ago by ninjanl

Replying to R.U.10:

Replying to ninjanl:

Using MinGW on Win7 with SVN HEAD, the build fails with the latest patches.

I corrected the patch which now compiles on Cygwin.
Please let me know if it corrects your problem.

It seems to compile OK now. However the docview sample exhibits a somewhat strange behaviour. When I create a new document, or attempt to open a document, the new wxAuiNoteBook Tab is created but the "canvas" appears to be limited to a small area in the upper left corner of the client area. I'll look into this myself, unless you know the cause yourself. I do not see this with the non-traits patch.

Regards
Mal

comment:55 in reply to: ↑ 54 Changed 3 years ago by R.U.10

Replying to ninjanl:

It seems to compile OK now. However the docview sample exhibits a somewhat strange behaviour.

That's what I tried to explain in my previous post. I don't know from where that comes because it worked well with older revisions of wx.
Anyway I modified the code of the sample to correct this behavior.

Regards,
KH

comment:56 Changed 21 months ago by R.U.10

  • Blocked By 15118 added
  • Cc kinaou.herve@… added

I updated the patches to take in account the modifications brought since #14684.

I have not tested the implementation related to gtk/mdi.cpp and especially the method wxMDIClientWindow::SetActiveChild in gtk1/mdi.cpp, if someone can check and correct it if necessary.

Note that the problem reported in #15118 is blocking since it is not possible to click on a tab without entering in an infinite loop...

comment:57 Changed 19 months ago by vadz

  • Blocked By

(In #15118) I can't reproduce this under Win7 with r73878. Does this only happen under XP?

comment:58 Changed 18 months ago by vadz

I didn't test the patches under OS X yet (has anybody already done it by chance?) but they look globally fine to me and I'm ready to apply at least the first patch, with the refactoring, even right now (again, assuming it works under OS X). But should we apply the second patch, with the changes to wxAUI code, before dealing with #14756? I think it should be fine to do it, the only file both of the patches change is src/aui/framemanager.cpp and the changes there done by this one are relatively trivial. So, any objections to applying this?

comment:59 Changed 18 months ago by vadz

  • Blocking

(In #10939) Unfortunately the patch needs to be modified to deal with the XRC refactoring (i.e. the changes of r72727, see #10996). Could anybody interested in this please do it?

I'm not going to have time to do it before 3.0 myself.

comment:60 follow-up: Changed 18 months ago by vadz

  • Milestone changed from 3.0 to 3.2

I've tested the patch now and unfortunately it doesn't work for me under wxGTK, using either plain MDI or AUI results in multiple

src/gtk/toplevel.cpp(1028): assert "Assert failure" failed in DoMoveWindow(): DoMoveWindow called for wxTopLevelWindowGTK

and the child windows are created as top level windows.

I thought that it was me who broke this by my changes to the patches, as I renamed quite a few things to be more clear (IMHO), but I didn't really change anything and, indeed, I still the same problem with the original, unmodified patch.

I've pushed my changes to Github, perhaps I broke more things than that, but at least this problem does need to be fixed.

Also, I didn't mention it before, but we absolutely need at least some documentation for this, even if it's just to mention these classes existence and to say that they should be used exactly in the same way as their non-AUI counterparts.

And I also suspect that wxOSX and other platforms using generic MDI implementations are currently broken as the changes in wx/mdi.h haven't been propagated there.

With all this being said, I am not sure we are going to have time to fix all this for 3.0. If you can do it -- all the better, the changes to the existing code look good and while I didn't really review the new code yet, if it works, it would be a good argument in its favour :-) But if you don't have time to do it, it would be another AUI improvement for 3.2.

comment:61 Changed 18 months ago by wsu

  • Blocking

(In #10939) I have posted an updated patch that will run with r74246. However, it still doesn't do anything about tests or samples.

comment:62 Changed 17 months ago by wsu

  • Blocking

(In #10939) I have added a patch that modifies the docview sample to demonstrate use of LoadDocChildFrameAny().

comment:63 Changed 17 months ago by wsu

  • Blocking

(In #10939) I forgot to state in the previous comment that the changes to the docview sample add dependencies on

wxmsw29u[d]_xrc.lib
wxmsw29u[d]_html.lib
wxmsw29u[d]_adv.lib
wxbase29u[d]_xml.lib

The .vcproj file says not to edit it, so I didn't include that in the patch. (I did verify that adding those libs to the .vcproj gave me a functioning sample.)

I apologize for leaving some of the work undone, but I don't know how to correctly add libs to the project, much less how to add them to the other OS versions.

comment:64 Changed 17 months ago by vadz

  • Blocking

(In #10939) To add the libraries docview.bkl needs to be edited and bakefile_gen reran but I can do it.

OTOH I really don't have to actually test it, could anybody please do it and provide feedback here?

TIA!

comment:65 in reply to: ↑ 60 Changed 13 months ago by R.U.10

  • Blocked By
  • Blocking
  • Patch unset

Replying to vadz:

src/gtk/toplevel.cpp(1028): assert "Assert failure" failed in DoMoveWindow(): DoMoveWindow called for wxTopLevelWindowGTK


What is wrong with the call to wxFrame::DoMoveWindow on GTK ?

The call to wxWindow::DoMoveWindow works well (is redirected to wxWindowGTK) but the wxFrame equivalent generates the assert (is redirected to wxTopLevelWindowGTK).

See aui/tabmdi.cpp, method wxAuiMDIChildFrame::ApplyMDIChildFrameRect: the call to wxAuiMDIChildFrameBase::DoMoveWindow generates the assert (wxAuiMDIChildFrameBase is the template equivalent of wxFrame).


and the child windows are created as top level windows.


This is a similar issue, in aui/tabmdi.cpp, method wxAuiMDIChildFrame::Create, if the call to wxAuiMDIParentFrameBase::Create is replaced by wxWindow::Create, the widget is well placed.

The problem here is that the GTK child frame widget has a NULL parent and thus cannot be correctly positioned inside its parent.

I tried a conditional check like that:

#ifdef __WXGTK__
    wxWindow::Create(parent,
                     id,
#else
    wxAuiMDIChildFrameBase::Create(parent,
                                   id,
                                   title,
#endif
#ifdef __WXGTK__
        wxWindow::DoMoveWindow(m_mdiNewRect.x, m_mdiNewRect.y,
#else
        wxAuiMDIChildFrameBase::DoMoveWindow(m_mdiNewRect.x, m_mdiNewRect.y,
#endif

It works but generates warnings in the Console:

(docview:2626): GLib-GObject-WARNING **: invalid cast from `wxPizza' to `GtkWindow'
(docview:2626): Gtk-CRITICAL **: gtk_window_move: assertion `GTK_IS_WINDOW (window)' failed

I'm far from being comfortable with GTK, so all advices are welcome,

Regards,
KH

Changed 13 months ago by R.U.10

Refactored base classes based on traits for Aui Doc/View integration

Changed 13 months ago by R.U.10

Aui Doc/View implementation based on traits and modified sample

comment:66 Changed 13 months ago by R.U.10

  • Patch set

I updated the patches (from wx29-svn and not from vadz git) with a beginning of documentation, the modified bakefiles, and the hack for GTK reported in the previous comment.

Note: See TracTickets for help on using tickets.