UI: Input Placeholders #112104

Harley Acheson · 2023-09-07T21:12:31+02:00

Harley Acheson commented

2023-09-07 21:12:31 +02:00

Optional inline hint describing the expected value of an input field.

Implementation of input placeholders, proposed in #111436. This only shows the placeholder when the input is empty and not editing, at 33% opacity of the text color.

Following shows it being set in space_outliner.py:

row.prop(space, "filter_text", icon='VIEWZOOM', text="", placeholder="Search")

Optional inline hint describing the expected value of an input field. --- Implementation of input placeholders, proposed in #111436. This only shows the placeholder when the input is empty and not editing, at 33% opacity of the text color. ![image](/attachments/a76fb3bd-1e78-4b6b-bb76-5dc5feec07c9) Following shows it being set in `space_outliner.py`: ``` row.prop(space, "filter_text", icon='VIEWZOOM', text="", placeholder="Search") ```

Placeholders.gif

91 KiB

Placeholders.gif

85 KiB

👍 7 ❤️ 3 🎉 1

Harley Acheson added 1 commit 2023-09-07 21:12:44 +02:00

74b332146e WIP: UI: Input Placeholders

Demonstration (only) of input placeholders.

Harley Acheson added this to the User Interface project 2023-09-07 21:12:57 +02:00

Harley Acheson requested review from Pablo Vazquez 2023-09-07 21:13:06 +02:00

Pablo Vazquez commented

2023-09-10 19:38:52 +02:00

Nice to have this prototype, helps a lot to actually see it working.

I'm not sure about the use of italics. It's not standard (nor on web or apps) and if we ever use italics for something else it'd conflict (one of the the most popular RCS posts refers to it).

Here is a comparison with the following changes:

Regular text, no italics.
Slightly dimmer 0.33f.
No shadow. There's no need for it and it helps to "blend" as part of the input.

Patch	Tweaked

     uiFontStyleDraw_Params params{};
     params.align = align;
     uiFontStyle style = *fstyle;
-    style.italic = true;
+    style.shadow = 0;
     uchar col[4];
     copy_v4_v4_uchar(col, wcol->text);
-    col[3] *= 0.4f;
+    col[3] *= 0.33f;
     int font_xofs, font_yofs;
     UI_fontstyle_draw_ex(&style,

On a light theme.

Nice to have this prototype, helps a lot to actually see it working. I'm not sure about the use of italics. It's not standard (nor on web or apps) and if we ever use italics for something else it'd conflict (one of the [the most popular RCS posts](https://blender.community/c/rightclickselect/Btcbbc/) refers to it). Here is a comparison with the following changes: * Regular text, no italics. * Slightly dimmer `0.33f`. * No shadow. There's no need for it and it helps to "blend" as part of the input. |Patch|Tweaked| |----|----| |![Patch](/attachments/8516823a-a951-494e-9a75-ebde715eaac2)|![Dark Theme](/attachments/16422b7e-2e9f-414d-949c-84fa1c1153b9)| ```diff uiFontStyleDraw_Params params{}; params.align = align; uiFontStyle style = *fstyle; - style.italic = true; + style.shadow = 0; uchar col[4]; copy_v4_v4_uchar(col, wcol->text); - col[3] *= 0.4f; + col[3] *= 0.33f; int font_xofs, font_yofs; UI_fontstyle_draw_ex(&style, ``` On a light theme. ![Light Theme](/attachments/4d7c5618-7b80-4bd5-a8e0-75a9f654aa6c)

image.png

104 KiB

image.png

104 KiB

image.png

98 KiB

image.png

99 KiB

image.png

98 KiB

image.png

98 KiB

image.png

104 KiB

image.png

99 KiB

Harley Acheson added 2 commits 2023-09-10 21:08:57 +02:00

fbc25c99ad Merge branch 'main' of projects.blender.org:blender/blender into placeholders

849fd85ed5 Changes suggested by Pablo

Harley Acheson commented

2023-09-10 21:13:13 +02:00

@pablovazquez - Regular text, no italics... Slightly dimmer 0.33f... No shadow.

Thanks! I updated this PR and the capture in the first comment.

> @pablovazquez - Regular text, no italics... Slightly dimmer `0.33f`... No shadow. Thanks! I updated this PR and the capture in the first comment.

👍 1

Harley Acheson added 2 commits 2023-09-11 04:58:11 +02:00

3bad3969cd Merge branch 'main' of projects.blender.org:blender/blender into placeholders

e28c2205b3 Mostly working...

Harley Acheson commented

2023-09-11 05:05:13 +02:00

@pablovazquez

I have updated this PR further to work more-or-less correctly, although the details of the implementation could change.

All the hardcoded text is gone and are all set like this:

        layout.prop(scene, "camera", placeholder="Object")
        layout.prop(scene, "background_set", placeholder="Scene")
        layout.prop(scene, "active_clip", text="Active Clip", placeholder="Movie Clip")

@pablovazquez I have updated this PR further to work more-or-less correctly, although the details of the implementation could change. All the hardcoded text is gone and are all set like this: ``` layout.prop(scene, "camera", placeholder="Object") layout.prop(scene, "background_set", placeholder="Scene") layout.prop(scene, "active_clip", text="Active Clip", placeholder="Movie Clip") ```

Pablo Vazquez commented

2023-09-11 16:01:12 +02:00

This is great! Super easy to add.

Design-wise I think this is good to go.

This is great! Super easy to add. Design-wise I think this is good to go. <video src="/attachments/308ab850-16d3-493a-a2fb-527f4207172a" title="add_placeholder.mp4" controls></video>

add_placeholder.mp4

259 KiB

🎉 2

Pablo Vazquez approved these changes 2023-09-11 16:01:23 +02:00

Harley Acheson added 1 commit 2023-09-11 17:53:45 +02:00

63d0193763 Cleanup and comment improvements.

Harley Acheson changed title from ~~WIP: UI: Input Placeholders~~ to UI: Input Placeholders

2023-09-11 17:55:06 +02:00

Harley Acheson requested review from Julian Eisel 2023-09-11 17:55:25 +02:00

Julian Eisel commented

2023-09-12 14:39:32 +02:00

Great!

I think we can and should have a default placeholder for many cases. For example the pointer properties and ID templates can get this from the RNA struct UI name. In search buttons the default can be "Search". The placeholder parameter can then be an override.

Great! I think we can and should have a default placeholder for many cases. For example the pointer properties and ID templates can get this from the RNA struct UI name. In search buttons the default can be "Search". The `placeholder` parameter can then be an override.

👍 1

Harley Acheson added 2 commits 2023-09-12 21:29:48 +02:00

282a5fdf6d Merge branch 'main' of projects.blender.org:blender/blender into placeholders

cc5fded665 Default placeholders

Pablo Vazquez commented

2023-09-12 21:44:58 +02:00

I think we can and should have a default placeholder for many cases.

Good point. It'd be akin to how operators/props/enums display a text label unless we do text="".

> I think we can and should have a default placeholder for many cases. Good point. It'd be akin to how operators/props/enums display a text label unless we do `text=""`.

Harley Acheson commented

2023-09-12 21:53:18 +02:00

@JulianEisel @pablovazquez

I have updated this PR to remove all the changes to the python files that had added placeholders. Instead they are all set automatically, in a new UI_but_placeholder_get function. You can still explicitly set a placeholder to override those defaults.

I'm not sure if my determination of defaults is ideal or not. For buts of type UI_BTYPE_SEARCH_MENU If use the Id names, for UI_BTYPE_TEXT I check the property identifier against "search_filter", "filter_text", "filter_search" to set "Search". These all seem to work correctly when translated.

@JulianEisel @pablovazquez I have updated this PR to remove all the changes to the python files that had added placeholders. Instead they are all set automatically, in a new `UI_but_placeholder_get` function. You can still explicitly set a placeholder to override those defaults. I'm not sure if my determination of defaults is ideal or not. For buts of type UI_BTYPE_SEARCH_MENU If use the Id names, for UI_BTYPE_TEXT I check the property identifier against "search_filter", "filter_text", "filter_search" to set "Search". These all seem to work correctly when translated.

Damien Picard commented

2023-09-14 03:32:45 +02:00

This will allow the extraction of those new messages to the .po translation files:

diff --git a/scripts/modules/bl_i18n_utils/bl_extract_messages.py b/scripts/modules/bl_i18n_utils/bl_extract_messages.py
index 48b7d8f7cfb..fe3c58aa19c 100644
--- a/scripts/modules/bl_i18n_utils/bl_extract_messages.py
+++ b/scripts/modules/bl_i18n_utils/bl_extract_messages.py
@@ -588,6 +588,7 @@ def dump_py_messages_from_files(msgs, reports, files, settings):
                   ),
         "message": (),
         "heading": (),
+        "placeholder": (),
     }
 
     context_kw_set = {}
diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc
index a7ff92e0e58..4eaba75263f 100644
--- a/source/blender/editors/interface/interface.cc
+++ b/source/blender/editors/interface/interface.cc
@@ -5930,7 +5930,7 @@ const char *UI_but_placeholder_get(uiBut *but)
     else if (but->type == UI_BTYPE_TEXT) {
       const char *identifier = RNA_property_identifier(but->rnaprop);
       if (STR_ELEM(identifier, "search_filter", "filter_text", "filter_search")) {
-        placeholder = "Search";
+        placeholder = N_("Search");
       }
     }
   }

I have a slight concern about the translation of these messages: they can currently only use the default translation context. This means that there is no way to distinguish between "[The] search" (noun) and "[Do] search" (verb), which at least in French makes this specific message sound off already. And I’m sure other placeholders can use equally short messages, which will bring up the same kind of issue.

But then again it may be overkill to add a placeholder_ctxt argument just for that. Maybe it could reuse the existing text_ctxt? It should hopefully reduce the probability of a conflict enough.

This will allow the extraction of those new messages to the .po translation files: ```diff diff --git a/scripts/modules/bl_i18n_utils/bl_extract_messages.py b/scripts/modules/bl_i18n_utils/bl_extract_messages.py index 48b7d8f7cfb..fe3c58aa19c 100644 --- a/scripts/modules/bl_i18n_utils/bl_extract_messages.py +++ b/scripts/modules/bl_i18n_utils/bl_extract_messages.py @@ -588,6 +588,7 @@ def dump_py_messages_from_files(msgs, reports, files, settings): ), "message": (), "heading": (), + "placeholder": (), } context_kw_set = {} diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc index a7ff92e0e58..4eaba75263f 100644 --- a/source/blender/editors/interface/interface.cc +++ b/source/blender/editors/interface/interface.cc @@ -5930,7 +5930,7 @@ const char *UI_but_placeholder_get(uiBut *but) else if (but->type == UI_BTYPE_TEXT) { const char *identifier = RNA_property_identifier(but->rnaprop); if (STR_ELEM(identifier, "search_filter", "filter_text", "filter_search")) { - placeholder = "Search"; + placeholder = N_("Search"); } } } ``` I have a slight concern about the translation of these messages: they can currently only use the default translation context. This means that there is no way to distinguish between "[The] search" (noun) and "[Do] search" (verb), which at least in French makes this specific message sound off already. And I’m sure other placeholders can use equally short messages, which will bring up the same kind of issue. But then again it may be overkill to add a `placeholder_ctxt` argument just for that. Maybe it could reuse the existing `text_ctxt`? It should hopefully reduce the probability of a conflict enough.

Harley Acheson added 2 commits 2023-09-14 04:52:02 +02:00

97c67578dc Merge branch 'main' of projects.blender.org:blender/blender into placeholders

6d7c84df49 Changes requested by Damien Picard

Harley Acheson commented

2023-09-14 04:53:14 +02:00

@pioverfour

I updated the PR with your suggested changes above. I'm not sure what to do about context though, and would appreciate any help and guidance you can provide.

@pioverfour I updated the PR with your suggested changes above. I'm not sure what to do about context though, and would appreciate any help and guidance you can provide.

Damien Picard commented

2023-09-14 23:41:02 +02:00

