XRC reorganisation

[steve_lamerton]

I have finally started work on the patch to reorganise XRC to reduce dependencies based off the discussion on ticket #10889 and on wx-dev. I haven't managed to do much work yet but I had a couple of questions and I figured it was easier to put the code up, makes it easier to see what I am talking about.

So the questions I have so far:

1. Does it really make sense to just rename the existing xmlres.[h/cpp] to xmlresimpl.[h/cpp] considering it is really only the handler that has changed? Or perhaps leave it like it was and xmlresourcehandler in a new file called xmlreshndl? This would mean that all of the includes for the handlers would need to change though, not that that is a big problem, and it would make this huge patch much smaller and more readable.

2. I am not sure that the naming makes any sense at the moment. There is an ABC called wxXmlResourceHandler that defines a bare minimum of functions whoch would need to be overridden to create an XRC handler, plus a new method called GetImplementation which passes a pointer to a wxXmlResourceHandlerImpl. As wxXmlResourceHandlerImpl is actually just a helper class (it is not derived from wxXmlResourceHandler) does it really make sense to call it an implementation? It is more of a helper class at the moment.

3. I am not sure that I am going about the GetImplementation bit correctly, it will require all of the the calls made by the handlers to change, for example from GetBitmap() to GetImplementation->GetBitmap(). Was this what people were thinking of on the wx-dev discussion?

Thanks in advance

[vadz]

Sorry, it's really difficult to see anything in this diff... when renaming the files it probably makes sense to keep the old names while you're changing them so that diff could compare similar files with similar ones -- as it is now the patch is huge and contains mostly deletions and additions but doesn't allow to see what changed.

Naming issues aside, I don't understand your point (3) -- why should the existing handlers code change? As long as xrc library is available, all wxXmlResourceHandler helpers (e.g. IsOfClass() to take a concrete example) should still work. They will just be using wxXmlResourceHandlerImpl behind the screens but that's all. Or did I misunderstand the question?

[steve_lamerton]

I have redone the patch and moved the header for the handler out into its own file to make it much easier to see what is going on. You can safely ignore my question 3, I have no idea what I was thinking when I wrote that, you can ignore question two as well. Question one still stands and I am slightly wary to go and update all of the files if the name / location of the handler is likely to change, I think it should be fine as it is however.

Because of this at the moment in this patch I have only updated wxAnimationCtrlXmlHandler to use the correct header and a couple of the wxXmlResourceHandler methods to use the implementation. Just wanted to make sure I was heading along the right lines really.

[vadz]

Except for the typo in the name of wxXmlResourceHandleImpl (there is a missing "r") this looks like what I had in mind, thanks!

There is also the creation of m_impl which is missing which is the other non-trivial part.

[steve_lamerton]

Sorry for the long delay between updates on this patch, working on my own program seems to be taking up most of my programming time at the moment!

I have done a fair bit of work on this patch however, even though it is not yet complete, and have uploaded two separate versions, a small version that shows only the new xmlresourcehandler files and the changes to the existsing xmlres files and a complete patch that includes all of the header renaming and the replacements of variables with functions.

In summary the changes in this patch are:

- Split the existing wxXmlResourceHandler into two halves, a new wxXmlResourceHandler and wxXmlResourceHandlerImpl. wxXmlResourceHandler lives in core and wxXmlResourceHandlerImpl in the xrc library.

- Update all of the existing xrc handlers headers to point to the new wxXmlResourceHandler.

- Update all of the existing xrc handlers to use new getter functions as the variables that they used to use are now hidden in the implementation.

As is the patch allows wxWidgets to compile fine, but when linking an app, such as the xrc sample, there are a number of linker errors. Unfortunately I am not really sure how to fix them, they seem to be because the new xmlreshandler.cpp file compiled object isn't being seen, but I have added it to the bakefile and regenerated it, so any advice on what I am doing wrong would be most appreciated, although I realise it is a bit of a massive undertaking with a patch of this size!

[vadz]

One thing which seems immediately suspicious is that wxXmlResourceHandler, which is in core, is declared using WXDLLIMPEXP_XRC -- this can already account for link errors if you're using DLL/shared library build.

Beyond this I'm still not sure how is this expected to work though: as I wrote before, creation of m_impl (which is called just impl in the patch but really should start with m_ per wx coding standards guide) is the non-trivial part here. You can't just new it as you do as this would create link dependencies of core on xrc library which is precisely what we want to avoid. Instead it should remain NULL by default and be set from e.g. wxXmlResource::AddHandler() which should be in xrc library.

Also, concerning the size of the patch: it's not really necessary to make all the handlers work with the new version at once. You can temporarily disable (by removing them from/commenting them out in build/bakefiles/files.bkl) all but one of them. If you manage to make a single handler work, the others will work too.

Good luck!

[steve_lamerton]

