Hi Peter,
On Mittwoch, 8. Oktober 2008, Peter Clifton wrote:
> On Wed, 2008-10-08 at 15:02 +0200, Werner Hoch wrote:
> > As Peter B. wrote, maybe the o_complex_get_promotable function
> > should just be a filter for an generic function that returns a list
> > of toplevel attribute objects.
> >
> > Thus the change of the component dialog wouldn't be required.
>
> I lost track already.. I thought we weren't looking specifically for
> promotable attributes. Clearly, the component selector uses a
> different filtering, so there is no need to re-use
> o_complex_get_promotable() for the component selector, and no
> immediate need to touch that code.
We need a function that collects all attributes and a second that
filters the first list to fit the current requirements.
In the current two functions o_complex_get_promotable and
o_complex_is_eligible_attribute things are mixed together a little bit.
> I'm still not sure why you're putting filters on the display list of
> attributes though... surely the whole point is to show what
> attributes a symbols has, not just "some" of them.
>
> The only filtering I can imagine being useful is a "negative" filter
> action, so, for example, - if I picked out an attribute and had a
> different way of previewing it (footprint=, or some documentation=
> hyperlink for example), I wouldn't repeat it in the list).
>
> We can't possibly list every attribute every user might want to
> display in the list, so I think the default ought to be an unfiltered
> view.
see other mail.
> Regarding code:
>
> + for (i=0; i < length; i++) {
> + SCM_ASSERT(scm_is_string(scm_list_ref(stringlist,
> scm_from_int(i))), + scm_list_ref(stringlist,
> scm_from_int(i)), SCM_ARG1, + "list element is not a
> string");
> + attr = g_strdup(SCM_STRING_CHARS(scm_list_ref(stringlist,
> scm_from_int(i)))); + list = g_list_append(list, attr);
> + }
>
> The typical idiom (faster for long lists) is to use g_list_prepend()
> then g_list_reverse() at the end. For the number of items here, it
> probably doesn't really matter.
Done.
>
> + /* If the command is called multiple times, remove the old list
> and replace it + with the new one */
> + for (iter = default_component_select_attrlist;
> + iter != NULL;
> + iter = g_list_next(iter))
> + g_free(iter->data);
> + g_list_free(default_component_select_attrlist);
>
> You could do this easier:
>
> g_list_foreach (default_component_select_attrlist, g_free, NULL);
> g_list_free (default_component_select_attrlist);
>
> *Relying on the fact that I can call a function with a shorter
> argument signature, and it still works. The foreach callback function
> actually has a userdata argument, but it doesn't matter if we specify
> g_free. You might need to cast it to (GFunc) if the compiler warns
> though.
Even if I remember that I've used g_list_foreach that way I couldn't
find the docs.
>
> + if (o_attrib_get_name_value(object->text->string, &name,
> &value)) { + if (g_str_has_prefix(name, listiter->data)
> + && strlen(name) == strlen(listiter->data)) {
>
> strcmp(name, listiter->data) == 0 ?
done
> + }
> + else {
>
> Indentation bug / typo.
you're pedantic today ;-), fixed
I remember that I've used this style while studiing.
BTW: Have you installed any pre commit hooks in git to detect such
things?
> Are you using stgit? It would be about perfect for rolling any
> desired changes into these patches.
I've installed it but I'm still using git [reset rebase cherry-pick].
Regards
Werner
_______________________________________________
geda-dev mailing list
[email protected]
http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev
