fix unselect multiple entiries on right click by UziTech · Pull Request #1115 · atom/tree-view

UziTech · 2017-05-22T21:21:46Z

Description of the Change

fixes unselecting multiple entries when right clicking on a selected entry.

winstliu · 2017-05-22T22:01:06Z

Nice. Can you please add a spec for this?

UziTech · 2017-05-23T02:16:03Z

When ever I try to run the tests on my machine I get

Error: Cannot find module '../build/Release/pathwatcher.node'

winstliu · 2017-05-23T03:02:56Z

Did you apm install instead of npm install? Followed by maybe an apm rebuild?

UziTech · 2017-05-23T03:49:57Z

That worked thank you

UziTech · 2017-06-15T18:41:08Z

Any word on this? @50Wliu is there a reason the needs-testing flag is still on this?

winstliu · 2017-06-15T21:32:07Z

Sorry for the delay. needs-testing can serve different purposes - I use it to denote that a feature needs to be tested (as in, by interacting with the change in Atom), not that it needs specs.

Finishing up my last few language-php PRs and working on a language-html PR, then I'll probably get to this.

UziTech · 2017-06-15T22:05:30Z

sounds good. thanks

winstliu · 2017-06-15T22:08:54Z

There's some styling issues going on when multi-selecting: the Tree View gets pushed slightly to the left. Looks like a lot of the styles only target .full-menu and not .multi-select.

Full Menu	Multi Select

winstliu · 2017-06-15T22:20:26Z

/cc @simurai: could you help with the above? ☝️ It looks like you added the .full-menu styles in #1111.

Following diff seems to work, but I'm wondering if there's a better way.

diff --git a/styles/tree-view.less b/styles/tree-view.less
index b0851a7..83db906 100644
--- a/styles/tree-view.less
+++ b/styles/tree-view.less
@@ -13,7 +13,7 @@
   display: flex;
   flex-direction: column;
 
