GNOME Bugzilla – Bug 694154
Use GtkEntry instead of GtkSearchEntry for new printers
Last modified: 2014-03-03 16:18:07 UTC
Created attachment 236736 [details] screenshot of using of GtkEntry instead of GtkSearchEntry for new printers The search entry in new printer dialog uses GtkSearchEntry currently. It should use GtkEntry because it doesn't search as you type. The entry waits for press of "Enter" or of secondary icon and starts to search after that. Searching for available printers is long running process and can not be executed after each key-press. Fix: replace GtkSearchEntry by GtkEntry and use magnifier icon for the right icon. I'll ask for for UI freeze break if accepted.
Created attachment 236737 [details] [review] Make search entry usage clearer for new printers
Review of attachment 236737 [details] [review]: That doesn't make usage any clearer. Add a button to the right of the entry and carry on using a GtkSearchEntry.
Created attachment 267307 [details] [review] Don't wait for activation to start search This patch changes behaviour of the search entry so that it starts to search when "search-changed" signal is received. It cancels previous searches if needed.
Created attachment 267308 [details] [review] Don't show tooltip on clear icon when searching for printers
Created attachment 267309 [details] [review] Don't react to icon-press when searching for new printers
Created attachment 267310 [details] [review] Delay searching for remote printers Start search for remote printers after 500ms of users inactivity instead of GtkSearchEntry's default 150ms.
Review of attachment 267307 [details] [review]: That looks fine.
Review of attachment 267308 [details] [review]: Fine by me.
Review of attachment 267309 [details] [review]: Sure.
Review of attachment 267310 [details] [review]: ::: panels/printers/pp-new-printer-dialog.c @@ +53,3 @@ static void actualize_devices_list (PpNewPrinterDialog *dialog); static void populate_devices_list (PpNewPrinterDialog *dialog); +static void search_address_cb1 (GtkEntry *entry, Please give those more descriptive names. @@ +135,3 @@ PpHost *remote_cups_host; PpSamba *samba_host; + guint host_search_id; host_search_timeout_id? idle_id? something else? @@ +1478,3 @@ + gchar *host_name; + gint host_port; +} THost; A more descriptive name as well please. @@ +1656,3 @@ + (GDestroyNotify) search_for_remote_printers_free); + else + g_idle_add_full (G_PRIORITY_DEFAULT, Why do you leave a timeout dangling like that? Can't you call the function directly?
Created attachment 269016 [details] [review] Delay searching for remote printers Hi, thank you for the reviews. (In reply to comment #10) > Review of attachment 267310 [details] [review]: > > ::: panels/printers/pp-new-printer-dialog.c > @@ +53,3 @@ > static void actualize_devices_list (PpNewPrinterDialog *dialog); > static void populate_devices_list (PpNewPrinterDialog *dialog); > +static void search_address_cb1 (GtkEntry *entry, > > Please give those more descriptive names. Done - search_entry_activated_cb(), search_entry_changed_cb(). > @@ +135,3 @@ > PpHost *remote_cups_host; > PpSamba *samba_host; > + guint host_search_id; > > host_search_timeout_id? idle_id? something else? Done - host_search_timeout_id. > @@ +1478,3 @@ > + gchar *host_name; > + gint host_port; > +} THost; > > A more descriptive name as well please. Done - THostSearchData. > @@ +1656,3 @@ > + (GDestroyNotify) > search_for_remote_printers_free); > + else > + g_idle_add_full (G_PRIORITY_DEFAULT, > > Why do you leave a timeout dangling like that? Can't you call the function > directly? Fixed. You are right, I can call the function and the data free function directly. Regards Marek
Review of attachment 269016 [details] [review]: Looks good overall. ::: panels/printers/pp-new-printer-dialog.c @@ +48,3 @@ #endif +#define HOST_SEARCH_DELAY 350 That's 500 minus the default 150 msecs from the search entry. Would be good to write it as such: #define HOST_SEARCH_DELAY (500 - 150) and clarify in a comment
Comment on attachment 269016 [details] [review] Delay searching for remote printers Thank you again. I've pushed all those patches to master. (In reply to comment #12) > Review of attachment 269016 [details] [review]: > > Looks good overall. > > ::: panels/printers/pp-new-printer-dialog.c > @@ +48,3 @@ > #endif > > +#define HOST_SEARCH_DELAY 350 > > That's 500 minus the default 150 msecs from the search entry. > Would be good to write it as such: > #define HOST_SEARCH_DELAY (500 - 150) > and clarify in a comment I've added the comment there and changed the value to (500 - 150). Regards Marek
(In reply to comment #12) > Review of attachment 269016 [details] [review]: > > Looks good overall. > > ::: panels/printers/pp-new-printer-dialog.c > @@ +48,3 @@ > #endif > > +#define HOST_SEARCH_DELAY 350 > > That's 500 minus the default 150 msecs from the search entry. > Would be good to write it as such: > #define HOST_SEARCH_DELAY (500 - 150) > and clarify in a comment
*** Bug 725593 has been marked as a duplicate of this bug. ***