Skip to content
This repository was archived by the owner on Nov 25, 2020. It is now read-only.

autocomplete: clear active item after selection #84

Merged
merged 1 commit into from
Sep 24, 2014
Merged

autocomplete: clear active item after selection #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/components/autocomplete/autocompletecontroller.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ module.exports = class KDAutoCompleteController extends KDViewController
event.stopPropagation()
event.preventDefault()
return no
else
when 40 #downarrow
if @dropdown.getView().$().is(":visible")
@dropdown.getListView().goDown()
Expand Down Expand Up @@ -190,7 +189,9 @@ module.exports = class KDAutoCompleteController extends KDViewController
inputView = @getView()
# log @getOptions().selectedItemsLimit, @selectedItemCounter
if @getOptions().selectedItemsLimit is null or @getOptions().selectedItemsLimit > @selectedItemCounter
activeItem = @dropdown.getListView().getActiveItem()
listView = @dropdown.getListView()
activeItem = listView.getActiveItem()
listView.setActiveItem null
if activeItem.item
@appendAutoCompletedItem()
@addItemToSubmitQueue activeItem.item
Expand Down
3 changes: 3 additions & 0 deletions src/components/autocomplete/autocompletelist.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,6 @@ module.exports = class KDAutoCompleteListView extends KDListView
active.index = i
break
active

setActiveItem: (target) ->
item.active = item is target for item in @items
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think itemDataPath option should play a part here if it is set to a non-null value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is designed to be symmetrical to the getter. Can you explain your idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identity of a list item is determined by itemDataPath when an object is passed as an argument to a method of list view. I thought keeping the same behavior in similar views would be consistent semantically. Equality operator is not always enough to determine same object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sinan can you advise about this? I am not sure I understand about the item data path. In the case of autocomplete, i believe that the is operator will be sufficient to compare list items.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have two different copies of the very same object then is operator returns false and comparing argument of the function against list data misses the item you want to be processed.

This is what we have experienced with removing an item from list view. Data was fetched from two different sources. Identical two objects stored in different places in the memory. is operator was returning false. So, implementation improved to take data path into account if it's provided to compare identity of the argument and item in the list.

This is the case I wanted to be covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but I don't think that can happen for this method, which is just going to set the active item for the autocomplete view, and not for list views in general. If you want me to make this change, can you please open a PR against my fork that implements your suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I can find time to do that then I will open another pull request. This shouldn't hold off the pull request. I'll be merging this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I would make the change myself, but I wasn't able to grok your suggestion. I don't understand how I should check the identity of the object by the item data path, which is just a string.