I don't think this is related to 125275; looks to me as though the
checkboxes really do have the same labels.  That's a HIG violation
IMO... but the maintainers may not wish to accept a fix that changes
the labels to read "group read" "group write" etc.

I see several possibly fixes:

* change accessible-names of the checkboxes to 'group read', etc.

or

* add the labelled-by relation pointing to the 'Group', 'User', etc.
labels in the dialog  (currently the checkboxes have no AtkRelation)

transferring to nautilus

Relations were created in the dialog to fix bug #127810.

Does this problem this occur with the patch for bug #127810 applied?
That is, using a bulid from after 26th November.

Padraig this bug still applies for GOK, as of a build from 2nd Dec.

moving back to gok; looks as though GOK is not checking LABELLED_BY
relations for objects that have names.  

probably the 'right' approach is to append the label to the object's
name in the GokKeyLabel - but that will actually be bad in some cases
as a heuristic.  That's because making the name longer will be very
bad for GOK's layout, and in some cases a large number of objects may
be labelled by a single label - IOW in some cases the label will be
helpful but not necessary for disambiguation.

So one approach would be to check for duplicate names in the UIGrab
keyboard, and disambiguate via label-relations if available.

FWIW I like the sound of that approach.  

In the case of no labelled-by relation, perhaps we would check and
compare descriptions.

Failing that we could try to provide coordinate based information?
(e.g. "on/off (135, 80)" or give up...  or attend to bug 109186...

David:

we already check for labels and descriptions if a name is not
available, see gok-spy.c.  What we _don't_ do yet is either:

* append labels to objects that have non-empty names; or
* use labels to disambiguate objects with the same names.

I suggest that we do the latter, but not the former (for reasons of
concision).

Bill, I know about the existing checking for descriptions but I don't
think we currently do that if we get duplicate names...

To be clear: I agree with the you.  I was just imagining what we could
do if the labelled by search, failed or gave duplicate labels.

Created attachment 22073 [details] [review]
proposed patch - dunno if it works, i don't have a recent nautilus...

hi David;
thanks for the patch.  I'd suggest a couple of things:

(1) i see on examination that it may clash with the performance work
I'm doing, so merge cleanup will probably be required.  

(2) I don't quite understand the inner loop in
resolve_duplicate_names; it seems to me that 
if (gok_spy_is_duplicate_name (name, node))
is already having to loop, so we have, at the least, two iterations of
the loop here. The inner loop seems like a real performance killer and
not necessary - couldn't we do this with only one loop?
Maybe I'm missing the intent here.

(3) I would suggest that if the current node's name matches another
node, we want to return the matching/duplicate node instead of or
along with the boolean. Then we can check the current node _and_ the
matching node for LABELLED_BY, in case only one has a labelled_by
relation.  If the current node had no labelled-by relation, then
there's a possibility that there's a second/third duplicate, so for
that case (if current node didn't have labelled-by) we need to repeat
the _is_duplicate check, so there'll be a while loop there.


(2) it'd be nice to break the inner part of resolve_duplcate_names
out, i.e. everything after the getRelationSet call:

IOW

name = node->pname;
while ((node = node->next) != NULL) 
{
  search_node = node;
  while (search_node &&
         dup_node = gok_spy_node_for_name (search_node, name))
  { 
    AccessibleRelation *label_relations = 
                        gok_spy_get_labels (dup_node);
    /* return only LABELLED_BY targets */
    if (label_relations)
    {
       gok_spy_prepend_label (dup_node, label_relations);
       /* then free the relations */
    }
    label_relations = gok_spy_get_labels (node);
    if (label_relations) {
       gok_spy_prepend_label (node, label_relations);
       /* free the relations */
    }
    else {
       /* we presumably fixed the first dup, look for more */
    }
    start_search = dup_node->next
  } 
}

*** This bug has been marked as a duplicate of 128420 ***

I didn't mean to mark this as a dup, sorry

Hi Bill,

It's quite possible my algorithm can be improved ;-)  Do you know if
my patch works?

Would your suggested gok_spy_node_for_name not have a loop?  Ditto
with gok_spy_prepend_label.  Also, I'm thinking maybe this is a mistake:

    start_search = dup_node->next

Did you mean?:

    search_node = search_node->next

I haven't had my coffee yet, but I'm not seeing the performance
improvement...  I think there might be one though -- I need some hand
holding perhaps.

I think my algorithm averages O(n^2) which doesn't really worry me...
maybe it should?   I think have an idea for O(n) if necessary...

your patch loops twice in series, i.e. it checks for dups (loop), THEN
loops again to resolve each dup.

the typo isn't quite what you thought - the 'start_search' line needs
to go inside the 'else' clause - the intent is

" if we didn't find a label for the current ambiguous node, then we must
  continue disambiguating all following nodes "

the assumption being that if we disambiguated the current node via
LABELLED_BY, we don't need to disambiguate dups beyond the first one
found.  Possibly it would be better to do this disambiguation loop in
all cases, for consistency of labels.

My pseudocode nly loops from current forward, and avoids the double
looping.

In any case I think we need to avoid the heavy level of nesting in the
first patch.

"Possibly it would be better to do this disambiguation loop in
all cases, for consistency of labels"

I definitely think so.  It will also help the user disambiguate a
duplicated name, by not having to notice the other concatenated
version and use deduction.  To much cognitive load on a user who is
trying to look at gok and their app and perform their work all at the
same time ;-)

"My pseudocode nly loops from current forward, and avoids the double
looping."

I'm still not clear on this and still haven't had my coffee ;-)  We
need scan for duplicates for every different name - agreed?

I'll make some improvements, and in so doing, perhaps I'll
understand...  I still haven't confirmed if my patch even works though
:-(  (I can understand you not wanting to contaminate your sandbox)

here's the key problem with the first patch:

* for each node, you call gok_spy_is_duplicated_name()
  (which traverses the tree).  Also I don't know why you need to
  strcpy the name before passing it here.

* if the answer is 'yes', you must search _again_ to find 
  each duplicate.  For the case of two duplicated strings, this means:

-> search forward to first dup, return 'yes'
-> if 'yes', loop through to end of list doing strcmp again.

ugh, if the dup is near the end of the list, you have strcmp'ed your
way through the entire list twice.  Alternatively, you could

* set start-search to 'current->next', and return first duplicate
  as dup_node
* disambiguate it, set start-search to dup_node->next
* repeat until dup_node returned by the search method returns NULL

For the simple case of two duplicate names (above), this would mean

* search for duplicates, return the (first and only) dup_node
* disambiguate
* search from dup->node, return false, and you're done.

In this case you will see that you have traversed the list only once
inside the outermost while.

If there are no duplicates the search is roughly N2/2, if there are
duplicates the search is also about N2/2 (since the various inner
loops don't backtrack, they only search forwards from where they 
left off).

does that make more sense?

This is why I suggested changing the 'is_dup' API to return a node. 
Also I think making the disambiguation code live in a separte small
function call makes sense from a clarity and nesting point of view.

That is clear to me.  (Interestingly I have had some coffee)  And I
agree with breaking out a function.

Thanks Bill.

Created attachment 22092 [details] [review]
i am happier with this patch.

Created attachment 22093 [details] [review]
i am happier with this patch (corrected private protoype name)

looks much better, agreed :-)

a minor nit: you may not need to check the return from
AccessibleRelation_getTarget if you are not exceeding nTargets.

also the code in gok_spy_distinguish_node_anem which appends the label
is surely wrong... you are for isntance freeing node->pname
immediately after assigning it, etc.  I think there's a leak
currently, and I still have no idea why you are doing a strncpy in
gok_spy_resolve_namesakes; you don't need to do that prior to doing
the various strcmps.
but the basic logic looks good now.

i store the name to preserve it. once fixed, the prepended version
will (hopefully) no longer have matches, so we use the original name
to look for matches in the remaining nodes.

i'll attach another version fixing my free-ing madness ;-)

Created attachment 22097 [details] [review]
revised patch

new patch does not appear to disambiguate the first node, only its
namesakes.  e.g. gok_spy_distinguish_node_name() is never called on
'node' in gok_spy_resolve_namesakes.

I also think that we need more than 15 chars of name; but less than
1000 (as in the early patches ;-)  In some cases truncation to 15
chars could actually increase ambiguity rather than decreasing it. 
I'd suggest something like 32 chars.

Adding PATCH keyword.

Agreed - almost there...

Created attachment 22128 [details] [review]
this patch works.  will commit in a couple hours.

Fixed in cvs.  Thanks for review Bill.