XRC support for ribbonbar

[aasselin]

this patch adds support for wxRibbonBar and wxRibbonXXX objects to the XRC file format.

the patch adds the xh_ribbon.cpp/.h files in .bkl
it adds the files themselves, and entries in xmlrsall.cpp to load the handler
the XRC samples .xrc file was altered to add a wxRibbonBar notebook page in the controls box

[aasselin]

adds XRC support for ribbon

[aasselin]

just a note,

[vadz]

Thanks for the patch, it looks ok to me but I don't really know this code very well so I'd appreciate a review from people who do.

I have just one small remark: could you please use wxON_BLOCK_EXIT_SET() from wx/scopeguard.h instead of this TemporaryInitializer class? It does almost the same thing and it'd be better to avoid using different ways to do the same thing in different wx files.

There is also the usual problem with adding this handler in InitAllHandlers(): this would make xrc library dependent on ribbon and we don't want to do this (the same problem as with richtext). So we should avoid doing this and instead document that people using ribbon should call AddHandler() themselves.

[aasselin]

dear Vadim,

thanks for having a look :),
regarding wxON_BLOCK_EXIT_SET, i just can't see if a macro really does what I need, or how I can write that. TemporaryInitializer does the following "save old value, put temporary value, ...., restore old value on scope exit", all of that in one elegant instructions. how to do this with scopeguard stuff?

about InitAllHandlers, OK i'll remove the code I added there

Best regards
Armel

[jmsalli]

What about the dynamic linking issue? I.e. patch, as it is, will have XRC lib depend on Ribbon lib. AFAIU this is the reason wxRichTextCtrl handler is disabled (as a comment points out in files.bkl), and I don't recall that any decision has yet been made on what to do about this.

[vadz]

Replying to aasselin:

regarding wxON_BLOCK_EXIT_SET, i just can't see if a macro really does what I need, or how I can write that. TemporaryInitializer does the following "save old value, put temporary value, ...., restore old value on scope exit", all of that in one elegant instructions. how to do this with scopeguard stuff?

It's a bit more verbose, you need to do

const T oldValue = value;
wxON_BLOCK_EXIT_SET(value, oldValue);

value = newValue;

If you think it's useful to have TemporaryInitializer, we should rename it to wxBlockScopeSetter or something like this and put it in wx/scopeguard.h. But it's really too close to the existing stuff there to duplicate it.

[vadz]

Replying to jmsalli:

What about the dynamic linking issue?

Nothing, i.e. it's still unresolved, but we have a patch for this, see #10996. Turns out I forgot to look at the latest version, somehow...

[vadz]

(In #10996) Sorry, status change was accidental.

[aasselin]

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)

[aasselin]

updated xrc handler

[vadz]

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

[aasselin]

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

[vadz]

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]

i'll try to read through 10996 and understand if I can help get it finalized then.

[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]

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]

(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 [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 [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.