API breaking consistency fixes intended for CEGUI 0.8.0

[CrazyEddie]

Ok. This is the previously discussed sticky where I shall be collecting various breaking changes that will go into 0.8.0 (svn main trunk) in order to fix API inconsistency and / or API spelling issues. Note that this does not extend to things like documentation and other supporting stuff which can be changed at any time without breaking stuff.

This first post will be edited as necessary to include all the 'approved' changes that will be going in. If it's not in this first post, but is listed in some reply, this means that it's not currently intended to go in.

All ideas, suggestions, things I said I'd do in other posts but have probably forgotten about should be posted below by you guys, so I can decide which ones should be adopted.

Finally, if anybody even mentions changing from the existing British spellings used, or adding alternative spellings, your account will be deleted and you will be permanently banned. This is not a joke, and there will be no additional warnings.

Ok, now that's out the way, what follows is the list (the crossed ones are already completed in cegui_mk2/trunk)...

• All misspelt instances of the word 'carat' will be fixed to the correct 'caret'
• Fix the inconsistent mappings from C++ symbol name to text event names (text should match C++ symbol, just without the Event prefix).
• typedef from Vector2 to Point will be removed
• typedef from GUISheet to DefaultWindow will be removed, and GUISheet renamed to DefaultWindow.
• EventMouseEnters will be renamed to EventMouseEntersSurface (old name to be removed)
• EventMouseLeaves will be renamed to EventMouseLeavesSurface (old name to be removed)
• BiDiVisualMapping to be renamed BidiVisualMapping for consistency with BidiCharType (and because BiDi is supposed to be short for Bidirectional)
• ListHeader::SegmentNameSuffix to change type from character array to CEGUI::String
• Window::EventWindowUpdated will be renamed to Window::EventUpdated

CE

[Kulik]

BidiCharType vs BiDiVisualMapping - I think BidiCharType should be changed to BiDiCharType

ListHeader::SegmentNameSuffix is array of chars, I think it should be const String as in the rest of CEGUI, this breaks bindings because size of the array isn't known from the header file

ConstBaseIterator::operator != should be !operator==(rhs); instead of !this == rhs (this is a bug, if anything it should be !(*this == rhs), could probably go into CEGUI sooner, I discovered this when binding)

I think RefCounted template class should not be declared with CEGUI_EXPORT, it creates all sorts of weird problems on MSVC (linker errors when dllimporting inline template methods/operators, etc), I am not sure about other platforms but Ogre doesn't use OGRE_EXPORT on SharedPtr as well, so I presume it's safe to omit it there

I will probably come up with more tomorrow

[CrazyEddie]

BidiCharType vs BiDiVisualMapping - I think BidiCharType should be changed to BiDiCharType

This is added to the list.

ListHeader::SegmentNameSuffix is array of chars, I think it should be const String as in the rest of CEGUI, this breaks bindings because size of the array isn't known from the header file

This is added to the list.

ConstBaseIterator::operator != should be !operator==(rhs); instead of !this == rhs (this is a bug, if anything it should be !(*this == rhs), could probably go into CEGUI sooner, I discovered this when binding)

Yeah, this looks like a bug. It will be fixed later today in v0-7

I think RefCounted template class should not be declared with CEGUI_EXPORT, it creates all sorts of weird problems on MSVC (linker errors when dllimporting inline template methods/operators, etc), I am not sure about other platforms but Ogre doesn't use OGRE_EXPORT on SharedPtr as well, so I presume it's safe to omit it there

I think this could also be considered bug-ish, if it turns out that it's totally safe to change this (and it should be) I'll remove that later today in v0-7.

Thanks, and keep them coming!

CE

[Jamarr]

Please don't hate me

1) The CEGUI namespace should be lower-cased. In my experience, this is the convention for 99% of third-party libraries that use namespaces. No one likes changing case to write namespaces. Likewise, sub-namespaces like CEGUI::WindowProperties could be renamed: cegui::window::properties; this will not conflict with cegui::Window since classes start uppercase.

2) I am not familiar with the BIDI interface, so feel free fill me in. In case of renaming Bidi > BiDi, I can only see this as being useful if the BIDI library itself uses this naming convention. Personally, I cringe whenever I see mixed-case acronyms. It is simply harder to read; same goes for all upper-case acronyms in multi-word names. Ideally, all acronyms should follow camel-case, eg: BidiCharType and BidiVisualMapping.

3) The Window::EventWindowUpdated name is redundant. This should simply be: Window::EventUpdated. Avoid redundant names when the context makes it obvious

4) In the current system event-names are unnecessarily coupled with their associated element purely for association sake, which in turn requires the word Event to be prepended to every symbol for sake of organization. Instead of defining the event-name in the elements header file, we could have a sub namespaces (similar to properties). This would allow one to reference the event-names for an element without having to pull in the element definition it self, and vice-verse (thus reducing coupling). This would also eliminate the need to have Event prepended to every event symbol. So for example cegui::Window would have it's own WindowEvents.h file with event-names defined within the cegui::window::events namespace. To access the currently labeled CEGUI::Window::EventWindowUpdated you would instead use cegui::window::events::Updated. I think this would also create a better association between the symbolic and string names since they would be equivalent (within the correct namespace).

[CrazyEddie]

Please don't hate me

1) The CEGUI namespace should be lower-cased. In my experience, this is the convention for 99% of third-party libraries that use namespaces. No one likes changing case to write namespaces. Likewise, sub-namespaces like CEGUI::WindowProperties could be renamed: cegui::window::properties; this will not conflict with cegui::Window since classes start uppercase.

Hmmm. I'm yet to be convinced on this point, but please keep trying A consensus one way or the other from other contributors will have to decide this (since I'm too chicken to make such a change and take all the responsibility )

2) I am not familiar with the BIDI interface, so feel free fill me in. In case of renaming Bidi > BiDi, I can only see this as being useful if the BIDI library itself uses this naming convention. Personally, I cringe whenever I see mixed-case acronyms. It is simply harder to read; same goes for all upper-case acronyms in multi-word names. Ideally, all acronyms should follow camel-case, eg: BidiCharType and BidiVisualMapping.

Either is fine with me, so long as it's consistent. Since it's short for bidirectional, I guess that Bidi is more accurate. I'll update the change to reflect this

3) The Window::EventWindowUpdated name is redundant. This should simply be: Window::EventUpdated. Avoid redundant names when the context makes it obvious

Yeah, definitely. This was mentioned on IRC also.

4) In the current system event-names are unnecessarily coupled with their associated element purely for association sake, which in turn requires the word Event to be prepended to every symbol for sake of organization. Instead of defining the event-name in the elements header file, we could have a sub namespaces (similar to properties). This would allow one to reference the event-names for an element without having to pull in the element definition it self, and vice-verse (thus reducing coupling). This would also eliminate the need to have Event prepended to every event symbol. So for example cegui::Window would have it's own WindowEvents.h file with event-names defined within the cegui::window::events namespace. To access the currently labeled CEGUI::Window::EventWindowUpdated you would instead use cegui::window::events::Updated. I think this would also create a better association between the symbolic and string names since they would be equivalent (within the correct namespace).

I see the merits of certain aspects of this, but my initial reaction to lots of nested namespaces is to resist. Again I think I need more input / convincing on this issue.

CE

[Kulik]

I personaly don't have any problems with CEGUI namespace, I think CEGUI::Window looks nicer than cegui::Window but that's completely subjective

I agree with "Bidi"

I think Events and Properties have few things in common and could be reorganised so it fits. Events have namespaces, Properties should also have namespaces (will also make it easier to edit them in editors), in my opinion, Events shouldn't be auto-added, this could create "silent bugs" where somebody misspells an event name and nothing is outputted about it, it just won't work.
So I would do it like this (lot of further discussion is needed), addProperty and addEvent, addStandardProperties and addStandardEvents, etc.. I would unify the way these two are registered and handled. I will think of some more ideas with the events, like having the ability to chain events (pre, post, actual - Jamarr's idea from a different thread).

As for the Event prefix, I like the syntax. It plays well with namespaces, but as I said in above, these could be reorganised a bit.

[emarcotte]

I don't know which namespace style I like more, it is true there are more namespaces out there all lower case, but then again, there are also many with upper case (Ogre, Poco project, others I can't remember off the top of my head). ctags is smart enough to autocomplete it either way in vim, so I don't mind either way

[Jamarr]

The namespace style is mainly cosmetic; indeed there are several libraries that all follow different conventions: alllowercase, CamelCase, camelCase, UPPERCASE, etc. The reasoning behind most is purely aesthetic. However, two points worth some actual merit: 1) the standard-library uses all lower-case for namespaces and 2) it is undeniably quicker and easier to type without having to change case. I think these two alone are enough to make the case over purely aesthetic reasons.

As far as nested namespaces are concerned, you are right in that they should be avoided in most cases. However, that does not mean they serve no purpose. So long as their intent is to prevent naming collisions via separation their use is well within reason. And it has been shown that CEGUI already uses nested namespaces (CEGUI::WindowProperties::*). The purpose of nesting namespaces is mainly for separation and cleaning up scope. Pretend for a moment that events where separated into their own namespace, perhaps CEGUI::WindowEvents for consistency sake. This is a step in the right direction, but we can still improve on this. By using CEGUI::WindowProperties and CEGUI::WindowEvents there are now two window-related namespaces in the CEGUI scope; these are minor now, but are still a form of scope pollution. By introducing a middle-man, CEGUI::Window, we have removed one more unnecessary name from the core CEGUI scope. Now imagine the need for CEGUI::WindowAnimations, CEGUI::WindowLayouts, CEGUI::WindowPolicy or what have you; the existing convention has potential to bloat the core namespace. Now, I do believe in YAGNI and if WindowEvents do not need their own namespace then stick with what you got.

Now as far as decoupling Window::Event* from Window it is really very simple: unnecessary coupling. If you can avoid coupling two objects, why purposefully couple them? The Window header has absolutely no use for the event names; they are only used in the implementation. Allow others to reference event names without having to pull in everything required by the Window interface. For example, exporting event-names or binding to global events without having to pull in associated element headers. This also avoids the need to recompile code using the Window interface when adding unrelated events. This can be done by moving the events from the Window class definition into their own file, WindowEvents.h, and giving them a proper namespace ala CEGUI::WindowEvents or cegui::window::events

[Kulik]

Now this is a big one (please don't hate me):

There is a big inconsistency between the names "Window", "Widget", "WidgetLookNFeel", etc... Falagard uses the name Widget for Windows, whilst CEGUIBase uses Windows. I think these should be unified but it's going to break lots of existing code. I vote to rename CEGUI::Window to CEGUI::Widget and WindowManager to WidgetManager, etc, but I would be happy the other way around too, renaming falagard WidgetLookNFeel to WindowLookNFeel, etc. I think it creates lot of confusion for the newcomers because they don't immediately connect Window and Widget and they make false assumption that Window is "bigger" than Widget (I had this problem myself). Most libraries nowadays use the name Widget and use "Window" for current "FrameWindow". Now this is a lot of source code changes so if this gets the vote, I will at least create the patch for it to save you some trouble (you're the one they're gonna hate for it though, can't do anything about that ). This change will also make it harder to merge existing patches to trunk, so IMO we shouldn't immediately do it. Maybe when 0.8.0 release gets really close.

[kornerr]

I want to clarify the namespace change.
If all we get with that is typing two more dots everywhere, there's no use of it, because in this case we could just as well use two dashes or any 2 other alphanumerical characters.

The correct usage that I found so far is as follows.
I have a Game, Scene classes which represent subsystems. Both Game and Scene in my case need to have Tiles - in Game space (logical) and Scene space (visual). So that's when I use namespaces: I have game::Tile only visible to Game, and scene::Tile only visible to Scene. Thus, such architecture implies that scene:: namespace contains implementation details for Scene class that are of no importance to other subsystems (Game, Application, etc).

[Jamarr]

I agree with CEGUI::Widget being more intuitive than CEGUI::Window. The accepted gui term for an abstract element is widget. If we look at the core definitions of each word: A window is an opening > an area for information to reside and/or pass through > a means of accessing information. A widget is intentionally abstract > a gadget > a thing > some piece of the pie. A window is a widget > everything is a widget; but a widget is not a widow and it certainly is not everything

[Jamarr]

I want to clarify the namespace change. If all we get with that is typing two more dots everywhere, there's no use of it, because in this case we could just as well use two dashes or any 2 other alphanumerical characters.

No one has said anything about introducing namespaces to simplify typing. I think you are confusing two separate issues here: 1) namespace style and 2) decoupling Window::Event names by moving them into a separate header and namespace.

[CrazyEddie]

Just a note to say that I'm not ignoring the thread, just giving it some time to grow a bit, and for some of the ideas to rattle around in my head for a while

CE.

[Kulik]

As discussed on IRC:
PropertyHelper should be reimplemented with templates.
PropertyHelper::floatToString becomes PropertyHelper::toString<float>, PropertyHelper::stringToFloat becomes PropertyHelper::fromString<float>
This little change will shrink BasicInterpolators code to 2 templated interpolators - discrete and linear.

Plus another idea
If properties' string representation contained hint about their datatype (perhaps UDim(0, 0) instead of {0,0}, UVector2(UDim(0, 0), UDim(0, 0)) instead of {{0,0}, {0,0}}), we could create expression parser to dynamically set properties based on other properties (for example making additional widgets inside visible when width is greater than something, changing position of widgets inside based on UnifiedArea, etc). This will also make animations much more powerful because keyframes could have expressions instead of values. The major drawback is that the new string representation of property types is more verbose and will make doing layouts in XML by hand a bit harder (but more readable IMO, plus it is less error prone since we can check datatypes with Property's d_dataType). Migrating existing layouts will possibly be hard too, since you can't distinguish between Size and UVector2 for example.

I would also change cegui_reldim macro to UDim CEGUI::reldim(float) or even completely remove it and encourage direct use of UDim.

[MarkR]

As discussed on IRC:
PropertyHelper should be reimplemented with templates.
PropertyHelper::floatToString becomes PropertyHelper::toString<float>, PropertyHelper::stringToFloat becomes PropertyHelper::fromString<float>
This little change will shrink BasicInterpolators code to 2 templated interpolators - discrete and linear.

How about kicking the whole PropertyHelper and implement something like boost::lexical_cast instead?