[POLL] Rename IGUIElement::setRelativePosition(rect<f32&g

[rogerborg]

For those who didn't know, there are two radically different ways of setting a GUI element's position.

Code: Select all

//! Sets the relative rectangle of this element.
void setRelativePosition(const core::rect<s32>& r)

//! Sets the relative rectangle of this element.
void setRelativePosition(const core::rect<f32>& r)

Now, without reading the implementation, which one does what?

If you said "setRelativePosition(const core::rect<f32>& r) sets the element's position and size as a proportion of its parent's size then congratulations: you are either psychic, or you wrote the method.

We could comment that method to explain what it does, but it still wouldn't be obvious. Instead, I suggest that we deliberately break it, and rename it to:

Code: Select all

	//! Sets the relative rectangle of this element as a proportion of its parent's area.
	/** \param r  The rectangle to set, interpreted as a proportion of the parent's area.  
	Meaningful values are in the range [0...1], unless you intend this element to spill 
	outside its parent. */
	void setProportionalPosition(const core::rect<f32>& r)

[Yoran]

Agree!

[rogerborg]

Well, that was pretty resounding. Committed in SVN 1777, thanks for the feedback.

[Mel]

Hmmm, why instead of breaking the compatibility renaming the method, you create a macro for it so you can call it what you feel is right? I mean. Changing the name of a method is the best way to break backwards compatibility, which, i think, it is not the most desirable behavior in any engine. Or perhaps, renaming it and leaving a small macro for backwards compatibilty. Although i agree that "relative position" is not very representative.

[Dorth]

It is a complete fail at a name, sorry to be blunt. It not only doesn't describe accurately what's going on, it actually lies about it, unless you have a very tolerant acceptance of the terms. Thus it should be renamed as a bug, not even an improvement and fixed as such.

[MasterGod]

First, I think its pretty obvious that if it's float it means it would be between 0 to 1 because those are the only reasonable values in my opinion.

Second, I think its not so obvious to the majority and so the name of the method should be changed.

Third, I agree with Dorth that this name is really bad because if the word relative with the type float was a hint to what the method does, now it completely confuses me.
Sorry I can't figure a better name but this one (setProportionalPosition) is improper for the current functionality I believe so.

[Dorth]

While I was actually against the original name, I must admit I am equally against the new and my argument could hold for both. How about

setPositionProportionalToParent

Long and tedious, but people can define it shorter, it says what it must and is no longer confusing.

[rogerborg]

Please keep them coming... I've still got an open mind on this.

[hybrid]

I suggested setRelativePositionProportional on IRC. This subsumes the relation to parent, because that's the meaning at least in Irrlicht. But I wouldn't care with the new name, either.

[Dark_Kilauea]

Hybrid's name sounds a lot better and gets the point across that it is based on the parent (and follows the naming convention of Irrlicht as a whole)

[rogerborg]

Changed to setRelativePositionProportional() in SVN 1790.