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 uiEntryOnFinished() event to uiEntry #290

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bcampbell
Copy link
Contributor

This change adds an event to uiEntry, which fires when the editing is 'complete', as opposed to OnChanged(), which fires more-or-less for every keypress.
I settled on the term 'Finished' rather than 'Enter' or 'Submit' or similar.
'Submit' sounded too form-like, and 'Enter' seems to imply the 'enter' key, but there are other conditions where the event might fire (eg tabbing, using search-box icons etc).

There's no specific attempt here to address the dialog-submission issues, but it's something of a moot point right now as libui doesn't support building dialog windows at the moment (right?).
Pretty sure nothing here would interfere with OS-specific dialog behaviour in any major way anyway.

Addresses #1 (and related: andlabs/ui#205)

@@ -73,6 +92,9 @@ static uiEntry *finishNewEntry(GtkWidget *w, const gchar *signal)
e->onChangedSignal = g_signal_connect(e->widget, signal, G_CALLBACK(onChanged), e);
uiEntryOnChanged(e, defaultOnChanged, NULL);

g_signal_connect(e->widget, "activate", G_CALLBACK(onFinished), e);
Copy link
Owner

@andlabs andlabs Jan 15, 2018

Choose a reason for hiding this comment

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

Would editing-done (in GtkCellEditable) be a better signal here? The docs seem to imply they are connected, but... (TODO to self: check the source code)

Note to self: TODO investigate the phrasing "it is also commonly used by applications to intercept activation of entries" in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd discounted editing-done because GtkCellEditable is designed for entry widgets embedded in tables rather than stand-alone. But the docs do rather imply that stock GtkEntry always supports the GtkCellEditable interface.
So I had another go at it, but no dice. Just couldn't see any editing-done signals triggering. I might have been doing it all wrong, but a internet search seems strangely silent on the whole subject.
Like you said, time to dig through the Gtk source code.

I see the "it is also commonly used by applications to intercept activation of entries" line as acknowledging that the activate signal is used for this purpose by a lot of apps, even if it wasn't originally intended for this use. And, given that, I reckon activate is a sound enough method - it works and is unlikely to be removed any time soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh - I've just been poking through the Gtk+ source code.
Calling gtk_cell_editable_start_editing () on GtkEntry will cause it to wire up it's activate signal to a callback that emits both the editing-done and remove-widget GtkCellEditable signals.
(see the GtkCellEditable implementation for GtkEntry: https://git.gnome.org/browse/gtk+/tree/gtk/gtkentry.c#n4587 )

So I think we're better off sticking with activate.

*lResult = 0;
return TRUE;
}
if (code == EN_KILLFOCUS) {
Copy link
Owner

@andlabs andlabs Jan 15, 2018

Choose a reason for hiding this comment

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

Windows was the thing I was the least sure about what constitutes "finished editing", because the traditional wisdom was (as far as I can gather) to validate everything when dismissing a dialog box. My original idea for validating correctness of user entries was to provide a function uiEntryAlertInvalid() that would display a notification that didn't interrupt entry (on Windows, it'd use EM_SHOWBALLOONTIP) and you'd do this on the Changed event, but I'm not sure what people would prefer doing :/ (Pre-libui package ui actually did have this, but I've now forgotten what I called the method back then.)

That being said, I'd suggest keeping these in mind:

https://blogs.msdn.microsoft.com/oldnewthing/20040419-00/?p=39753
https://blogs.msdn.microsoft.com/oldnewthing/20050808-16/?p=34673 (though this seems to imply that EN_KILLFOCUS doesn't have the same pitfalls that WM_KILLFOCUS does, thus invalidating this whole comment? TODO...)
https://blogs.msdn.microsoft.com/oldnewthing/20090420-00/?p=18523

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't dialog validation a slightly moot point anyway, without dialog box support?
I'd guess that a hypothetical dialog box would have some OnDismiss() callback that's invoked when OK or cancel is pressed. And that's where dialog-validation-as-a-whole code would go. I don't think that handling EN_KILLFOCUS here would preclude hooking into whatever windows (and gtk and osx).

Caveat: I have no experience or knowledge whatsoever about dialog boxes on any of the target platforms :-)

The use case I'm interested in myself is less to do with validation, and more to do with reacting to a "completed" change, rather than the higher-frequency OnChanged() callbacks. In particular, I have a search box where the searches take a few seconds to perform, so I only want to perform them when the user has completed entering their search query.

Copy link
Contributor Author

@bcampbell bcampbell Jan 16, 2018

Choose a reason for hiding this comment

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

And yes, my reading of those Raymond Chen posts also seems to suggest that WM_KILLFOCUS is the tricky one, rather than EN_FOCUS.

if (isSearchField(e->textfield)) {
NSSearchField *s;
s = (NSSearchField *) (e->textfield);
// TODO requires OSX >= 10.10 (is that an issue?)
Copy link
Owner

Choose a reason for hiding this comment

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

Calling it directly is, but this kind of thing is what future.m is for.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually the equivalent property on NSSearchFieldCell is 10.3 and newer, so just replace s in the call with [s cell] (or preferably, given the coding style I've written myself into with this and that I should probably clan up someday, a cast version of [s cell]).

// There's also an option to change the behaviour to trigger only when the
// enter key is hit, or the search icon is pressed. This is the
// sendsWholeSearchString flag, and we'll set it if (and only if) OnFinished()
// is used.
Copy link
Owner

Choose a reason for hiding this comment

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

Good comment here! As for the text itself, yes, that is an issue, and I'm not sure what workarounds there are. The control editing system in OS X is something I don't fully understand yet, especially since NSSearchField can also use target-action. I'll have to look into it more...

@andlabs
Copy link
Owner

andlabs commented Jan 15, 2018

(FWIW I haven't been working on this for the past two months due to copyright clearance with my new employer; once I get that I'll start merging everything.)

@bcampbell
Copy link
Contributor Author

Thanks for the great feedback- much appreciated. I'll pick my way through it over the next week or so and try knocking off the rough edges!

@andlabs
Copy link
Owner

andlabs commented Apr 20, 2018

You will need to change the merge target on this PR.

@bcampbell bcampbell changed the base branch from utflib-and-attrstr to master April 20, 2018 20:06
@andlabs andlabs added this to the Post-Alpha 5 milestone Dec 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants