Created attachment 244442 [details] [review]
Add new GtkDelayedSearchEntry widget

This search entry will emit the "changed" signal after 200 msecs,
so that searching through big lists, or doing online searches
feels more responsive.

Did you forget to git add the new files?

Created attachment 244464 [details] [review]
Add new GtkDelayedSearchEntry widget

This search entry will emit the "changed" signal after 200 msecs,
so that searching through big lists, or doing online searches
feels more responsive.

Hmm, not really fond of doing yet another subclass for this - can't this be a simple property of the search entry ?

There's no private struct member for GtkSearchEntry (because it wasn't needed at the time, and I forgot about it), so we can't (cleanly) extend GtkSearchEntry without breaking the ABI.

I'm also not 100% clear whether we want the "normal" GtkSearchEntry to behave that way. gnome-documents uses a timeout for local search too, not just for online ones, and gnome-control-center's input search dialogues do too, even though the data about the input sources is local and cached, but the widget movement is too quick and actually makes the search feel slower.

If there were a private data, I would add a timeout value with a default of 200 msecs, as we've done in this subclass, and apps that need instantaneousness could change that value to 0.

g_type_instance_get_private() is now cheaper, so you can use that to access the private data structure without a "priv" pointer.

you can also store the offset of the private data as a static global variable and use G_STRUCT_MEMBER() to retrieve the private data without any additional type check (and roughly with the same cost as a pointer deref).

Created attachment 244524 [details] [review]
GtkSearchEntry: Delay the changed signal by default

Emit the "changed" signal after 200 msecs, so that searching
through big lists, or doing online searches feels more responsive.

Created attachment 244528 [details] [review]
GtkSearchEntry: Delay the changed signal by default

Emit the "changed" signal after 150 msecs, so that searching
through big lists, or doing online searches feels more responsive.

This is something already done in various applications to make
search-as-you type more responsive (gnome-shell, gnome-documents,
gnome-control-center, etc.). The 150 msecs is the value currently
used by gnome-shell, so keep it (invisibly) consistent.

Review of attachment 244524 [details] [review]:

