[PATCH] AutoCompletingComboBox gets initialized with wrong value for "key:subkey" key elements in custom presets.xml

[ax]

EDIT: please see comment #7 for how to reliably reproduce this bug. the original description below might be misleading.

---

how to reproduce:

in a data layer that has has both "key" and "key:subkey" elements, eg. elements tagged with "source=bing" as well as elements tagged with "source:population=psgc", if you try editing "source=bing" in the properties panel by double-clicking or pressing "edit", the key will be initialized with the wrong value "source:population=psgc". even if you only change the value and don't touch the key, the key will be changed nevertheless because it is wrongly initialized!

cause:

AutoCompletingComboBox::insertString(..., "source", ...)​ calls AutoCompletingComboBox::lookupItem("source")​. This method loops through all existing keys in the current layer and returns a key that '''starts with''' "source"​. as "source" is before "source:population" in the list being iterated over, bestItem is first set to the correct value "source", but in the next iteration overwritten with the wrong value "source:population". this wrong key is then inserted into the combobox.

solution:

i'm not familiar with the autocomplete code. on a first glance, i don't understand why AutoCompletingComboBox::insertString() needs a fuzzy startsWith to insert a discret item. but there might be a reason behind this. which is why i leave the fix to this issue to someone who knows :)

[bastiK]

Replying to ax:

how to reproduce:

in a data layer that has has both "key" and "key:subkey" elements, eg. elements tagged with "source=bing" as well as elements tagged with "source:population=psgc", if you try editing "source=bing" in the properties panel by double-clicking or pressing "edit", the key will be initialized with the wrong value "source:population=psgc". even if you only change the value and don't touch the key, the key will be changed nevertheless because it is wrongly initialized!

Tried this, but could not reproduce. Can you provide a *.osm file, e.g. with only 2 nodes in it?

[ax]

Replying to bastiK:

Tried this, but could not reproduce. Can you provide a *.osm file, e.g. with only 2 nodes in it?

sure - here you go. one node containing both "key" and "key:subkey" actually is enough. try editing the "source" of attached node.

[ax]

or might this be because of a custom preset file containing "source:population"? i'll attach that as well.

[ax]

oh well - seems to be the preset file indeed. if i enable that and load 5876.osm, i get the bug as described. anything wrong with the presets_ax.xml?

[ax]

finally able to reliably reproduce this:

• add attached presets_bug_5876.xml to your presets (Edit > Preferences > Map Settings > Tagging Presets > Active Presets > Add; OK; Restart)
• open a new empty layer
• add an empty node
• apply the preset "Bug 5876" to this point (accept default)
• to the same node, add "source=bla" via properties panel
• select the "source=bla" line in the properties panel and double-click or "Edit"

result: in the panel that pops up, the selected value in the "Key" combobox is "source:population", not "source" as expected.

bastiK, please help! :)

[bastiK]

I appreciate the good bug description, but it comes only for special configurations, so it has no special priority for me.

[ax]

looking at this a little more. with the preconditions as described in comment #7, ie.

1. a "key:subkey", eg. "source:population", in preset.xml, and
2. no "key", eg. "source", in preset.xml

this is what is happening in AutoCompletingComboBox::AutoCompletingComboBoxDocument::insertString()​, before and after the call to lookupItem()​:

// lookup and select a matching item
// DEBUG: curText == "source"
Object item = lookupItem(curText);  
// DEBUG: item.value == "source:population"

this is obvisiously wrong and the reason for this bug. so what is happening in lookupItem(curText)?:

private Object lookupItem(String pattern) {
    ComboBoxModel model = comboBox.getModel();
    AutoCompletionListItem bestItem = null;
    for (int i = 0, n = model.getSize(); i < n; i++) {
        AutoCompletionListItem currentItem = (AutoCompletionListItem) model.getElementAt(i);
        if (currentItem.getValue().startsWith(pattern)) {
            if (bestItem == null || currentItem.getPriority().compareTo(bestItem.getPriority()) > 0) {
                bestItem = currentItem;
            }
        }
    }
    return bestItem; // may be null
}

• we iterate over all possible autocomplete items (keys in this case). these items are composed of all preset keys, plus all keys from the currently active data layer. items have a priority, with boolean values for inDataSet, inStandard, selected (see AutoCompletionItemPritority). the priority for the source* keys in our test case is
  • for "source", inDataSet: true, inStandard: false, selected: false
  • for "source:population", inDataSet: true, inStandard: true, selected: false
  ie. "source:population" has a higher priority than source
• in the iteration, we check for an autocomplete item that startsWith the value we are editing ("source" in our testcase). of all the items that match this test ("source" and "source:population" in our testcase), we
• choose the one with the best autocomplete priority - in our testcase, this is "source:population" - the wrong element

so what is wrong here? i'm not sure. i think insertString() shouldn't do the whole stuff to lookupItem and beyond, but just return, which means selecting should be set - which it isn't. why - i don't know. i would appreciate any help.

[stoecker]

Try this inner part:

        if (currentItem.equals(pattern)) {
            return currentItem;
        }
        else if (currentItem.getValue().startsWith(pattern)) {
            if (bestItem == null || currentItem.getPriority().compareTo(bestItem.getPriority()) > 0) {
                bestItem = currentItem;
            }
        }

Does it have negative effects in other places?

[ax]

this will probably work - will try.

i have a feeling though that this is just a workaround around a conceptual problem. i want to populate a combobox with a given fixed value (it is there in properties panel, i double click it). why do i have to iterate through all possible values and do lookups if i have the value and know it is there?

[bastiK]

Replying to ax:

looking at this a little more. with the preconditions as described in comment #7, ie.

1. a "key:subkey", eg. "source:population", in preset.xml, and
2. no "key", eg. "source", in preset.xml

this is what is happening in AutoCompletingComboBox::AutoCompletingComboBoxDocument::insertString()​, before and after the call to lookupItem()​:

// lookup and select a matching item
// DEBUG: curText == "source"
Object item = lookupItem(curText);  
// DEBUG: item.value == "source:population"

this is obvisiously wrong and the reason for this bug. so what is happening in lookupItem(curText)?:

private Object lookupItem(String pattern) {
    ComboBoxModel model = comboBox.getModel();
    AutoCompletionListItem bestItem = null;
    for (int i = 0, n = model.getSize(); i < n; i++) {
        AutoCompletionListItem currentItem = (AutoCompletionListItem) model.getElementAt(i);
        if (currentItem.getValue().startsWith(pattern)) {
            if (bestItem == null || currentItem.getPriority().compareTo(bestItem.getPriority()) > 0) {
                bestItem = currentItem;
            }
        }
    }
    return bestItem; // may be null
}

• we iterate over all possible autocomplete items (keys in this case). these items are composed of all preset keys, plus all keys from the currently active data layer. items have a priority, with boolean values for inDataSet, inStandard, selected (see AutoCompletionItemPritority). the priority for the source* keys in our test case is
  • for "source", inDataSet: true, inStandard: false, selected: false
  • for "source:population", inDataSet: true, inStandard: true, selected: false
  ie. "source:population" has a higher priority than source
• in the iteration, we check for an autocomplete item that startsWith the value we are editing ("source" in our testcase). of all the items that match this test ("source" and "source:population" in our testcase), we
• choose the one with the best autocomplete priority - in our testcase, this is "source:population" - the wrong element

so what is wrong here? i'm not sure. i think insertString() shouldn't do the whole stuff to lookupItem and beyond, but just return, which means selecting should be set - which it isn't. why - i don't know. i would appreciate any help.

I think you are right so far. But another marker is required, 'selecting' is to prevent endless recursion when comboBox.setSelectedItem(item); would trigger another call to lookupItem(). What we need is another method in AutoCompletingComboBox that sets an item, and takes care that lookupItem() is not executed.

[stoecker]

i have a feeling though that this is just a workaround around a conceptual problem. i want to populate a combobox with a given fixed value (it is there in properties panel, i double click it). why do i have to iterate through all possible values and do lookups if i have the value and know it is there?

The AutoCompleteComboBox will never be 100% sure. There is too much guesswork involved to get results right in nearly all situations. It does good in most cases, but fails in others. The above fix should ensure only, that in case we have an exact match it is preferred.

[anonymous]

Replying to stoecker:

Try this inner part:

        if (currentItem.equals(pattern)) {
            return currentItem;
        }
        else if (currentItem.getValue().startsWith(pattern)) {
            if (bestItem == null || currentItem.getPriority().compareTo(bestItem.getPriority()) > 0) {
                bestItem = currentItem;
            }
        }

I guess this should work.

[bastiK]

anonymous <-- me

[ax]

Replying to bastiK:

I think you are right so far. But another marker is required, 'selecting' is to prevent endless recursion when comboBox.setSelectedItem(item); would trigger another call to lookupItem(). What we need is another method in AutoCompletingComboBox that sets an item, and takes care that lookupItem() is not executed.

a marker, or a test, yes. selecting looked like it could be used, but apparently it can not - thanks for the info.

i spent some more time on this and think i found a reliable test now to determine if the string being inserted originates from a valid autocomplete item (in which case it doesn't have to be looked up again), or from (unmatched) typing in the combobox (in which case it has to be looked up). this patch fixes my issue. would appreciate if you could have a look at it.

[ax]

on a (little) related note: at the beginning of insertString(),

@Override public void insertString(int offs, String str, AttributeSet a) throws BadLocationException {
    if (selecting || (offs == 0 && str.equals(getText(0, getLength()))))
        return;
    boolean initial = (offs == 0 && getLength() == 0 && str.length() > 1);
    super.insertString(offs, str, a);

    // return immediately when selecting an item
    // Note: this is done after calling super method because we need
    // ActionListener informed
    if (selecting)
        return;

there is one selecting too much. either the first or the second can be removed. wasn't sure if removing the first would have consequences, so i left it for now. if anyone knows ...

[bastiK]

Looks reasonable, but I cannot say from the source, if your patch does the right thing. Please test thoroughly (also in Relation editor and Find history). If you say it has no regressions, I will apply without further ado.

[ax]

i tested different datasets, small and big ones, incl. Relation editor and Find history, and didn't find any unexpected behavior. so from my side this could go in.

any option on comment:18?

[bastiK]

In [3866/josm]:

fixed #5876 (patch by ax) - AutoCompletingComboBox gets initialized with wrong value for "key:subkey" key elements in custom presets.xml

[bastiK]

Replying to ax:

any option on comment:18?

I wouldn't twiddle with ugly and complicated code, just to make it look a little better. (Except for comments and really trivial things.) The risk of breaking things is too high and there is no gain in functionality.

[ax]

Replying to bastiK:

Replying to ax:

any option on comment:18?

I wouldn't twiddle with ugly and complicated code, just to make it look a little better. (Except for comments and really trivial things.) The risk of breaking things is too high and there is no gain in functionality.

it would make the code more comprehensible and a little less error-prone. and doing this here and there could go a long way. but of course i see your point.

maybe i give this a concentrated look another time. let's first see if my patch doesn't break anything :) thanks for applying!

[stoecker]

The second return is only superfluous, when super.insertString() does not change selecting. And even if this is the case who knows if it stays that way in future ;-)

[bastiK]

ax: Please have a look at #5927

[ax]

Replying to bastiK:

ax: Please have a look at #5927

will do.

[ax]

Replying to ax:

will do.

done.