Modify

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#5876 closed defect (fixed)

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

Reported by: ax Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: Cc:

Description (last modified by 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 :)

Attachments (4)

5876.osm (608 bytes) - added by ax 11 years ago.
presets_ax.xml (3.7 KB) - added by ax 11 years ago.
presets_bug_5876.xml (249 bytes) - added by ax 11 years ago.
josm_5876.patch (1.3 KB) - added by ax 11 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 in reply to:  description ; Changed 11 years ago by 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?

comment:2 in reply to:  1 Changed 11 years ago by 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.

Changed 11 years ago by ax

Attachment: 5876.osm added

comment:3 Changed 11 years ago by ax

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

Changed 11 years ago by ax

Attachment: presets_ax.xml added

comment:4 Changed 11 years ago by 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?

comment:5 Changed 11 years ago by ax

Priority: majornormal
Summary: AutoCompletingComboBox gets initialized with wrong value if data layer has both "key" and "key:subkey" elementsAutoCompletingComboBox gets initialized with wrong value if data layer has both "key" and "key:subkey" elements and there is a "key:subkey" in presets.xml

comment:6 Changed 11 years ago by ax

Priority: normalmajor
Summary: AutoCompletingComboBox gets initialized with wrong value if data layer has both "key" and "key:subkey" elements and there is a "key:subkey" in presets.xmlAutoCompletingComboBox gets initialized with wrong value for "key:subkey" key elements in custom presets.xml

comment:7 Changed 11 years ago by 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! :)

Changed 11 years ago by ax

Attachment: presets_bug_5876.xml added

comment:8 Changed 11 years ago by ax

Description: modified (diff)

comment:9 Changed 11 years ago by bastiK

Priority: majornormal

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

comment:10 Changed 11 years ago by 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.

comment:11 Changed 11 years ago by 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?

comment:12 Changed 11 years ago by 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?

comment:13 in reply to:  10 ; Changed 11 years ago by 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.

comment:14 in reply to:  12 Changed 11 years ago by 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.

comment:15 in reply to:  11 Changed 11 years ago by 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.

comment:16 Changed 11 years ago by bastiK

anonymous <-- me

comment:17 in reply to:  13 Changed 11 years ago by 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.

Changed 11 years ago by ax

Attachment: josm_5876.patch added

comment:18 Changed 11 years ago by 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 ...

comment:19 Changed 11 years ago by ax

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

comment:20 Changed 11 years ago by 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.

comment:21 Changed 11 years ago by 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?

comment:22 Changed 11 years ago by bastiK

Resolution: fixed
Status: newclosed

In [3866/josm]:

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

comment:23 in reply to:  21 ; Changed 11 years ago by 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.

comment:24 in reply to:  23 Changed 11 years ago by 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!

comment:25 Changed 11 years ago by 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 ;-)

comment:26 Changed 11 years ago by bastiK

ax: Please have a look at #5927

comment:27 in reply to:  26 ; Changed 11 years ago by ax

Replying to bastiK:

ax: Please have a look at #5927

will do.

comment:28 in reply to:  27 Changed 11 years ago by ax

Replying to ax:

will do.

done.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.