frauzufall / ofxGuiExtended

ofParameter based GUI addon for openFrameworks; derived from the core OF ofxGui addon and ofxDOM.
MIT License
112 stars 29 forks source link

Feature ofxMinimalLabel #12

Closed thomasgeissl closed 9 years ago

thomasgeissl commented 9 years ago

This is just a minimal label without having parameter.name prefixed. I needed it for a gui, I am porting. Passing an empty string to an ofxLabel was not an option, since my application is based on ofParameters and i need the parameter name somewhere else.

As mentioned in the comments, the better way might be to make the name prefix optional in ofxLabel.

frauzufall commented 9 years ago

I would create an ofxLabelExtended class that has this feature. In this way other features could be implemented as well (like font size) and later pushed to the core addon as ofxLabel.

thomasgeissl commented 9 years ago

Okay, sounds like the better solution. I will do the changes this week and push it again.

frauzufall commented 9 years ago

cool!

thomasgeissl commented 9 years ago

I pushed my changes and updated exampleControls. I just saw that you copied the original functions in your extended classes. For instance: ofxMinimalToggle implements addListener and removeListener functions. What is the reason for this redundancy? Is it already there to be easily integrated into the core? In order to be consistent, I should probably update my branch again.

frauzufall commented 9 years ago

That's an interesting question. I started to copy the functions because ofxButton does the same with some functions of ofxToggle in the core addon. But I don't really know why this is necessary. Perhaps @arturoc can tell us if we can just remove them. I would prefer that.

frauzufall commented 9 years ago

Ah no, I understand it now. ofxButton needs to have its own listener functions because it has its own valueChanged function. ofxMinimalToggle doesn't need this so I can remove the functions. I will do that and your PR is fine.

arturoc commented 9 years ago

yeah that's it, ofxButton doesn't need a value it's just a trigger so the add/removeListener functions change from adding a listener to the underlying parameter in ofxToggle to ofxButton's event

frauzufall commented 9 years ago

@thomasgeissl There is one thing not consistent with the rest - your function setShowLabelName returning a pointer to the object to avoid having a local pointer to the control. We already started to discuss this problem at https://github.com/openframeworks/openFrameworks/pull/4119. I will keep this return value as long as we have no better solution for this.

thomasgeissl commented 9 years ago

setShowLabelName should also be renamed to showLabelName to be consistent with showHeader in ofxGuiGroupExtended. https://github.com/frauzufall/ofxGuiExtended/blob/master/src/ofxGuiGroupExtended.h#L43

frauzufall commented 9 years ago

I'd rather rename showHeader to setShowHeader, I was already thinking about this.

It would actually be nice if setShowLabel would be a function of ofxBaseGui. The other controls also could use this option.