GNOME Bugzilla – Bug 679284
Don't change hostname on every character entered
Last modified: 2012-11-06 11:56:08 UTC
We are currently setting/changing the hostname everytime the user enters a character in the device name field. ** Message: Cached variant is 0xe55d20 ** Message: Got cached property: 'Jon's Laptop' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's ' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's C' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's Co' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's Com' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's Comp' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's Compu' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's Comput' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's Compute' ** Message: Cached variant is (nil) ** Message: Got manual property: 'Jon's Computer' This is what happens to a client listening to changes on the hostname interface (nautilus in this case).
I don't see how that's wrong with doing that. There's no other entry in the dialogue that somebody would likely go to when done with this one, so we can't apply it on focus out. I also don't see think that get notified of 9 or 10 characters should be a major performance drain.
Changing the hostname has implications. Performance isn't the biggest concern here. I don't think it makes any sense at all to change the hostname every character. Simply using a small timeout would be much preferable.
Created attachment 227821 [details] [review] info-panel: Set the hostname only after a small delay Avoid setting the hostname from the entry on every single key press and instead wait for a second after the last key press.
Review of attachment 227821 [details] [review]: Use a separate function to set/unset the timeout. and use a define for the 1 second constant
Created attachment 227883 [details] [review] info-panel: Set the hostname only after a small delay Avoid setting the hostname from the entry on every single key press and instead wait for a second after the last key press.
Review of attachment 227883 [details] [review]: ::: panels/info/cc-info-panel.c @@ +56,3 @@ #define KEY_SESSION_NAME "session-name" +#define SET_HOSTNAME_TIMEOUT (1) Not (1), 1. @@ +114,3 @@ GSettings *session_settings; + guint set_hostname_timeout_source; ..._source_id and line it up please. @@ +122,3 @@ static void refresh_update_button (CcInfoPanel *self); +static gboolean set_hostname_timeout (gpointer user_data); +static void remove_hostname_timeout (CcInfoPanel *panel); Can you move the functions above so they don't need declarations? @@ +513,3 @@ + { + remove_hostname_timeout (CC_INFO_PANEL (object)); + No need for a linefeed here. @@ +1767,3 @@ +static gboolean +set_hostname_timeout (gpointer user_data) Use CcInfoPanel *self directly in the function declaration. @@ +1792,3 @@ +reset_hostname_timeout (CcInfoPanel *panel) +{ + CcInfoPanelPrivate *priv = panel->priv; Remove the priv variable (a single use)
(In reply to comment #6) > Review of attachment 227883 [details] [review]: > > ::: panels/info/cc-info-panel.c > @@ +56,3 @@ > #define KEY_SESSION_NAME "session-name" > > +#define SET_HOSTNAME_TIMEOUT (1) > > Not (1), 1. > > @@ +114,3 @@ > GSettings *session_settings; > > + guint set_hostname_timeout_source; > > ..._source_id > and line it up please. Line up with what? The other variables are grouped and lined up by logical usage; the timeout source variables stands on it's own.
(In reply to comment #6) > Review of attachment 227883 [details] [review]: > @@ +122,3 @@ > static void refresh_update_button (CcInfoPanel *self); > +static gboolean set_hostname_timeout (gpointer user_data); > +static void remove_hostname_timeout (CcInfoPanel *panel); > > Can you move the functions above so they don't need declarations? > This would mean moving info_panel_set_hostname as well, or forward declaring it.
Created attachment 228095 [details] [review] info-panel: Set the hostname only after a small delay Updated patch based on the discussion on IRC.
Review of attachment 228095 [details] [review]: Looks good for master.
Attachment 228095 [details] pushed as 53d06c7 - info-panel: Set the hostname only after a small delay