Re: [bug] attributes stuff in options_menu.c broken

On 05.11.2008, at 10:22, Urs Janßen wrote:

> In <[email protected]>, Dennis Preiser  
> wrote:
>
>> read_attributes_files() in select.c is IMO needed to apply changed
>> options to all groups. Without this, changes in the menu have no
>
> not if we apply the change "in memory" and without writing out
> (you've fixed that) the changed values it was plain wrong. so either
> we must guard all change_config_file() with a reread of the
> attributes file or update the default values as well....

The issue is that glob_attributes are only attatched to a group if no  
scope matches the group->name. If a scope matches the group, this  
group gets its own attributes struct.

If, for instance, a scope=* section is present all groups gets its  
own attributes struct and no group gets attached the glob_attributes.  
In this case, changes in the menu at selection-level have no effect  
whether glob_attributes is updated or not without this  
read_attributes_files() in select.c.

We can loop over all groups and set the group->attribute->option (if  
it is not set from the attributes file) as a preliminary solution.  
Then the read_attributes_files() in select.c can be removed. The  
macro could looks like this:

#define UPDATE_INT_ATTRIBUTES(option) do { \
	if (group != NULL) \
		group->attribute->option = tinrc.option; \
	else { \
		int i; \
		for_each_group(i) { \
			group = &active[i]; \
			if (group->attribute->option == glob_attributes.option) \
				group->attribute->option = tinrc.option; \
		} \
	} \
	if ((num_local_attributes > 0) && (local_attributes != NULL)) { \
		struct t_attribute *attr; \
		int i = 0; \
		for (; i < num_local_attributes; i++) { \
			attr = &local_attributes[i]; \
			if (attr->option == glob_attributes.option) \
				attr->option = tinrc.option; \
		} \
	} \
	glob_attributes.option = tinrc.option; \
	} while (0)

In principle, we do most off the stuff read_attributes_files() does  
then.

>> Attached is a patch which writes changed numeric options into the
>> scope-specific attributes. This should tin stop writing this garbage
>
> you've missed some - and some of the missed options are 'problematic'

That's why I didn't touched these options.

> as thier attribut name differs from the tinrc-name (we should unify
> them, see als doc/config-anomalies):

[options]

This will be included in a later diff together with the other  
attributes stuff I'm working on.

> and we need to update the glob_attributes as well (could be done via
> read_attributes_files(), but that's missing after most of the
> change_config_file() calls).

The macro handles glob_attributes already. Should this be done in an  
other way?

> and IMHO this check is "wrong" (but omitting it may realy confuse  
> ppl. so
> it's ok for the moment (till we have an attributes menu)):
>
> 	if (attr->option == glob_attributes.option) \
> 		attr->option = tinrc.option; \
>
> wrong cause we do a _global_ change, thus any value must be set to  
> the new
> value (but that would destroy fine grained attributes ...)

That's the problem: make the attributes-stuff works as good as  
possible until the attributes menu.

Dennis