Here is what I would do for the translation part:

diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc
index 4eaba75263f..7bb8de579d9 100644
--- a/source/blender/editors/interface/interface.cc
+++ b/source/blender/editors/interface/interface.cc
@@ -5926,16 +5926,17 @@ const char *UI_but_placeholder_get(uiBut *but)
       if (idcode != 0) {
         RNA_enum_name(rna_enum_id_type_items, idcode, &placeholder);
       }
+      placeholder = CTX_IFACE_(BLT_I18NCONTEXT_ID_ID, placeholder);
     }
     else if (but->type == UI_BTYPE_TEXT) {
       const char *identifier = RNA_property_identifier(but->rnaprop);
       if (STR_ELEM(identifier, "search_filter", "filter_text", "filter_search")) {
-        placeholder = N_("Search");
+        placeholder = CTX_IFACE_(BLT_I18NCONTEXT_ID_WINDOWMANAGER, "Search");
       }
     }
   }
 
-  return IFACE_(placeholder);
+  return placeholder;
 }
 
 void UI_but_type_set_menu_from_pulldown(uiBut *but)
diff --git a/source/blender/makesrna/intern/rna_ui_api.cc b/source/blender/makesrna/intern/rna_ui_api.cc
index bb2f004cf78..e464288c562 100644
--- a/source/blender/makesrna/intern/rna_ui_api.cc
+++ b/source/blender/makesrna/intern/rna_ui_api.cc
@@ -109,6 +109,7 @@ static void rna_uiItemR(uiLayout *layout,
 
   /* Get translated name (label). */
   name = rna_translate_ui_text(name, text_ctxt, nullptr, prop, translate);
+  placeholder = rna_translate_ui_text(placeholder, text_ctxt, nullptr, prop, translate);
 
   if (slider) {
     flag |= UI_ITEM_R_SLIDER;

I’m not sure at all the code is correct, it’s just the way I could make it work the way I think makes sense:

When a context is specified in layout code, translate from rna_translate_ui_text() using the existing text_ctxt argument.
When trying to find a preset in UI_but_placeholder_get(), translate using already known contexts:
- The ID types all use the BLT_I18NCONTEXT_ID_ID context, and are already extracted elsewhere;
- The hardcoded Search can have a hardcoded context as well, say BLT_I18NCONTEXT_ID_WINDOWMANAGER because the search is expected to happen in the UI, and the WM context is the closest we have to that.

I’m not sure how familiar you are with the translation logic, let me know if anything is unclear.

This handles the translation part, but the extraction part remains (UI code parsing from bl_extract_messages.py). I couldn’t make the extraction work using text_ctxt yet…

However if you don’t want this blocking the patch, I don’t expect there will be hundreds of placeholders soon so I think it’s fine to proceed with the change as is, without the disambiguation. We can improve it at a later stage. And @mont29 will probably have ideas on how to proceed, too.

EDIT: If you want to test that this works, you can put the attached object translation file in your user’s blender/4.0/datafiles/locale/fr/LC_MESSAGES/ dir, and switch the language to French. It includes a "Search" translation with "WindowManager" context. This should make the search bar display "Placeholder with context!".

Here is what I would do for the translation part: ```diff diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc index 4eaba75263f..7bb8de579d9 100644 --- a/source/blender/editors/interface/interface.cc +++ b/source/blender/editors/interface/interface.cc @@ -5926,16 +5926,17 @@ const char *UI_but_placeholder_get(uiBut *but) if (idcode != 0) { RNA_enum_name(rna_enum_id_type_items, idcode, &placeholder); } + placeholder = CTX_IFACE_(BLT_I18NCONTEXT_ID_ID, placeholder); } else if (but->type == UI_BTYPE_TEXT) { const char *identifier = RNA_property_identifier(but->rnaprop); if (STR_ELEM(identifier, "search_filter", "filter_text", "filter_search")) { - placeholder = N_("Search"); + placeholder = CTX_IFACE_(BLT_I18NCONTEXT_ID_WINDOWMANAGER, "Search"); } } } - return IFACE_(placeholder); + return placeholder; } void UI_but_type_set_menu_from_pulldown(uiBut *but) diff --git a/source/blender/makesrna/intern/rna_ui_api.cc b/source/blender/makesrna/intern/rna_ui_api.cc index bb2f004cf78..e464288c562 100644 --- a/source/blender/makesrna/intern/rna_ui_api.cc +++ b/source/blender/makesrna/intern/rna_ui_api.cc @@ -109,6 +109,7 @@ static void rna_uiItemR(uiLayout *layout, /* Get translated name (label). */ name = rna_translate_ui_text(name, text_ctxt, nullptr, prop, translate); + placeholder = rna_translate_ui_text(placeholder, text_ctxt, nullptr, prop, translate); if (slider) { flag |= UI_ITEM_R_SLIDER; ``` I’m not sure at all the code is correct, it’s just the way I could make it work the way I think makes sense: - When a context is specified in layout code, translate from `rna_translate_ui_text()` using the existing `text_ctxt` argument. - When trying to find a preset in `UI_but_placeholder_get()`, translate using already known contexts: - The ID types all use the `BLT_I18NCONTEXT_ID_ID` context, and are already extracted elsewhere; - The hardcoded `Search` can have a hardcoded context as well, say `BLT_I18NCONTEXT_ID_WINDOWMANAGER` because the search is expected to happen in the UI, and the WM context is the closest we have to that. I’m not sure how familiar you are with the translation logic, let me know if anything is unclear. This handles the translation part, but the extraction part remains (UI code parsing from bl_extract_messages.py). I couldn’t make the extraction work using `text_ctxt` yet… However if you don’t want this blocking the patch, I don’t expect there will be hundreds of placeholders soon so I think it’s fine to proceed with the change as is, without the disambiguation. We can improve it at a later stage. And @mont29 will probably have ideas on how to proceed, too. ----- EDIT: If you want to test that this works, you can put the attached object translation file in your user’s `blender/4.0/datafiles/locale/fr/LC_MESSAGES/` dir, and switch the language to French. It includes a "Search" translation with "WindowManager" context. This should make the search bar display "Placeholder with context!". ![image](/attachments/e7cf4b0b-971f-4dcc-b5cd-60c746b2a080)

blender.mo

3.0 MiB

image.png

18 KiB

Harley Acheson added 2 commits 2023-09-15 05:00:54 +02:00

a3c9bbb747 Merge branch 'main' of projects.blender.org:blender/blender into placeholders

6710f073aa Translation-related changes suggested by Damien Picard

Harley Acheson commented

2023-09-15 05:05:10 +02:00

@pioverfour

Thanks for the help.

I’m not sure how familiar you are with the translation logic, let me know if anything is unclear.

It's making more sense now with your help. I did have translation going on in rna_ui_api.cc early on (probably without context), but then when I added UI_but_placeholder_get it made sense to do it there there, but maybe not in both places. I can see now that it has to be done differently in both places to work correctly.

Thanks again!

@pioverfour Thanks for the help. > I’m not sure how familiar you are with the translation logic, let me know if anything is unclear. It's making more sense now with your help. I did have translation going on in `rna_ui_api.cc` early on (probably without context), but then when I added `UI_but_placeholder_get` it made sense to do it there there, but maybe not in both places. I can see now that it has to be done differently in both places to work correctly. Thanks again!

Damien Picard commented

2023-09-15 14:30:26 +02:00

This should do it for the extraction part! (CC @mont29)

diff --git a/scripts/modules/bl_i18n_utils/bl_extract_messages.py b/scripts/modules/bl_i18n_utils/bl_extract_messages.py
index fe3c58aa19c..8716a149f46 100644
--- a/scripts/modules/bl_i18n_utils/bl_extract_messages.py
+++ b/scripts/modules/bl_i18n_utils/bl_extract_messages.py
@@ -588,7 +588,8 @@ def dump_py_messages_from_files(msgs, reports, files, settings):
                   ),
         "message": (),
         "heading": (),
-        "placeholder": (),
+        "placeholder": ((("text_ctxt",), _ctxt_to_ctxt),
+                        ),
     }
 
     context_kw_set = {}
