Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

listView Filter Improvements #254

Open
wants to merge 9 commits into
base: x64
Choose a base branch
from
Open

Conversation

MarkKoz
Copy link

@MarkKoz MarkKoz commented Nov 22, 2021

Summary

Two major enhancements have been made to the filtering capabilities of listView:

  1. Pressing Enter cycles through selection of all matches. If it reaches the last match, then it cycles back to the first match.
  2. A box can be checked to only show items that match the filter. This can be freely toggled on and off.

Implementation

The first feature is fairly straight forward to implement. However, the second added significant complexity. Some things I had to do:

  • Add an Angular filter on the items, which is triggered whenever a filter changes or the toggle to only show matches changes. This re-applies all filters if only matches are chosen to be shown.
  • Keep track of the previous index both for the entire array and for the filtered one. The index for the items array cannot be set directly. Instead, the filtered index has a setter which is responsible for updating both values to ensure they stay in sync. I'm not a big fan of this approach. I think it's confusing to keep track of two previous indices, but I haven't come up with a better solution yet.
  • Add a watchCollection for the array of filtered items. This is used to convert the previous index when the checkbox for only showing matches is toggled.

Known Issues

Angular Filter Triggers Unnecessarily

Something I cannot figure out is why the Angular filter is triggered (sometimes twice in a row) for seemingly unnecessary actions e.g. just clicking on an empty space in the list or selecting an item. It happens even when items is the only parameter to the filter function. This is only evident if a breakpoint is set in that function.

This Angular filter may be redundant anyway. Perhaps it's feasible to manually update an array of filtered items. After all, it needs to update when either the items change, the filters change, or the toggle changes, and there are event handlers/watchers for each of those already.

Possibly Redundant Call When Array Updates

The watchCollection calls $scope.filterChanged(), but in retrospect I am not sure if that's necessary. The original line of thinking was that the array may be updated by inserting or deleting an item somehow, in which case the filters would have to be re-applied and a new first index computed.

Selected Item May Be Out of View

When toggling only showing matches, it does not scroll to the selected item again, meaning it may go out of view. Since the currently selected index is not tracked, there's no way to know where to scroll to. It could do a linear search for the first selected index, but not sure if that's worth it. Always tracking the index as a variable seems annoying since it has to get converted sometimes like the previous index is currently converted.

filterChanged was only executing before the filteredItems array got
updated. Thus, the index it was finding was not necessarily going to be
accurate once filteredItems was updated.
For example, take 3 items named 1, 2, and 3. If 2 is dragged so that the
order becomes 2, 1, 3, then the next item being selected was 3 rather
than 1. This was because the `filteredValue` setter was setting the
unfiltered `value` using the `index` property of the item in the array,
but `itemsReordered` had not been handled yet. This meant that the
`index` was still the item's previous position. When the watchCollection
for `filteredItems` was triggered due to the drag causing a re-ordering,
it was setting `filteredValue` to `value`, which had the wrong index.

Fix this by setting `filteredValue` after `itemsReordered` is handled.
The `adjust` variable in `onItemDrop` was incorrectly evaluating to
false because the length comparison used `filteredItems`'s lengths. At
the time of comparison, that array is not updated yet to reflect the
change caused by `getItem`.
@alandtse
Copy link

alandtse commented Dec 5, 2021

Thanks for doing this. It's a feature zedit has needed for a while. I hope it gets merged.

For example (to help encourage a close on them if it does get merged):
#222
#213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants