add placeholder for uiEntry and uiEditableCombobox #224

matyalatte · 2023-09-16T01:47:17Z

Related to #218
This PR will add placeholders for uiEntry and uiEditableCombobox.
For Windows, you can also use uiWindows*Placeholder functions for other edit controls and combobox controls.

Added functions - uiEntryPlaceholder, uiEntrySetPlaceholder - uiEditableComboboxPlaceholder, uiEditableComboboxSetPlaceholder - uiWindowsEntryPlaceholder, uiWindowsSetEntryPlaceholder - uiWindowsComboboxPlaceholder, uiWindowsSetComboboxPlaceholder

matyalatte · 2023-09-16T02:09:11Z

Here is a custom version of the controlgallery.
controlgallery.zip

You can see the placeholders on the example app.
And you can see the placeholder's text on the cosole.

cody271 · 2023-09-16T05:22:39Z

Here is a custom version of the controlgallery.
controlgallery.zip

Please try to keep all contributions inside version control. Passing around source files like that is not recommended!

cody271 · 2023-09-16T05:24:02Z

We should add placeholders to controlgallery, but keep it simple:

diff --git a/examples/controlgallery/main.c b/examples/controlgallery/main.c
index 6415e4ff..e6cc9a48 100644
--- a/examples/controlgallery/main.c
+++ b/examples/controlgallery/main.c
@@ -54,14 +54,18 @@ static uiControl *makeBasicControlsPage(void)
 	uiFormSetPadded(entryForm, 1);
 	uiGroupSetChild(group, uiControl(entryForm));
 
+	uiEntry* entry = uiNewEntry();
+	uiEntrySetPlaceholder(entry, "Type here");
 	uiFormAppend(entryForm,
 		"Entry",
-		uiControl(uiNewEntry()),
+		uiControl(entry),
 		0);
+
 	uiFormAppend(entryForm,
 		"Password Entry",
 		uiControl(uiNewPasswordEntry()),
 		0);
+
 	uiFormAppend(entryForm,
 		"Search Entry",
 		uiControl(uiNewSearchEntry()),
@@ -147,6 +151,7 @@ static uiControl *makeNumbersPage()
 	uiEditableComboboxAppend(ecbox, "Editable Item 1");
 	uiEditableComboboxAppend(ecbox, "Editable Item 2");
 	uiEditableComboboxAppend(ecbox, "Editable Item 3");
+	uiEditableComboboxSetPlaceholder(ecbox, "Select an item");
 	uiBoxAppend(vbox, uiControl(ecbox), 0);
 
 	rb = uiNewRadioButtons();
@@ -264,6 +269,7 @@ static uiControl *makeDataChoosersPage(void)
 
 	button = uiNewButton("  Open File  ");
 	entry = uiNewEntry();
+	uiEntrySetPlaceholder(entry, "Select a file");
 	uiEntrySetReadOnly(entry, 1);
 	uiButtonOnClicked(button, onOpenFileClicked, entry);
 	uiGridAppend(grid, uiControl(button),

cody271 · 2023-09-16T05:27:30Z

These sorts of checks should be added to the test suite, instead of the examples.

matyalatte · 2023-09-16T05:28:12Z

Please try to keep all contributions inside version control.

It's just for testing setter and getter methods on the GUI. so I thought it shouldn't be added to the PR.

We should add placeholders to controlgallery, but keep it simple:

ok. I'll add another commit for it.

ui_windows.h

cody271 · 2023-09-17T04:25:29Z

ui.h

+ * @param text Placeholder text.\n
+ *             A valid, `NUL` terminated UTF-8 string.\n
+ *             Data is copied internally. Ownership is not transferred.
+ * @warning Read-only entries can't not display the placeholder on Windows


Hopefully this limitation can be fixed, I will try the Windows implementation next.

I have confirmed that it is a limitation of Win32 API that EM_SETCUEBANNER doesn't support read only entries or multi-line entries!

cody271 · 2023-09-18T13:56:45Z

Might as well add multi-line entry support now?
As the Unix/Darwin part is simple to implement. Underlying Windows implementation here is broken anyway.

Make a new issue specific to Windows (to keep track of it), then we can move on for now. It can be fixed in a later PR..

cody271

Nit: functions private, not fucntions private.

szanni

I have so far only checked uiEntry and added a few more test cases - one of which crashes hard on all platforms.

From an API perspective it is the question of what the default value of uiEntryPlaceholder should be. An empty string or NULL?
And do we want to support the setting of NULL or do we unset via an empty string?

I haven't checked but the crash on all three platforms suggests they return some type of NULL value initially. The test I added expects an empty string, as that is what uiEntryText returns by default.
I am not entirely opposed to NULL values either. The docs would need to be changed to reflect that though.

I would love to also have a qa test for the interaction of read only and placeholder text.
And I would wouldn't mind having two different PRs for uiEntry and uiCombobox as this is a lot of code - I do understand they have a fair amount of overlap though on win32.

szanni · 2023-09-19T23:01:01Z

ui.h

+ * @param text Placeholder text.\n
+ *             A valid, `NUL` terminated UTF-8 string.\n
+ *             Data is copied internally. Ownership is not transferred.
+ * @warning Read-only entries can't not display the placeholder on Windows


Suggested change

* @warning Read-only entries can't not display the placeholder on Windows

* @warning Read only entries do not display the placeholder text on Windows.

Spelling and consistent punctuation.

And I confirmed as well that EM_SETCUEBANNER does not play nice with ES_READONLY.

Read-only -> Read only

I can see some comments that use "read-only" in the repo. (e.g. windows/multilineentry.cpp)
But should I use "Read only" in the document?

can't -> do

I thought I fixed it already but I seemed to revert it somehow...

fixed at 3302ef9

szanni · 2023-09-19T23:08:20Z

test/unit/entry.c

@@ -123,6 +140,7 @@ int entryRunUnitTests(void)
 		entryUnitTests(entryTextDefault),
 		entryUnitTests(entrySetText),
 		entryUnitTests(entrySetTextNoCallback),
+		entryUnitTests(entrySetPlaceholder),
 		entryUnitTests(entryReadOnlyDefault),
 		entryUnitTests(entrySetReadOnly),


Suggested change

entryUnitTests(entrySetReadOnly),

entryUnitTests(entrySetReadOnly),

entryUnitTests(entryReadOnlySetPlaceholder),

entryUnitTests(entryPlaceholderSetReadOnly),

And add the following two tests to line 87:

// win32: Make sure the placeholder gets set in read only mode, even though not visually displayed. static void entryReadOnlySetPlaceholder(void **state) { uiEntry **e = uiEntryPtrFromState(state); const char *text = "Text"; char *rv; uiEntrySetReadOnly(*e, 1); uiEntrySetPlaceholder(*e, text); assert_int_equal(uiEntryReadOnly(*e), 1); rv = uiEntryPlaceholder(*e); assert_string_equal(rv, text); uiFreeText(rv); } // win32: Make sure the placeholder is retained and read only mode settable with a placeholder set. static void entryPlaceholderSetReadOnly(void **state) { uiEntry **e = uiEntryPtrFromState(state); const char *text = "Text"; char *rv; uiEntrySetPlaceholder(*e, text); uiEntrySetReadOnly(*e, 1); assert_int_equal(uiEntryReadOnly(*e), 1); rv = uiEntryPlaceholder(*e); assert_string_equal(rv, text); uiFreeText(rv); uiEntrySetReadOnly(*e, 0); assert_int_equal(uiEntryReadOnly(*e), 0); rv = uiEntryPlaceholder(*e); assert_string_equal(rv, text); uiFreeText(rv); }

I noticed I don't need to edit windows/entry.cpp to pass these tests.
I mean, EM_SETCUEBANNER works internally with ES_READONLY.
I made another branch to test them but it passed the CI worklfow.

szanni · 2023-09-19T23:10:12Z

test/unit/entry.c

@@ -51,6 +51,23 @@ static void entrySetTextNoCallback(void **state)
 	uiEntrySetText(*e, "Text 2");
 }

+static void entrySetPlaceholder(void **state)


Suggested change

static void entrySetPlaceholder(void **state)

static void entryPlaceholderDefault(void **state)

{

uiEntry **e = uiEntryPtrFromState(state);

const char *text = "";

char *rv;

rv = uiEntryPlaceholder(*e);

assert_string_equal(rv, text);

uiFreeText(rv);

}

static void entrySetPlaceholder(void **state)

This currently crashes on all platforms and needs fixing.

fixed at ac3ce07

szanni · 2023-09-19T23:28:57Z

windows/uipriv_windows.hpp

None of the other functions are prefixed with uipriv. Do we wan't to specify any rules around this?

Other functions just predate the policy change. Original discussion here:

andlabs/libui#308

prefix all non-public names that aren't static with uipriv

matyalatte · 2023-09-23T09:14:45Z

Might as well add multi-line entry support now?

It requires WM_* events to catch focus events and to change the text color.
So, I think we should work on the event handling features first.
like uiEntryOnFocused, uiEntrySetTextColor, etc.

Make a new issue specific to Windows (to keep track of it), then we can move on for now. It can be fixed in a later PR.

Ok, I'll make another issue like "Read only entries do not display placeholders on Windows" after this PR is merged.

matyalatte mentioned this pull request Sep 16, 2023

[Enhancement] Placeholders for Text Entries #218

Open

add examples and tests for placeholder

a1c1c53

cody271 reviewed Sep 17, 2023

View reviewed changes

cody271 reviewed Sep 18, 2023

View reviewed changes

szanni suggested changes Sep 19, 2023

View reviewed changes

cody271 linked an issue Sep 22, 2023 that may be closed by this pull request

[Enhancement] Placeholders for Text Entries #218

Open

matyalatte added 3 commits September 23, 2023 12:12

make uiWindows*Placeholder functions private

287ad60

return an empty string when the placeholder text is NULL

ac3ce07

fix typos

3302ef9

matyalatte force-pushed the placeholder branch from a20f26b to 3302ef9 Compare September 23, 2023 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add placeholder for uiEntry and uiEditableCombobox #224

add placeholder for uiEntry and uiEditableCombobox #224

matyalatte commented Sep 16, 2023 •

edited

matyalatte commented Sep 16, 2023

cody271 commented Sep 16, 2023

cody271 commented Sep 16, 2023

cody271 commented Sep 16, 2023

matyalatte commented Sep 16, 2023 •

edited

cody271 Sep 17, 2023

cody271 Sep 18, 2023

cody271 commented Sep 18, 2023

cody271 left a comment

szanni left a comment

szanni Sep 19, 2023

matyalatte Sep 23, 2023

matyalatte Sep 23, 2023

szanni Sep 19, 2023

szanni Sep 19, 2023

matyalatte Sep 23, 2023 •

edited

szanni Sep 19, 2023

szanni Sep 19, 2023

matyalatte Sep 23, 2023

szanni Sep 19, 2023

cody271 Sep 22, 2023

matyalatte commented Sep 23, 2023

	* @warning Read-only entries can't not display the placeholder on Windows
	* @warning Read only entries do not display the placeholder text on Windows.

-static void entrySetPlaceholder(void **state)
+static void entryPlaceholderDefault(void **state)
+{
+	uiEntry **e = uiEntryPtrFromState(state);
+	const char *text = "";
+	char *rv;
+	rv = uiEntryPlaceholder(*e);
+	assert_string_equal(rv, text);
+	uiFreeText(rv);
+}
+static void entrySetPlaceholder(void **state)

add placeholder for uiEntry and uiEditableCombobox #224

Are you sure you want to change the base?

add placeholder for uiEntry and uiEditableCombobox #224

Conversation

matyalatte commented Sep 16, 2023 • edited

matyalatte commented Sep 16, 2023

cody271 commented Sep 16, 2023

cody271 commented Sep 16, 2023

cody271 commented Sep 16, 2023

matyalatte commented Sep 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cody271 commented Sep 18, 2023

cody271 left a comment

Choose a reason for hiding this comment

szanni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matyalatte Sep 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matyalatte commented Sep 23, 2023

matyalatte commented Sep 16, 2023 •

edited

matyalatte commented Sep 16, 2023 •

edited

matyalatte Sep 23, 2023 •

edited