-  .full-menu {
+  .full-menu, .multi-select {
     /*
      * Force a new stacking context to prevent a large, duplicate paint layer from
      * being created for tree-view's scrolling contents that can make the cost of
@@ -30,14 +30,14 @@
     padding-right: @component-padding;
     background-color: inherit;
 
-    // Expands .full-menu to take up full height.
+    // Expands .full-menu and .multi-select to take up full height.
     // This makes sure that the context menu can still be openend in the empty
     // area underneath the files.
     flex-grow: 1;
   }
 
-  .full-menu.list-tree {
-    // Expands .full-menu to take up as much width as needed by the content.
+  .full-menu.list-tree, .multi-select.list-tree {
+    // Expands .full-menu and .multi-select to take up as much width as needed by the content.
     // This makes sure that the selected item's "bar/background" expands to full width.
     position: relative;
     min-width: min-content;
@@ -47,10 +47,6 @@
     position: relative;
   }
 
-  .list-tree {
-    position: static;
-  }
-
   .entry {
     // This fixes #110, see that issue for more details
     &::before {

UziTech · 2017-06-16T00:01:02Z

it seems like just using .list-tree would work better than .full-menu, .multi-select. That way if any other state class was added in the future we wouldn't run into the same issue.

UziTech · 2017-06-16T00:18:55Z

never mind .list-tree seems to be on any expanded folder. A new class should probably be added.

simurai · 2017-06-16T06:38:34Z

@50Wliu It looks like you added the .full-menu styles in #1111.

Ouch.. 😩 Didn't realize that .full-menu is more like a "state". Also, it seems that

.list-tree {
  position: static;
}

is still needed when dragging.

@UziTech A new class should probably be added.

Yes, added a new .tree-view-root class in #1130.

winstliu

Looks good. Just a few minor spec comments.

winstliu · 2017-06-17T04:43:48Z

spec/tree-view-spec.js

+        button: 2
+      })
+
+      expect(treeView.getSelectedEntries().length).toBe(2);


How about another assertion to make sure that the multi-select class is present?

winstliu · 2017-06-17T04:44:34Z

spec/tree-view-spec.js

+      treeView.showMultiSelectMenu()
+
+      let child = entries.children[0];
+      while (child.children.length > 0 && (child = child.firstChild));


Can this while loop be expanded?

winstliu · 2017-06-17T04:45:26Z

Going to block this on #1130 to ensure that we don't forget about that.

mrveress · 2017-07-18T10:47:36Z

Hello everyone! Sorry for the question: when this functionality be in the new release? I waiting this almost 3 month and press context button on the keyboard (my context key almost destroyed because of this 😄 )

UziTech · 2017-07-19T15:31:34Z

@mrveress kind of hacky but I am using this in atom now by doing the following steps:

Fork this branch
Change the name property in package.json to "tree-view-temp" (or anything that is not the name of another package)
Disable the core "tree-view" package in atom
Install the forked package with the github url

Just make sure to uninstall that package and enable "tree-view" when it ships with this merged, or keep the forked branch up to date.

simurai · 2017-07-21T02:44:31Z

FYI: PR #1130 is now merged.

winstliu · 2017-07-21T03:15:42Z

@UziTech please rebase and then I'll give this a final test run.

UziTech · 2017-07-21T03:34:14Z

All test passed 👍 💯

winstliu · 2017-07-21T04:02:47Z

spec/tree-view-spec.js

+      const entries = treeView.roots[0].entries
+      treeView.selectEntry(entries.children[0])
+      treeView.selectMultipleEntries(entries.children[1])
+      treeView.showMultiSelectMenu()


To avoid explicitly stating this in the spec, is it possible to do a treeView.onMouseDown event on entries.children[1]?

winstliu · 2017-07-21T04:37:36Z

👍

fix unselect multiple entiries on right click

6e0dff9

UziTech mentioned this pull request May 22, 2017

Right-Click With Folder Selected + New File #252

Open

winstliu added the requires-changes label May 22, 2017

add test

ba9b331

winstliu added needs-review needs-testing and removed requires-changes labels May 23, 2017

Merge branch 'master' of https://github.com/atom/tree-view into patch-1

01ff0b2

winstliu mentioned this pull request Jun 15, 2017

[DO NOT MERGE] Testing Appveyor #1129

Closed

simurai mentioned this pull request Jun 16, 2017

Fix multi-select shifting #1130

Merged

winstliu reviewed Jun 17, 2017

View reviewed changes

winstliu added blocked and removed needs-review needs-testing labels Jun 17, 2017

add check for multi-select

6f72f97

simurai added needs-review needs-testing and removed blocked labels Jul 21, 2017

Merge remote-tracking branch 'refs/remotes/atom/master' into patch-1

aea3d78

Merge remote-tracking branch 'refs/remotes/atom/master' into patch-1

acdc16b

winstliu reviewed Jul 21, 2017

View reviewed changes

use treeView.onMouseDown

bff166c

winstliu merged commit 8d28d89 into atom:master Jul 21, 2017

This was referenced Jul 21, 2017

Deleting multiple files results in only a single file being deleted and the rest becoming deselected #1050

Closed

Can't move selected files to another folder #1033

Closed

UziTech deleted the patch-1 branch August 3, 2017 22:05

Conversation

UziTech commented May 22, 2017

Description of the Change

Uh oh!

winstliu commented May 22, 2017

Uh oh!

UziTech commented May 23, 2017

Uh oh!

winstliu commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UziTech commented May 23, 2017

Uh oh!

UziTech commented Jun 15, 2017

Uh oh!

winstliu commented Jun 15, 2017

Uh oh!

UziTech commented Jun 15, 2017

Uh oh!

winstliu commented Jun 15, 2017

Uh oh!

winstliu commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UziTech commented Jun 16, 2017

Uh oh!

UziTech commented Jun 16, 2017

Uh oh!

simurai commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winstliu left a comment

Choose a reason for hiding this comment

Uh oh!

winstliu Jun 17, 2017

Choose a reason for hiding this comment

Uh oh!

winstliu Jun 17, 2017

Choose a reason for hiding this comment

Uh oh!

winstliu commented Jun 17, 2017

Uh oh!

mrveress commented Jul 18, 2017

Uh oh!

UziTech commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simurai commented Jul 21, 2017

Uh oh!

winstliu commented Jul 21, 2017

Uh oh!

UziTech commented Jul 21, 2017

Uh oh!

winstliu Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

winstliu commented Jul 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

winstliu commented May 23, 2017 •

edited

Loading

winstliu commented Jun 15, 2017 •

edited

Loading

simurai commented Jun 16, 2017 •

edited

Loading

UziTech commented Jul 19, 2017 •

edited

Loading