::: demos/gtk-demo/search_entry2.c
@@ +21,3 @@
+		   GtkLabel       *result_label)
+{
+	const char *text;

coding style: indentation.

::: gtk/gtksearchentry.c
@@ -54,0 +54,20 @@
+typedef struct {
+  guint delayed_changed_id;
+  gboolean in_timeout;
... 17 more ...

need to chain up to the parent class finalize.

@@ +76,3 @@
 gtk_search_entry_class_init (GtkSearchEntryClass *klass)
 {
+  GObjectClass *object_class = (GObjectClass *) klass;

use the proper cast macro, please.

Created attachment 244529 [details] [review]
GtkSearchEntry: Delay the changed signal by default

Emit the "changed" signal after 150 msecs, so that searching
through big lists, or doing online searches feels more responsive.

This is something already done in various applications to make
search-as-you type more responsive (gnome-shell, gnome-documents,
gnome-control-center, etc.). The 150 msecs is the value currently
used by gnome-shell, so keep it (invisibly) consistent.

Attachment 244529 [details] pushed as 51e2386 - GtkSearchEntry: Delay the changed signal by default

(In reply to comment #12)
> Attachment 244529 [details] pushed as 51e2386 - GtkSearchEntry: Delay the changed signal
> by default

the delayed "change" signal breaks the gtk-entry-completion support in general, since it relies on the default behaviour of this signal. the attached completion test case demonstrates the issue. it shows the completion for numbers from 00 to 99. 

with this test case you can quickly observe what is going wrong: just type in the entry, say 0, it should bring the completion popup filled with entries, select there 05 for example and hit enter. 05 will appear in the entry and completion should disappear. it does, but instead it appears again with just one item entry, which you have selected in the previous step. then click on the "clear" icon, this deletes the text from the entry (for us "05"), but again does not hide the completion entry list, due to this change https://git.gnome.org/browse/gtk+/commit/gtk/gtksearchentry.c?id=536fc22de4f4dfe9f1962e8f825c097ba81282c1

about my patch proposal: I suggest to restore the default changed signal behaviour, because who knows what else it breaks and instead I would introduce a new signal name "slow-changed", so this new signal is coupled to the original "changed" signal, and contains this delayed behaviour. as a result it fixes the libgweather-loc-entry
https://bugzilla.gnome.org/show_bug.cgi?id=702618
, completion entry support. moreover it does not break other apps which were using this delayed signal, it just makes all this apps again slow responsible until they all will be ported to the new signal "slow-changed".

Created attachment 247969 [details] [review]
restore "changed" signal. add "slow-changed" signal with coupled delayed behaviour.

Created attachment 247970 [details]
test case for completion support

Created attachment 247971 [details]
test case for "slow-chaged" signal support

This is a problem indeed. In the future though, make sure that you reopen bugs that you comment in, as otherwise it won't show up in reports.

As for the bug itself, there's a number of things we could do:
- Use private API between the search entry and the completion popup to stop it from misbehaving when using a timeout to emit "changed"
- Add a "delay-change" property, which would make this behaviour opt-in (or opt-out). Changes to the completion would be needed still.
- Add real support for completion with "delayed" changes. Showing a spinner in the entry for example.

Simply reverting and adding a "slow-changed" isn't a good solution IMO.

(In reply to comment #17)
> This is a problem indeed. In the future though, make sure that you reopen bugs
> that you comment in, as otherwise it won't show up in reports.
> 
I have a git access, but I have no permissions to in bugzilla, so I was not able to reopen it.
> As for the bug itself, there's a number of things we could do:
> - Use private API between the search entry and the completion popup to stop it
> from misbehaving when using a timeout to emit "changed"
This is possible, however this general questions should be addressed: Is there a guarantee that other thing are not got broken, by loosing original "changed" signal? Can a delayed "changed" signal replays the original "changed" signal in all possible scenarios? otherwise it will be impossible to create new enhanced widget by building on gtksearchentry widget functionality.
> - Add a "delay-change" property, which would make this behaviour opt-in (or
> opt-out). Changes to the completion would be needed still.
This could be done as well for the additional "slow-changed" signal I have proposed. This is an enhancement and thus is not really relevant to the current issue we have now.
> - Add real support for completion with "delayed" changes. Showing a spinner in
> the entry for example.
> 
Same here, questions raised above should be addressed, as well.
> Simply reverting and adding a "slow-changed" isn't a good solution IMO.
Probably, but the more I think about it, the more I like it. 

If the delayed change signal cannot replace original changed in all possible scenarios, I believe, that to have two signals is a way to go actually.

(In reply to comment #18)
<snip>
> If the delayed change signal cannot replace original changed in all possible
> scenarios, I believe, that to have two signals is a way to go actually.

I don't agree. GtkSearchEntry isn't GtkEntry. If you want to have the raw "changed" signal, use a GtkEntry. GtkSearchEntry is designed for the use case of searches. That it broke completion is a problem, but making it harder to replace existing search entries with something more suitable for searching isn't good enough.

I came across this delayed changed signal problem too. A simple solution: use the notify::text signal.

A use case where a GtkSearchEntry + instantaneous changed signal is desired: search some text in a GtkTextView. The highlighting of the search matches should be done as fast as possible for the visible region of the text view on the screen.

If you don't want to add a property to adjust the delay, the delay should at least be documented, with maybe a note to use the notify::text trick.

adding GtkSearchEntry:delay-change boolean property (set to TRUE by default) is a perfectly valid solution. a GtkEntry::slow-changed is not: apart from the unintelligible name, its semantics are impressively fuzzy; what's "slow"? the notification or the widget? the signal emission? from an app developer perspective, it's entirely not trivial to understand how this thing works.

A property would not solve much though: if it is set to 0 you get completion but you still need to manually implement throttling of the online queries etc, if you leave it to 150 then you have broken completion.

Also changing the behavior of a signal of the parent class is pretty surprising from a programmer point of view: if I swap MyEntry with GtkSearchEntry or vice versa I would normally not expect this kind of change.


I think we all agree (Evgeny included) that "slow-changed" is a bad name, but we can come up with more sensible names: "start-search"? "needle-changed"? 


Incidentally by looking at the implementation I find fairly strange that the code is using g_signal_connect (entry, "changed", ...) instead of overriding the signal. This usually makes it harder to further subclass the widget

(In reply to comment #21)
> adding GtkSearchEntry:delay-change boolean property (set to TRUE by default) is
> a perfectly valid solution.
The fact is, right now the completion support is broken, the libgweatherlocationentry is broken, so adding a new property will not change much. One should rather call this boolean property  GtkSearchEntry:break-completion-support )))
 a GtkEntry::slow-changed is not: apart from the
> unintelligible name, its semantics are impressively fuzzy; what's "slow"? the
> notification or the widget? the signal emission? from an app developer
> perspective, it's entirely not trivial to understand how this thing works.
It does not make sense right now to discuss the name of the new signal,  because it's not yet clear if a new signal is required or not.


At the present moment there are only two suggestions (Or have I missed something?):
  
1: leave GtkSearchEntry:changed signal commit as it is, despite the fact 
   that it has actually the name, but another behaviour compared with the 
   GtkEntry:changed signal.
    
    THEN: We need to 

      a: rewrite GtkEntryCompletion support, and possible other broken due to
         this signal functionality.

      b: inform clearly in documentation, like with text font size 25, 
         what developers should expect when dealing with GtkSearchEntry!!! 
         Example (based on libgweather experience):

         Imagine the following scenario, you have as a developer an 
         AmazingWidget which inherits GtkEntry. Then you come across a 
         new idea: "Ohh, GtkSearchEntry actually suits better for my widget", 
         it has a nice search icon, and moreover a nice delete icon with the 
         ability for the user to remove text he has entered immediately. 
         Awesome, GtkSearchEntry is inherited from GtkEntry, so must be very
         easy to port. So, then you change AmazingWidget : GtkEntry to
         AmazingWidget : GtkSearchEntry and done, then you test, 
         first impression is amazing again)) 
         But surprise, surprise, you might end with a broken widget(((


2: add a new signal GtkSearchEntry:<new-name> specially and perfectly suited 
   for the delayed changed behaviour.

    THEN: We need to

      a: revert GtkSearchEntry:changed delayed signal commit. (This restores
         the completion support, and other widgets)

      b: think about the name of this new signal and its enhancement. 
         (Proposed name is very bad, I agree)

Created attachment 248440 [details] [review]
GtkEntryCompletion: Fix completion with GtkSearchEntry

Listen to the notify::text property instead of the "changed" signal
so that completion is not slowed down by a delayed "changed" signal.

Created attachment 248441 [details] [review]
GtkSearchEntry: Mention the delay in "changed" in docs

And tell users of the widget to use "notify::text" if they need
to be told about changes straight away.

(In reply to comment #25)
> Created an attachment (id=248441) [details] [review]
> GtkSearchEntry: Mention the delay in "changed" in docs
> 
> And tell users of the widget to use "notify::text" if they need
> to be told about changes straight away.

If somebody knows how to link to the "text" property of GtkEntry and "changed" signal, I'd like to know so I can fix this properly.

(In reply to comment #26)
> (In reply to comment #25)
> > Created an attachment (id=248441) [details] [review] [details] [review]
> > GtkSearchEntry: Mention the delay in "changed" in docs
> > 
> > And tell users of the widget to use "notify::text" if they need
> > to be told about changes straight away.
> 
> If somebody knows how to link to the "text" property of GtkEntry and "changed"
> signal, I'd like to know so I can fix this properly.

"the GtkEntry::changed signal" and "the GtkEntry:text property"

> "the GtkEntry::changed signal" and "the GtkEntry:text property"

I think it won't work without the #:

#GtkEntry::changed and #GtkEntry:text

https://developer.gnome.org/gtk-doc-manual/unstable/documenting_syntax.html.en

I really do not get it... 

the entry has an existing signal whose behavior is part of the API/ABI, such signal is not a good fit for online searches, we add a new one with the desired behavior.

Instead we hijack the signal in an hackish way (connect instead of override), break existing users in the wild, patch the in tree users, document that people have to use hack to break their existing code

To be honest I also do not understand this)

(In reply to comment #24)
> Created an attachment (id=248440) [details] [review]
> GtkEntryCompletion: Fix completion with GtkSearchEntry
> 
> Listen to the notify::text property instead of the "changed" signal
> so that completion is not slowed down by a delayed "changed" signal.

Do this really fixes completion?

Nice to see it finally resolved :-) Thank you Matthias.