I have uploaded a new version of the patch.

The major changes in this version are:

- Documentation updated, not sure if it is totally correct, not really worked on the docs much before
- wxXmlResourceHandler constructors have been moved into a new function called Create. Before the patch the constructors called a few wxXmlResourceHandler functions, mainly for adding supported window styles, however with the new implementation split this won’t work as we cannot just create the implementation in the constructor, or in GetImpl. I also think requiring the user to create one and pass it to the constructor is rather untidy. Instead I have moved the contents of the constructors to Create and then called this after SetImpl in wxXmlResource::[Add/Insert]Handler. As it is a pure virtual function users will get a compiler error if they have their own handlers defined, so this isn’t a silent change.
- As mentioned briefly above the wxXmlResourceHandlerImpl is now created in wxXmlResource::[Add/Insert]Handler to maintain (I hope) the split between core and xrc. It is then set with SetImpl, and Create is then called to set the handler up before it is added to the list of available handlers.
- The samples now compile and run.

Just one question left I think, should I remove the constructors now that they are empty? They seem a little redundant, but I thought I better check.

[vadz]

It looks like an important part of the patch, namely xmlreshandler.cpp was left out of it so unfortunately I can't really comment on the latest version nor test it without this file, could you please upload the full version?

To reply to your other questions, it's nice that the change to XRC handlers is not silent but OTOH it's still not very nice that all the existing handlers need to be changed. It would be great if we could avoid this somehow... Also, for compatibility reasons it would be better to keep wxXmlResourceHandler declaration in wx/xrc/xmlres.h to avoid all the changes to includes.

[steve_lamerton]

I have uploaded the patch again after making the header changes and adding the missing file, hopefully it'll work this time!

I really cannot see any way of making this work without changing the handlers. Obviously the implementation needs to be created at some point, and as you said the best place for this is in wxXmlResource::Add/InsertHander, which means that the functions carried out in the constructors will fail as they call AddStyle. So unless we require an implementation to be passed to the constructor (which IMO is far worse, at least from the point of view of the users), I just don't see how it can happen otherwise!

[vadz]

Sorry, I didn't have time to look at it yet but the patch doesn't apply currently:

% git apply --check xrcreorg.patch
xrcreorg.patch:2318: trailing whitespace.
/////////////////////////////////////////////////////////////////////////////
xrcreorg.patch:2319: trailing whitespace.
// Name:        wx/xrc/xmlreshandler.cpp
xrcreorg.patch:2320: trailing whitespace.
// Purpose:     XML resource handler
xrcreorg.patch:2321: trailing whitespace.
// Author:      Steve Lamerton
xrcreorg.patch:2322: trailing whitespace.
// Created:     2009/07/20
error: patch failed: src/xrc/xmlres.cpp:1876
error: src/xrc/xmlres.cpp: patch does not apply

Could you please redo it against the latest svn? TIA!

[steve_lamerton]

Should work this time, not quite sure what the error was about last time though.

[vadz]

Sorry, status change was accidental.

[aasselin]

(In #12058) OK i did the modif you proposed. (I don't see however why removing the call in InitAllHandler() would remove the dependency on ribbon DLL, unless there is some kind of late binding, dependency walker still shows the dependency btw)

[vadz]

Another patch with a handler which can't be applied before this one is integrated.

[aasselin]

(In #12058) just could not get the current state, can I reword the ribbon support now? is the XRC stuff in final state?

[vadz]

(In #12058) We really must do something like #10996 first. Otherwise we simply can't add support for ribbon to XRC without forcing all the applications using XRC to also link with the ribbon library which wouldn't be good.

[aasselin]

(In #12058) i'll try to read through 10996 and understand if I can help get it finalized then.

[steve_lamerton]

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]

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]

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]

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]

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]

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]

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]

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]

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]

Changed 3 WXDLLIMPEXP_XXX to WXDLLIMPEXP_FWD_XXX in xmlreshandler.h

[vadz]

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]

Still looks good to me, I look forward to seeing this committed!

[vadz]

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]

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]

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 #12058) I'm finally going to apply this soon but it would be great to fully document the new handler in docs/doxygen/overviews/xrc_format.h as it looks quite complex and I'm not entirely sure how is it supposed to work.

[vadz]

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 [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 #10889) (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.

[VZ]

(In #12058) (In [72729]) Add support for wxRibbonBar and related controls to XRC.

Add wxRibbonXmlHandler and an example of using it to the xrc sample.

Closes #12058.

[VZ]

(In #12058) (In [72755]) Disable dropdown menu support in wxRibbonXmlHandler.

The code handling it uses wxXmlNode directly as it's written now which isn't
allowed as it introduces a dependency of ribbon library on the xml one and so
currently breaks linking of the ribbon DLL under MSW.

See #12058.

[vadz]

(In #10384) This was fixed by r72727.