#73207 (Re: glib patches)

[Tim Janik <timj gtk org>]

On Sun, 28 Dec 2003, Matthias Clasen wrote:

> Hi Tim,
>
> there are a number of patches on the 2.4 API freeze milestone for GLib
> in bugzilla which are more or less blocking on feedback from you. It
> would be great if you could comment on these:

> 73207 gtk_accel_map_lock_path()
>       this is actually GTK, but since you wrote the accel map stuff,
>       maybe you want to comment on this one as well. It adds the ability
>       to lock individual paths.

this is not ok, it should not go into 2.4 in its current form.

first, i'd like to know what the issue about gconf accel paths is all
about, i've been unable to change any accelerators at runtime since
gconf started to set can-change-accels without the control-center
allowing offering functionality to change it.

second, if there is a reasonable point in locking single accelerator
paths, there are several issues with the patch:

an API gtk_accel_map_[un]lock_path() should probbal support
nesting as all other related API does (block/unblock signals,
freeze/thaw notifies).

gtk_accel_map_lock_path() probably should create a new entry and
lock it, if there's none in the map yet. otherwise, we introduce
subtle dependancies between locking, menu setup and accelerator map
parsing.
consequently, gtk_accel_map_unlock_path() shouldn't go silent if an
unexistant accel path is attempted to be unlocked.


> @@ -251,6 +254,9 @@ internal_change_entry (const gchar    *a
> 	entry->changed = TRUE;
>       return simulate ? TRUE : FALSE;
>     }
> +
> +  if (entry->locked)
> +    return FALSE;

this portion needs a comment on why you may return FALSE here
eventhough simulate maybe TRUE. (the reason being that non-changes
are caught above).


> @@ -239,7 +241,8 @@ internal_change_entry (const gchar    *a
>  	  entry = accel_path_lookup (accel_path);
>  	  entry->accel_key = accel_key;
>  	  entry->accel_mods = accel_mods;
> -	  entry->changed = TRUE;
> +	  entry->changed = 1;
> +	  entry->locked = 0;
>  	}
>        return TRUE;
>      }

there's no point in setting locked to FALSE here, in general
internal_change_entry() shouldn't change that field, and
gtk_accel_map_add_entry() already provides a default setting.

>
> Thanks,
>
> Matthias
>

PS: in this case, i'm not particularly happy about you sending me an email,
    claiming i'm "blocking" patches, and committing them on the same day...

---
ciaoTJ