Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for specifying text alignment on selectables #2347

Closed
wants to merge 1 commit into from

Conversation

haldean
Copy link
Contributor

@haldean haldean commented Feb 13, 2019

Adds an optional parameter to Selectable that allows clients to specify the text alignment within the Selectable, and adds a section in the demo to demonstrate text alignment support:

screenshot

In terms of implementation, this is extremely simple: Selectable was already calling an API that supports text alignment, but had hard-coded it to top-left. This adds a text_align parameter on the Selectable function and just passes that straight through to RenderTextClipped instead of the hard-coded (0, 0). Backwards-compatibility is preserved by defaulting the text_align parameter to (0, 0), i.e., top-left.

A video of the demo in action:

screencap

Works on #1260

@ocornut
Copy link
Owner

ocornut commented Feb 14, 2019

Hello @haldean,

Could you clarify what are you using this for?
Do you typically always use the same alignment?

My intuition is we ought to avoid adding too many rarely used parameters to function, and this could be a better fit as a style variable. See style.ButtonTextAlign/ImGuiStyleVar_ButtonTextAlign ?

@haldean
Copy link
Contributor Author

haldean commented Feb 14, 2019

I am using it in a couple places in my UI; one in which I want the text to be properly centered (for a UI not unlike the one in the demo, I have a grid of selectables I want to turn on and off), and everywhere else I want it to be center-left (I have a bunch of places where I manually set the height to be larger to make it a bigger click target, and I want the text to be vertically centered in the taller box).

I'll make the change to be a style variable; using PushStyleVar for my grid UI makes sense, because it's kind of an odd thing anyway. I was just going off of what was said in #1260 on this, but I actually think the style variable makes more sense.

@ocornut
Copy link
Owner

ocornut commented Feb 14, 2019

Thank you, that would be perfect!

For the names in the demo, I would suggest using snprintf to generate a string.

I am wondering if it would make sense changing the default from (0.0f,0.0f) to (0.0f,0.5f) ?

@ocornut ocornut added the style label Feb 14, 2019
@haldean
Copy link
Contributor Author

haldean commented Feb 14, 2019

I think (0, 0.5) is a nicer default but I also was exercising some caution around changing the current behavior; if you are OK with changing behavior to have a nicer default that sounds good to me!

@ocornut
Copy link
Owner

ocornut commented Feb 14, 2019

I don't know yet to be honest. Left-align is pretty much a required default to keep, and center-left align can look a little odder than embracing upper-left?

You can do the PR with (0.0f,0.0f) and it'll be easy to change soon or later if if deem it viable.

@haldean
Copy link
Contributor Author

haldean commented Feb 14, 2019

Yeah, I just tested with (0, 0.5) and actually a number of things draw poorly; menus in particular end up looking strange. I think keeping the default at (0, 0) is the way to go for now

@ocornut
Copy link
Owner

ocornut commented Feb 14, 2019

I think the RenderTextClipped() calls needs to be changed to use:

RenderTextClipped(bb_inner.Min, bb_inner.Max, ...., align, &bb);

Passing an different clipping rectangle from the inner rectangle.
This will probably fix what you saw on the menu.

Otherwise alignment will be incorrect (more noticeable if you increase style.FramePadding).

@haldean haldean force-pushed the selectable-text-align branch from 1490435 to d948cb3 Compare February 14, 2019 18:32
{ ImGuiDataType_Float, 1, (ImU32)IM_OFFSETOF(ImGuiStyle, GrabRounding) }, // ImGuiStyleVar_GrabRounding
{ ImGuiDataType_Float, 1, (ImU32)IM_OFFSETOF(ImGuiStyle, TabRounding) }, // ImGuiStyleVar_TabRounding
{ ImGuiDataType_Float, 2, (ImU32)IM_OFFSETOF(ImGuiStyle, ButtonTextAlign) }, // ImGuiStyleVar_ButtonTextAlign
{ ImGuiDataType_Float, 2, (ImU32)IM_OFFSETOF(ImGuiStyle, SelectableTextAlign) }, // ImGuiStyleVar_SelectableTextAlign
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of these had to change because SelectableTextAlign was too long for the comment to be aligned properly

@haldean haldean force-pushed the selectable-text-align branch from d948cb3 to 7fb48aa Compare February 14, 2019 18:34
@haldean
Copy link
Contributor Author

haldean commented Feb 14, 2019

You're right, that fixed it. I still left the default at (0, 0). It's a style variable now, and I updated the demo and added the appropriate sliders to the style editor. Let me know if there's any more changes you would like to see!

@haldean haldean force-pushed the selectable-text-align branch from 7fb48aa to 6896c0b Compare February 14, 2019 18:45
Adds a style variable to Selectable that allows clients to specify the
text alignment within Selectables, adds a section in the demo to
demonstrate selectable text alignment, and a pair of sliders in the
style editor to change selectable alignment on the fly.

In terms of implementation, this one is extremely simple: Selectable was
already calling an API that supports text alignment, but had hard-coded
it to top-left. This changes that to just pass the style variable
straight through to RenderTextClipped. Backwards-compatibility is
preserved by defaulting the text_align parameter to (0, 0), i.e.,
top-left.

This also fixes a bug with selectable text rendering that caused
right-aligned text in a selectable to be clipped incorrectly, because
the wrong clipping rectangle was being used.
@ocornut
Copy link
Owner

ocornut commented Feb 14, 2019

Thanks, I'll merge it shortly!

ocornut pushed a commit that referenced this pull request Feb 14, 2019
…2347)

Adds a style variable to Selectable that allows clients to specify the
text alignment within Selectables, adds a section in the demo to
demonstrate selectable text alignment, and a pair of sliders in the
style editor to change selectable alignment on the fly.

In terms of implementation, this one is extremely simple: Selectable was
already calling an API that supports text alignment, but had hard-coded
it to top-left. This changes that to just pass the style variable
straight through to RenderTextClipped. Backwards-compatibility is
preserved by defaulting the text_align parameter to (0, 0), i.e.,
top-left.

This also fixes a bug with selectable text rendering that caused
right-aligned text in a selectable to be clipped incorrectly, because
the wrong clipping rectangle was being used.
@ocornut
Copy link
Owner

ocornut commented Feb 14, 2019

Merged now, thank you Haldean !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants