XRC handler for wxAuiNotebook

[steve_lamerton]

This patch adds an xrc handler for wxAuiNotebook, it is against the trunk. I hope the files were added to the build system correctly, I have not tried to change it before, it seemed to work for me though!

[steve_lamerton]

After rebuilding my copy of the library I now get a linker error when using it in an application that I did not get before,

libwxmsw29u_xrc.a(xrclib_xmlrsall.o):xmlrsall.cpp:(.text+0x24b): 
undefined reference to `wxAuiNotebookXmlHandler::wxAuiNotebookXmlHandler()'

Any ideas as to why this is would be appreciated, I cannot see any differences between this and the other handlers, but I expect it is something obvious.

[vadz]

I don't know if it's the same problem or not but in any case we have a problem with this patch because xrc library now depends on aui one -- so you can't use xrc without aui any more which is not good. We already had the same problem with wxRichTextCtrl but we never found any good solution for it, we need to do this now.

[steve_lamerton]

Yes, as it is a linker error is probably is the same issue, looking back through #10384 it seems like a possible solution would be to inline the class, not quite sure what you mean but if you explain it I am quite willing to try and write it!

[vadz]

(In #10996) Another patch with a handler which can't be applied before this one is integrated.

[steve_lamerton]

(In #10996) Just to say that I plan on updating this patch this week against the current SVN trunk. I will probably split it into a couple of bits again for easier reading and then hopefully if no one objects and we get a bit of review I will go ahead and apply it.

[steve_lamerton]

(In #10996) I have attached what I think is the smallest patch I can to demonstrate the changes to xrc that still compiles.

Basically the patch does a few things:

1. Disables all of the handlers apart from the wxButton handler and enables the wxRichText handler, this keeps the patch small but you will need to rebake to test it.

2. Moves the existing wxXmlResourceHandler to core and redirects all of its functions to an implementation called wxXmlResourceHandlerImpl

3. Adds wxXmlResourceHandlerImpl to the xrc library which acts as a backend for wxXmlResourceHandler

4. Updates the existing stuff to make use of the new wxXmlResourceHandler, because you can no longer access its member varaibles (as they are actually in the implementation) some changes were needed

5. Updates the button and rich text handler to compile with the new handler

6. Moves the richtext handler to the richtext library

The patch basically works in the same way that was discussed back in wx-dev  here. By moving a minimal wxXmlResourceHandler to core we can put the handlers in the library that they come from (for richtext, aui, ribbon) and link to core without needing to link them to xrc. However with this approach we are going to have breaking changes with all existing handlers. This is because we currently add styles in the constructors, but we cannot do this any more as the implementation isn't available until after the handler has been added, you can see what I mean in wxXmlResource::AddHandler

The question I do have is what is the best way to test that I have achieved the needed separation between core, xrc and (for example) richtext? Currently everything compiles fine after this patch, but what settings should I use to test it properly?

[vadz]

(In #10996) Thanks a lot, it's very helpful to have such a small patch in order to be able to see how things work.

From what I can see this looks good but I think that at least for clarity purposes it would be better to split the (now core) class wxXmlResourceHandler into its own header to separate it from wxXmlResourceHandlerImpl. I also wonder if there are dependencies of wxXmlResourceHandler on xml library, it would be nice to avoid even this.

One thing which isn't clear to me is what happens to InitAllHandlers(). The code calling it should continue to work but this function is now in core while its implementation can only be/remain in xrc. In fact this makes me wonder if we can really use the name wxXmlResourceHandler for the core class, maybe it should be called wxXmlResourceHandlerBase to allow the xrc class to keep its current name and so for wxXmlResourceHandler::InitAllHandlers() to continue to work. As all the existing XRC handlers would apparently need to be modified anyhow (I'd love to avoid this but it doesn't seem possible) I don't think this would be a big problem, would it?

Finally, as always, there are also some style/naming comments:

1. I think that using Create() is a bad idea, we already have plenty of methods called like this (in all GUI classes using 2-step construction) and these existing Create()s have a very different semantics (in particular they're not virtual). If we can't get rid of it completely by passing the impl pointer to the ctor itself it should be called something different, e.g. Initialize() or whatever.
2. m_Impl should be called m_impl as in wx variable names are supposed to start with a lower case letter (m_ doesn't count, it's a prefix).

[steve_lamerton]

(In #10996) I have updated the small patch based on the feedback. I moved wxXmlResourceHandler declaration in to a separate header file and updated the two fixed handlers to work with it. As before a rebake will be needed.

InitAllHandlers shouldn't be a problem as it is actually wxXmlResource::InitAllHandlers() (ie not the handler) so hopefully we can leave it as is.

Also I changed from Create to Initialize and fixed the variable naming.

[steve_lamerton]

(In #10996) I have uploaded a slightly enlarged version of the patch that updates and enables enough of the handlers to run the xrc sample (but not any of the functionality, just the bits needed to show the initial frame and its controls). Still works the same as I described here and the changes suggested above have been implemented.

I have some time to work on this for the next few weeks but am not sure how to take this forward, I could commit it myself but it will be a big set of changes and so I would prefer the go ahead from someone with more experience than me. I can prepare the full patch if anyone would like to see the entire set of changes, to test more thoroughly, or to commit, although it is quite large.

[vadz]

(In #10996) Thanks for the update!

I'm afraid I didn't explain my idea about splitting wx/xrc/xmlres.h clearly, while I still think it's a good idea, it shouldn't be needed to modify all the existing XRC handlers to include wx/xrc/xmlreshandler.h instead of wx/xrc/xmlres.h, so I actually thought that wx/xrc/xmlres.h would continue to include everything needed for a handler declaration, whether directly in this header itself or by including wx/xrc/xmlreshnalder.h. Hopefully this shouldn't be a big change but sorry for misleading you anyhow.

Just to make this 100% clear: even though we can't (AFAICS) avoid changes to the existing XRC handlers, we definitely want to minimize them as much as possible so please take this into consideration when working on this patch.

---

Another possibly stupid question: what do we gain from replacing m_parentWindow with GetParentAsWindow() exactly? It seems like we could keep this variable (as well as all the other ones) in wxXmlResourceHandler without any problems, couldn't we?

---

OTOH I think we have a problem with deleting m_impl in wxXmlResourceHandler dtor: this should pull in xrc code, i.e. introduce dependency on xrc library in the core. I'm really curious why/how doesn't this happen, what platform are you testing this under?

To fix this I think we have no choice but to make wxXmlResourceHandlerImpl an abstract base class implemented in core (which will be fine as the class will be trivial) and define its real implementation in xrc library which would create the object of this real derived class in wxXmlResource::AddHandler().

Again, I think it's important to keep our goal in view here: we want to have as little code as possible inside the core library to allow defining XRC handlers in other libraries without pulling xrc library in. So we can't have a dependency on it in the core itself.

---

In any case, thanks for continuing to work on this and I do hope that we can commit your changes soon!

[steve_lamerton]

(In #10996) I basically rewrote the patch over the past couple of days and I think I am finally pretty much happy with it. There are some major changes over the last patch.

1. Move the variables back to the handler in core from the implementation, this means the existing handlers do not need to be modified any more. Because all the constructors of the existing handlers do is to add a list of supported styles we can do this before the implementation has been set without any problems.

2. Add a new abstract base class called wxXmlResourceHandlerImplBase (great name I know) to core, that way wxXmlResourceHandler can take a pointer to it without adding any extra dependencies and we can set the pointer to a wxXmlResourceHandlerImpl in xrc no problem. This also fixes the problem with deleting the pointer.

Everything including the dll builds build correctly with this patch, the only thing that might want changing is making the now public variables in wxXmlResourceHandler private and accessing them with getters and setters but it would be a lot more messy in my opinion.

[steve_lamerton]

(In #10996) Now that GSOC 2011 is over I have updated this patch again, now the smallest ever! Basic description is the same as the last update, this minimises the amount of code in core whilst still giving the required separation between xrc and the other libraries.

[RedTide]

(In #10996) This patch version is basically the same as v3, I just adapted it with recent changes on SVN trunk
(GetDirection() in xmlres.cpp and wxRichTextCtrl changes in files.bkl) since I needed it for testing my wxAuiXmlHandler class and seems to work fine.

[RedTide]

(In #10996) Changed 3 WXDLLIMPEXP_XXX to WXDLLIMPEXP_FWD_XXX in xmlreshandler.h

[vadz]

(In #10996) I've (finally!) started working on applying this patch and hope to apply it soon (== before 2.9.5). Please let me know if anybody sees any problems with it. The latest version of the patch can be found at  https://github.com/vadz/wxWidgets/tree/xrc-reorg

[steve_lamerton]

(In #10996) Still looks good to me, I look forward to seeing this committed!

[vadz]

(In #10996) One thing I wonder about: would it better to have all wxXmlResourceHandler methods forwarding to GetImpl() inline in wx/xrc/xmlreshandler.h? I'm not sure either way and I don't seem to get any noticeable difference in the library sizes whether I do it like this or leave the original patch alone. I think I'd still prefer keeping them inline just because it's easier to update/maintain like this, what do you think?

[vadz]

(In #10996) I think I'm quite happy with this patch, it's really extremely simple, thanks a lot for paring it down to just this.

The only thing I'd like to do before applying it would be to add a test of wxRichTextCtrl to the xrc sample (of course, this will require linking the sample with the richtext library), just to check that it does work as expected. If you have time to do it, it would be great, otherwise I'll try to do it myself soon.

Thanks again!

[steve_lamerton]

(In #10996) With regards to inlining I don't really have a preference either way. I think in one of the earlier versions of the patch they may well have been done that way. It is easy enough to swap it over.

I'll try and get a basic example added to the sample in the next day or two.

[vadz]

(In #10996) Actually #12058 already updates the xrc sample to use ribbon XRC handler so there is no need to do anything more, thanks. I'll commit this soon.

[VZ]

(In #10996) (In [72727]) Refactor wxXRC to allow defining handlers outside of xrc library.

Split wxXmlResourceHandler into an ABC and the real implementation to allow
referencing the ABC in the core library itself but without pulling in all of
the XRC into it. This also allows defining XRC handlers, which only depend on
this ABC and not the xrc library, in other libraries, such as richtext, as
demonstrated by the now enabled wxRichTextXMLHandler.

Closes #10996.

[VZ]

(In [72728]) Add support for wxAuiNotebook to XRC.

Add wxAuiNotebookXmlHandler to "aui" library, now that we can do it without
adding a dependency of it on "xrc" one.

Closes #10889.