@@ -622,7 +623,8 @@ def dump_py_messages_from_files(msgs, reports, files, settings):
         for arg_pos, (arg_kw, arg) in enumerate(func.parameters.items()):
             if (not arg.is_output) and (arg.type == 'STRING'):
                 for msgid, msgctxts in context_kw_set.items():
-                    if arg_kw in msgctxts:
+                    # The msgid can be missing if it is used in only some UILayout functions but not all
+                    if arg_kw in msgctxts and msgid in func_translate_args[func_id]:
                         func_translate_args[func_id][msgid][1][arg_kw] = arg_pos
     # The report() func of operators.
     for func_id, func in bpy.types.Operator.bl_rna.functions.items():

This should do it for the extraction part! (CC @mont29) ```diff diff --git a/scripts/modules/bl_i18n_utils/bl_extract_messages.py b/scripts/modules/bl_i18n_utils/bl_extract_messages.py index fe3c58aa19c..8716a149f46 100644 --- a/scripts/modules/bl_i18n_utils/bl_extract_messages.py +++ b/scripts/modules/bl_i18n_utils/bl_extract_messages.py @@ -588,7 +588,8 @@ def dump_py_messages_from_files(msgs, reports, files, settings): ), "message": (), "heading": (), - "placeholder": (), + "placeholder": ((("text_ctxt",), _ctxt_to_ctxt), + ), } context_kw_set = {} @@ -622,7 +623,8 @@ def dump_py_messages_from_files(msgs, reports, files, settings): for arg_pos, (arg_kw, arg) in enumerate(func.parameters.items()): if (not arg.is_output) and (arg.type == 'STRING'): for msgid, msgctxts in context_kw_set.items(): - if arg_kw in msgctxts: + # The msgid can be missing if it is used in only some UILayout functions but not all + if arg_kw in msgctxts and msgid in func_translate_args[func_id]: func_translate_args[func_id][msgid][1][arg_kw] = arg_pos # The report() func of operators. for func_id, func in bpy.types.Operator.bl_rna.functions.items(): ```

Bastien Montagne commented

2023-09-15 14:44:58 +02:00

Changes suggested by @pioverfour regarding handling of translations make total sense to me.

i do not have time currently to handle/check the 'extraction' part though

Changes suggested by @pioverfour regarding handling of translations make total sense to me. i do not have time currently to handle/check the 'extraction' part though

👍 1

Harley Acheson added 2 commits 2023-09-15 17:26:08 +02:00

50ab6c128a Merge branch 'main' of projects.blender.org:blender/blender into placeholders

271378eeca Changes to translation extractions, suggested by Damien Picard

Damien Picard commented

2023-09-18 00:06:55 +02:00

@Harley It looks like the second part of my latest diff was skipped, but without it there is an exception during message extraction.

Harley Acheson added 2 commits 2023-09-18 00:42:18 +02:00

2f031ec54f Merge branch 'main' of projects.blender.org:blender/blender into placeholders

1d1331cab5 Somehow missed this required change, suggested by Damien Picard.

Harley Acheson commented

2023-09-18 00:42:35 +02:00

@Harley It looks like the second part of my latest diff was skipped, but without it there is an exception during message extraction.

OMG, I completely missed that. So glad you double-checked. Thanks!!!

> @Harley It looks like the second part of my latest diff was skipped, but without it there is an exception during message extraction. OMG, I completely missed that. So glad you double-checked. Thanks!!!

❤️ 2

Hans Goudey requested review from Hans Goudey 2023-09-20 18:35:44 +02:00

Hans Goudey requested changes 2023-09-20 18:50:27 +02:00

Hans Goudey left a comment

The patch is simple and makes sense. I think exposing it as an argument in python makes sense too. I'd hope that the existing uiBut string storage could be used (drawstr) for example. That would require storing a flag like "the text is just a placeholder" for the drawing code, but that should be okay. The goal of reusing the existing storage is just to keep uiBut smaller and have more consistency about where we store draw strings.

The patch is simple and makes sense. I think exposing it as an argument in python makes sense too. I'd hope that the existing `uiBut` string storage could be used (`drawstr`) for example. That would require storing a flag like "the text is just a placeholder" for the drawing code, but that should be okay. The goal of reusing the existing storage is just to keep `uiBut` smaller and have more consistency about where we store draw strings.

Harley Acheson commented

2023-09-21 04:32:53 +02:00

The patch is simple and makes sense. I think exposing it as an argument in python makes sense too. I'd hope that the existing uiBut string storage could be used (drawstr) for example. That would require storing a flag like "the text is just a placeholder" for the drawing code, but that should be okay. The goal of reusing the existing storage is just to keep uiBut smaller and have more consistency about where we store draw strings.

I could be thinking about this completely wrong, but I had been assuming that because we want these to work with UI_BTYPE_TEXT (regular text input) that I couldn't use any of the strings needed for editable text, since they are needed. Or maybe I am misunderstanding the life cycle of these things?

> The patch is simple and makes sense. I think exposing it as an argument in python makes sense too. I'd hope that the existing `uiBut` string storage could be used (`drawstr`) for example. That would require storing a flag like "the text is just a placeholder" for the drawing code, but that should be okay. The goal of reusing the existing storage is just to keep `uiBut` smaller and have more consistency about where we store draw strings. I could be thinking about this completely wrong, but I had been assuming that because we want these to work with UI_BTYPE_TEXT (regular text input) that I couldn't use any of the strings needed for editable text, since they are needed. Or maybe I am misunderstanding the life cycle of these things?

Hans Goudey commented

2023-09-21 14:05:23 +02:00

I thought that a button would have either placeholder text or editable text, not both at the same time, since once you start editing, the placeholder isn't drawn anymore.

Harley Acheson commented

2023-09-22 00:57:37 +02:00

@HooglyBoogly - I thought that a button would have either placeholder text or editable text, not both at the same time, since once you start editing, the placeholder isn't drawn anymore.

Not sure it is worth the complexity of using one member for two different uses and then having to maintain a flag to keep track of it.

This just adds a single pointer. But uiBut still has some actual char arrays, like strdata at 128 bytes and drawstr with 400 bytes. So the 8 here seems like the least of our worries. Or perhaps I'm just being lazy.

> @HooglyBoogly - I thought that a button would have either placeholder text or editable text, not both at the same time, since once you start editing, the placeholder isn't drawn anymore. Not sure it is worth the complexity of using one member for two different uses and then having to maintain a flag to keep track of it. This just adds a single pointer. But uiBut still has some actual char arrays, like `strdata` at 128 bytes and `drawstr` with 400 bytes. So the 8 here seems like the least of our worries. Or perhaps I'm just being lazy.

Julian Eisel reviewed 2023-09-26 17:18:10 +02:00

source/blender/editors/include/UI_interface_c.hh Outdated

						
				@ -915,1 +915,4 @@

				/* Hint that describes the expected value when empty */

				void UI_but_placeholder_set(uiBut *but, const char *placeholder_text);

				const char *UI_but_placeholder_get(uiBut *but);

Julian Eisel commented

2023-09-26 17:17:48 +02:00

This shouldn't be in the public API, put it in interface_intern.hh.

This shouldn't be in the public API, put it in `interface_intern.hh`.

Harley marked this conversation as resolved

Harley Acheson added 1 commit 2023-09-26 19:48:51 +02:00

a5c5a707ef Merge branch 'main' of projects.blender.org:blender/blender into placeholders

Harley Acheson added 1 commit 2023-09-26 19:54:20 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

f016b9f16a Changes required by review

Hans Goudey approved these changes 2023-09-26 19:59:25 +02:00

Harley Acheson commented

2023-09-26 20:28:14 +02:00

@blender-bot build

Julian Eisel reviewed 2023-09-27 21:18:43 +02:00

source/blender/editors/interface/interface_intern.hh Outdated

						
				@ -741,0 +743,4 @@

				/**

				 * Hint that describes the expected value when empty.

				 */

				void UI_but_placeholder_set(uiBut *but, const char *placeholder_text);

Julian Eisel commented

2023-09-27 21:18:43 +02:00

My earlier comment was only about UI_but_placeholder_get(). The setter makes sense in the public API, it's also part of the public Python API. If that can override the placeholder, other external UI code should be able to as well.

Convention is also to prefix internal functions with ui_, not UI_.

My earlier comment was only about `UI_but_placeholder_get()`. The setter makes sense in the public API, it's also part of the public Python API. If that can override the placeholder, other external UI code should be able to as well. Convention is also to prefix internal functions with `ui_`, not `UI_`.

Harley marked this conversation as resolved

Harley Acheson added 1 commit 2023-09-27 21:36:30 +02:00

f5b06ca224 Changes require by review.

Harley Acheson commented

2023-09-27 21:38:58 +02:00

@JulianEisel - My earlier comment was only about UI_but_placeholder_get()

Yikes, thanks for noticing my mistake. I totally misread your "this" as "these".

> @JulianEisel - My earlier comment was only about UI_but_placeholder_get() Yikes, thanks for noticing my mistake. I totally misread your "this" as "these".

Jacques Lucke reviewed 2023-10-10 17:42:40 +02:00

source/blender/editors/interface/interface.cc Outdated

						
				@ -5900,0 +5905,4 @@

				void UI_but_placeholder_set(uiBut *but, const char *placeholder_text)

				{

				  /* Remove any existing placeholder. */

				  if (but && but->placeholder) {

Jacques Lucke commented

2023-10-10 17:42:40 +02:00

Do we have to check that but is non-null here? Seems like this function should only be called when it's non-null anyway.

This function can probably be simplified:

MEM_SAFE_FREE(but->placeholder);
but->placeholder = BLI_strdup_null(placeholder_text);

Do we have to check that `but` is non-null here? Seems like this function should only be called when it's non-null anyway. This function can probably be simplified: ``` MEM_SAFE_FREE(but->placeholder); but->placeholder = BLI_strdup_null(placeholder_text); ```

Julian Eisel commented

2023-10-10 17:53:49 +02:00

Indeed, usually we assume pointers like uiBut * are non-null in such functions (could also use ATTR_NONNULL()). Best would be to port all of these to use references soon, to make clear in the interface that null is not expected.

Indeed, usually we assume pointers like `uiBut *` are non-null in such functions (could also use `ATTR_NONNULL()`). Best would be to port all of these to use references soon, to make clear in the interface that null is not expected.

Harley marked this conversation as resolved

Harley Acheson changed title from ~~UI: Input Placeholders~~ to UI: Input Placeholders

2023-10-11 00:07:03 +02:00

Harley changed target branch from main to blender-v4.0-release

2023-10-11 00:07:06 +02:00

Harley Acheson added 13 commits 2023-10-11 00:15:02 +02:00

f0dcc3d544 WIP: UI: Input Placeholders

Demonstration (only) of input placeholders.

caf1db5903 Changes suggested by Pablo

f2c2b92db4 Mostly working...

581af1ac17 Cleanup and comment improvements.

66cbb734da Default placeholders

6b1889bb8c Changes requested by Damien Picard

da6ee3b54c Translation-related changes suggested by Damien Picard

85e15180bb Changes to translation extractions, suggested by Damien Picard

0690202d14 Somehow missed this required change, suggested by Damien Picard.

68b3b3b6ca Changes required by review

1300753f52 Changes require by review.

24338e0a45 Retarget and update

a2dad7c538 Merge branch 'placeholders' of projects.blender.org:Harley/blender into placeholders

Harley Acheson merged commit b0515e34f9 into blender-v4.0-release

2023-10-11 00:47:20 +02:00

Harley Acheson referenced this issue from a commit

2023-10-11 00:47:21 +02:00

UI: Input Placeholders

Harley Acheson deleted branch placeholders

2023-10-11 00:47:22 +02:00

Harley Acheson referenced this issue from a commit

2023-10-12 20:45:45 +02:00

UI: Input Placeholders

Harley Acheson referenced this pull request

2023-11-10 18:34:26 +01:00

UI: Placeholder in Text fields #111436

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

UI: Input Placeholders #112104