blender
/
blender
137
417
Fork 599
Code Issues 6.3k Pull Requests 778 Projects 19 Wiki Activity

UI: uiBut Indeterminate State #108210

Merged
Julian Eisel merged 15 commits from Harley/blender:Indeterminate into main 2023-07-17 19:37:24 +02:00
Conversation 5 Commits 15 Files Changed 4 +38 -28
3 changed files with 38 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 1a63ace6bb - Show all commits

5
source/blender/editors/include/UI_interface.h
View File

@ -86,6 +86,11 @@ typedef struct uiViewItemHandle uiViewItemHandle;
#define UI_SEP_CHAR '|'
#define UI_SEP_CHAR_S "|"
/**
* Character used when value is indeterminate (multiple, unknown, unset).
*/
#define UI_VALUE_INDETERMINATE_CHAR BLI_STR_UTF8_EM_DASH
/* Separator for text in search menus (right pointing arrow).
* keep in sync with `string_search.cc`. */
#define UI_MENU_ARROW_SEP BLI_STR_UTF8_BLACK_RIGHT_POINTING_SMALL_TRIANGLE

33
source/blender/editors/interface/interface.cc
View File

@ -3792,15 +3792,8 @@ static void ui_but_update_ex(uiBut *but, const bool validate)
/* if something changed in the button */
Harley marked this conversation as resolved Outdated
Julian Eisel commented 2023-06-09 16:52:36 +02:00
Outdated
Review
Copy Link

emdash isn't really useful either, this should be more descriptive. Would suggest something like UI_VALUE_INDETERMINATE_CHAR.

`emdash` isn't really useful either, this should be more descriptive. Would suggest something like `UI_VALUE_INDETERMINATE_CHAR`.
double value = UI_BUT_VALUE_UNSET;
Harley marked this conversation as resolved Outdated
Julian Eisel commented 2023-07-04 15:21:21 +02:00
Outdated
Review
Copy Link

Would put this in UI_interface.h, together with other defines like UI_SEP_CHAR_S.

Would put this in `UI_interface.h`, together with other defines like `UI_SEP_CHAR_S`.
/* Use emdash in place of text when in indeterminate state. */
const char *UI_VALUE_INDETERMINATE_CHAR = "\u2014";
ui_but_update_select_flag(but, &value);
if ((but->drawflag & UI_BUT_INDETERMINATE) && (but->flag & UI_SELECT)) {
but->flag &= ~UI_SELECT;
}
/* only update soft range while not editing */
if (!ui_but_is_editing(but)) {
Harley marked this conversation as resolved Outdated
Julian Eisel commented 2023-07-04 17:19:45 +02:00
Outdated
Review
Copy Link

I still can't see an explanation on why this is done, only what it does. Testing the patch, when I simply enable UI_BUT_INDETERMINATE for all buttons, I get a whole bunch of interaction issues/glitches. That is because UI_SELECT is supposed to impact the behavior, not just the drawing.

I guess this should only affect drawing (it's controlled by a draw flag after all), so this should probably be done in ui_draw_but(). At the end of that we already do similar things to safely manipulate flags for drawing.

I still can't see an explanation on _why_ this is done, only what it does. Testing the patch, when I simply enable `UI_BUT_INDETERMINATE` for all buttons, I get a whole bunch of interaction issues/glitches. That is because `UI_SELECT` is supposed to impact the behavior, not just the drawing. I guess this should only affect drawing (it's controlled by a draw flag after all), so this should probably be done in `ui_draw_but()`. At the end of that we already do similar things to safely manipulate flags for drawing.
if ((but->rnaprop != nullptr) || (but->poin && (but->pointype & UI_BUT_POIN_TYPES))) {
@ -3872,13 +3865,7 @@ static void ui_but_update_ex(uiBut *but, const bool validate)
}
}
}
if (but->drawflag & UI_BUT_INDETERMINATE) {
STRNCPY(but->drawstr, UI_VALUE_INDETERMINATE_CHAR);
but->drawflag &= ~(UI_BUT_TEXT_LEFT | UI_BUT_TEXT_RIGHT);
}
else {
STRNCPY(but->drawstr, but->str);
}
STRNCPY(but->drawstr, but->str);
}
break;
Harley marked this conversation as resolved Outdated
Julian Eisel commented 2023-06-05 12:23:24 +02:00
Outdated
Review
Copy Link

It's not clear what \u2014 is, better make this a named constant.

It's not clear what `\u2014` is, better make this a named constant.
@ -3888,11 +3875,7 @@ static void ui_but_update_ex(uiBut *but, const bool validate)
break;
}
UI_GET_BUT_VALUE_INIT(but, value);
if (but->drawflag & UI_BUT_INDETERMINATE) {
STRNCPY(but->drawstr, UI_VALUE_INDETERMINATE_CHAR);
but->drawflag &= ~(UI_BUT_TEXT_LEFT | UI_BUT_TEXT_RIGHT);
}
else if (ui_but_is_float(but)) {
if (ui_but_is_float(but)) {
ui_but_build_drawstr_float(but, value);
}
else {
@ -3915,15 +3898,9 @@ static void ui_but_update_ex(uiBut *but, const bool validate)
case UI_BTYPE_TEXT:
case UI_BTYPE_SEARCH_MENU:
if (!but->editstr) {
if (but->drawflag & UI_BUT_INDETERMINATE) {
STRNCPY(but->drawstr, UI_VALUE_INDETERMINATE_CHAR);
but->drawflag &= ~(UI_BUT_TEXT_LEFT | UI_BUT_TEXT_RIGHT);
}
else {
char str[UI_MAX_DRAW_STR];
ui_but_string_get(but, str, UI_MAX_DRAW_STR);
SNPRINTF(but->drawstr, "%s%s", but->str, str);
}
char str[UI_MAX_DRAW_STR];
ui_but_string_get(but, str, UI_MAX_DRAW_STR);
SNPRINTF(but->drawstr, "%s%s", but->str, str);
}
break;

28
source/blender/editors/interface/interface_widgets.cc
View File

@ -1937,6 +1937,19 @@ static void widget_draw_text(const uiFontStyle *fstyle,
}
}
/* If not editing and indeterminate, show dash.*/
if (but->drawflag & UI_BUT_INDETERMINATE && !but->editstr &&
ELEM(but->type,
UI_BTYPE_MENU,
UI_BTYPE_NUM,
UI_BTYPE_NUM_SLIDER,
UI_BTYPE_TEXT,
UI_BTYPE_SEARCH_MENU))
{
STRNCPY(but->drawstr, UI_VALUE_INDETERMINATE_CHAR);
Harley marked this conversation as resolved
Julian Eisel commented 2023-07-14 15:21:32 +02:00
Review
Copy Link

Do not modify but->drawstr here, the function shouldn't have such side effects. Modify the local drawstr instead.

Do not modify `but->drawstr` here, the function shouldn't have such side effects. Modify the local `drawstr` instead.
align = UI_STYLE_TEXT_CENTER;
}
Harley marked this conversation as resolved Outdated
Julian Eisel commented 2023-07-14 15:27:59 +02:00
Outdated
Review
Copy Link

This doesn't have any impact, use_right_only is already false at this point, and may only be set to true later. Same for drawstr_right.

I think a bool drawstr_overridden makes sense. When true we don't draw right aligned text. This could also be set when but->editstr is true.

This doesn't have any impact, `use_right_only` is already false at this point, and may only be set to true later. Same for `drawstr_right`. I think a `bool drawstr_overridden` makes sense. When true we don't draw right aligned text. This could also be set when `but->editstr` is true.
/* text button selection, cursor, composite underline */
if (but->editstr && but->pos != -1) {
int but_pos_ofs;
@ -5054,6 +5067,21 @@ void ui_draw_but(const bContext *C, ARegion *region, uiStyle *style, uiBut *but,
state.but_flag &= ~UI_BUT_OVERRIDDEN;
}
if ((state.but_drawflag & UI_BUT_INDETERMINATE)) {
if (state.but_flag & UI_SELECT) {
state.but_flag &= ~UI_SELECT;
}
Harley marked this conversation as resolved
Julian Eisel commented 2023-07-14 14:49:51 +02:00
Review
Copy Link

I'd remove the && (state.but_flag & UI_SELECT) part, just makes the logic look unnecessarily complex.

I'd remove the ` && (state.but_flag & UI_SELECT)` part, just makes the logic look unnecessarily complex.
if (ELEM(but->type,
UI_BTYPE_MENU,
UI_BTYPE_NUM,
UI_BTYPE_NUM_SLIDER,
UI_BTYPE_TEXT,
UI_BTYPE_SEARCH_MENU))
{
state.but_drawflag &= ~(UI_BUT_TEXT_LEFT | UI_BUT_TEXT_RIGHT);
}
}
const float zoom = 1.0f / but->block->aspect;
wt->state(wt, &state, but->emboss);
if (wt->custom) {