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

[filter] Bug fixes + Open item on enter if only one item is displayed #31

Merged
merged 6 commits into from
Sep 4, 2018

Conversation

urban-1
Copy link

@urban-1 urban-1 commented Aug 25, 2018

Hi @newinnovations - sry for disappearing but I was in the middle of changing jobs...

I tried to reproduce #30 but I couldn't. However, in the process I found few a minor bug with the filter function on files-view panel. When the file list is filtered, up/down arrow selection was not working as expected: it was selecting the hidden files in the background instead of only selecting what is displayed.

Along the way of fixing this, I added 2 small features:

  1. Alt+r l (that is L) will give focus to the filter/location bar
  2. Other than serving as a filter, this could be used as a location navigation, so hitting enter would take you to the typed path (ex: "/var/log" + enter will load the /var/log folder in the files-view). Now, if a single file/entry is displayed after filtering, enter will actually open that file or folder (using @confirmed() method)

Demo (one of my first gifs so not really a master-piece...):

demo

@urban-1
Copy link
Author

urban-1 commented Aug 25, 2018

(one thing to note for #30 is that I tried with atom 1.23.3 - need to upgrade and try again - just realized)

@newinnovations
Copy link
Owner

I do not really like the fact that the enter key is ambiguous: chdir/open file depending on the number of files in the list.
You could do chdir when the input box contains a slash, otherwise drop down into the list of files. I also think arrow-down should drop into the list.

@urban-1
Copy link
Author

urban-1 commented Aug 27, 2018

Sounds good, will update the branch later today :)

Thinking of it, should I combine all the above?

  1. If only one entry, open it: user would either hit backspace, or enter/arrow-down to move to the list and then open. So we are shortening the second option
  2. If "/" in filter, treat as chdir
  3. Enter without any of the above, focus on list

@urban-1
Copy link
Author

urban-1 commented Aug 27, 2018

Ok the desired behavior should be there. Both arrow-down and enter (when there is no / in the filter) move focus to the list. However, it seemed natural as I was testing it that if you are at the top of the list and you hit up-arrow to go back into editing the filter.

Finally, while testing fixed the following:

  • Clearing the filter when entering a folder
  • selectInitialItem was not taking into consideration the filtered items (kinda hidden since one could only go there with the mouse)

@newinnovations
Copy link
Owner

Much improved! When I press a backslash in the filter I get an uncaught regex exception in line 106 (files-view.coffee).

@newinnovations
Copy link
Owner

Pressing enter with a single file still opens it. And entering a name that does not match anything with enter throws an exception.

@urban-1
Copy link
Author

urban-1 commented Aug 30, 2018

  • Removed single file opening (altho I kinda liked it...)
  • Fixed the backslash issue by making the filtering based on str.include() rather than match(). The last is more powerful but assumes the user writes correct regex
  • Enter was a bit of a different problem: It happened on double-enter, the first was focusing on the list and the second was trying to open null! The bus was there before but hidden. Now enter on a filter with no results should have no effect (stays on the filter). However, the issues can be seen by manually clicking on the empty list and hitting enter. This was fixed in listEnter method

@newinnovations newinnovations merged commit 1096674 into newinnovations:master Sep 4, 2018
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