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

Improve behavior of TreeView::moveUp() with nested directories #1069

Merged
merged 8 commits into from Aug 11, 2018

Conversation

Projects
None yet
4 participants
@synthetiv
Copy link
Contributor

synthetiv commented Apr 15, 2017

Description of the Change

The previous moveUp() behavior checked whether the entry directly before the currently selected entry was an expanded directory, and if so, it selected the last child of that directory. That was perfectly fine for most cases, but if that last child entry was also an expanded directory, the moveUp() command would simply select the child directory instead of selecting its last entry. This pull request changes that behavior so that tree-view will recursively check whether the previous entry is an expanded directory, then check whether the last entry in that directory is too, and so on, before finally selecting the first entry it encounters that is either not a directory or not expanded.

Alternate Designs

An earlier revision of this change didn't explicitly check whether previousEntry was a directory or was expanded, and it also called TreeView::selectEntry() with each recursive iteration. That didn't seem to break anything, but it probably wasn't a great idea.

Benefits

Keyboard navigation of the tree-view will be a lot nicer, especially for users who have the "sort directories before files" option turned off.

Possible Drawbacks

I can't foresee any.

Applicable Issues

#1020
Fixes #1261

@50Wliu 50Wliu added the needs-review label Apr 17, 2017

@synthetiv

This comment has been minimized.

Copy link
Contributor Author

synthetiv commented Apr 18, 2017

Hi, @50Wliu – let me know if/when there's anything I should change. It wasn't obvious to me whether or not the failed beta build was related to my edits.

Thanks!

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Apr 18, 2017

If/when I review this I'll tell you :). It's not obvious to me whether or not the Appveyor failure is related to this PR (doesn't look like it), so I've restarted the build just in case.

@synthetiv

This comment has been minimized.

Copy link
Contributor Author

synthetiv commented Jun 26, 2018

This merges cleanly with master, and based on my limited user testing I'd say it's working fine, but for some reason I can't get the Jasmine tests to run at all (on any branch). Not sure if I'm running them wrong or if there's something wrong with the tests...

Edit: I just needed to run apm rebuild and apm install. I can confirm the tests are passing after merging locally.

@50Wliu 50Wliu self-assigned this Jun 28, 2018

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jul 3, 2018

@50Wliu can we help you with this?

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Jul 4, 2018

I assigned this to myself so I wouldn't forget about it. Plan to take a look after putting the finishing touches on one of my other PRs.

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Jul 4, 2018

@synthetiv what do you think about the following diff?

diff --git a/lib/tree-view.coffee b/lib/tree-view.coffee
index d72b91e..d9855cf 100644
--- a/lib/tree-view.coffee
+++ b/lib/tree-view.coffee
@@ -421,8 +421,6 @@ class TreeView
     selectedEntry = @selectedEntry()
     if selectedEntry?
       if previousEntry = @previousEntry(selectedEntry)
-        while previousEntry.classList.contains('directory', 'expanded') and previousEntry.entries.children.length > 0
-          previousEntry = _.last(previousEntry.entries.children)
         @selectEntry(previousEntry)
       else
         @selectEntry(selectedEntry.parentElement.closest('.directory'))
@@ -445,12 +443,21 @@ class TreeView
     return null
 
   previousEntry: (entry) ->
-    currentEntry = entry
-    while currentEntry?
-      currentEntry = currentEntry.previousSibling
-      if currentEntry?.matches('.entry')
-        return currentEntry
-    return null
+    previousEntry = entry.previousSibling
+    while previousEntry? and not previousEntry.matches('.entry')
+      previousEntry = previousEntry.previousSibling
+
+    return null unless previousEntry?
+
+    # If the previous entry is an expanded directory,
+    # we need to select the last entry in that directory,
+    # not the directory itself
+    if previousEntry.matches('.directory.expanded')
+      entries = previousEntry.querySelectorAll('.entry')
+      if entries.length > 0
+        return entries[entries.length - 1]
+
+    return previousEntry
 
   expandDirectory: (isRecursive=false) ->
     selectedEntry = @selectedEntry()

This adjusts the previousEntry method itself instead of working around it in moveUp.

In addition, can you add a test for the case when there is an expanded directory that contains no children entries?

@50Wliu 50Wliu added under-review and removed needs-review labels Jul 4, 2018

@synthetiv

This comment has been minimized.

Copy link
Contributor Author

synthetiv commented Jul 4, 2018

That seems like a good improvement, thanks. I'll apply that change and add a new test. If I don't get to it today, I'll get to it early next week.

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Jul 31, 2018

Hey @synthetiv, just checking in! If you're busy, I can push a commit to your fork applying that diff and adding the test.

@JK-mber

This comment has been minimized.

Copy link

JK-mber commented Aug 9, 2018

Hey @synthetiv and @50Wliu, any news on this one? I'm not sure what's still needed, but I'm keen to help where I can - I'm annoyed by this issue multiple times a day 😄

50Wliu added some commits Aug 9, 2018

@50Wliu 50Wliu force-pushed the synthetiv:master branch from f743b9e to d7d7d7a Aug 11, 2018

@50Wliu 50Wliu force-pushed the synthetiv:master branch from d7d7d7a to 7f33532 Aug 11, 2018

@50Wliu 50Wliu merged commit 1d1e4a8 into atom:master Aug 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.