[SourcesTree] Set root directory by AnshulMalik · Pull Request #4962 · firefox-devtools/debugger

AnshulMalik · 2017-12-21T06:34:53Z

Associated Issue: #4121

Summary of Changes

Change the root of sources tree to custom one

Screenshots/Videos (OPTIONAL)

AnshulMalik · 2017-12-21T07:02:36Z

-        click: () => setProjectDirectoryRoot(item.path)
+        click: () => {
+          const curRoot = this.props.projectRoot;
+          let newRoot = item.path;


Should I move this calculation in setProjectDirectoryRoot ?

jasonLaster · 2017-12-21T14:40:26Z

Great work! Can't wait to review :)

jasonLaster · 2017-12-21T18:52:48Z

dont worry about the travis test. that is a known bug

jasonLaster · 2018-01-02T15:50:23Z

-        click: () => setProjectDirectoryRoot(item.path)
+        click: () => {
+          const curRoot = this.props.projectRoot;
+          let newRoot = item.path;


yeah, that makes sense

jasonLaster · 2018-01-02T15:50:23Z

+            temp.splice(0, 2);
+            newRoot = `${curRoot}/${temp.join("/")}`;
+          }
+          this.props.setProjectDirectoryRoot(newRoot);


do you think we can do this in this.props.setProjectDirectoryRoot?

jasonLaster · 2018-01-02T15:50:23Z


+  if (projectRoot) {
+    const rootParts = projectRoot.replace("://", "").split("/");
+    parts.splice(0, rootParts.length - 2);


this logic looks really similar to the other logic. What's going on here?

If the project root is set, let's say /example.com/components, then it's path is /components
so we just keep the last part (I am thinking of making it easy to grasp, maybe use url library)

Now we start the tree path with /components.

jasonLaster · 2018-01-05T22:32:24Z

Waiting to see if this should be cleaned up and maybe another test...

lukaszsobek · 2018-01-19T06:46:03Z

 }

-export function setProjectDirectoryRoot(url: Object) {
+export function setProjectDirectoryRoot(curRoot: string, newRoot: string) {


Do we need to change anything if curRoor == newRoot? Maybe perform setProjectDirectoryRoot only after checking that the two roots are different.

I thought it'd be better if we keep the logic inside the function.

lukaszsobek · 2018-01-19T07:15:05Z

 }
+
+function isUnderRoot(url, projectRoot) {
+  if (!projectRoot) {


Not sure I'd expect isUnderRoot to return true if there is no root.

It's a hack when the project root is not set :P

lukaszsobek · 2018-01-19T07:19:23Z

        disabled: false,
-        click: () => setProjectDirectoryRoot(item.path)
+        click: () =>
+          this.props.setProjectDirectoryRoot(this.props.projectRoot, item.path)


maybe we spread this.props before putting it into the clickhandler ... seems a bit hard to read as it is now.

lukaszsobek · 2018-01-19T07:20:47Z

+      const domain = "http://example.com/";
+      const sources = createSourcesList(testData);
+      const root = "/example.com/components";
+      // const root = "";


lukaszsobek · 2018-01-19T07:22:09Z

+      - components
+      - TodoTextInput.js
+      - Header.js
+     */


is it a working version, or does the comment indicate there is something special to note here?

Comments were there to remind the tree structure, but they are not required, i'll remove them

jasonLaster · 2018-01-23T02:10:40Z

Looks good! I'll test soon

jasonLaster · 2018-01-24T20:46:48Z

Did a quick rebase
I found a couple rough edges
- it requires 2 times to set the root dir
- re-selecting the same node should show "Remove Root Directory"
I think the redux action should have the curRoot check based on a redux call...

diff --git a/src/actions/ui.js b/src/actions/ui.js
index 7e823d7..c2f03a8 100644
--- a/src/actions/ui.js
+++ b/src/actions/ui.js
@@ -138,9 +138,19 @@ export function closeConditionalPanel() {
 }
 
 export function setProjectDirectoryRoot(url: Object) {
-  return {
-    type: "SET_PROJECT_DIRECTORY_ROOT",
-    url
+  return ({ dispatch, getState }: ThunkArgs) => {
+    const curRoot = getProjectRoot(getState());
+
+    let newRoot = item.path;
+    if (curRoot) {
+      const temp = newRoot.replace("://", "").split("/");
+      temp.splice(0, 2);
+      newRoot = `${curRoot}/${temp.join("/")}`;
+    }
+    dispatch({
+      type: "SET_PROJECT_DIRECTORY_ROOT",
+      url
+    });
   };
 }
 
diff --git a/src/components/PrimaryPanes/SourcesTree.js b/src/components/PrimaryPanes/SourcesTree.js
index c45398f..000e5cb 100644
--- a/src/components/PrimaryPanes/SourcesTree.js
+++ b/src/components/PrimaryPanes/SourcesTree.js
@@ -246,22 +246,13 @@ class SourcesTree extends Component<Props, State> {
       menuOptions.push(copySourceUri2);
     }
 
-    if (isDirectory(item) && features.root) {
+    if (isDirectory(item) && isCurrentRoot(item.path, props.projectRoot)) {
       menuOptions.push({
         id: "node-set-directory-root",
         label: setDirectoryRootLabel,
         accesskey: setDirectoryRootKey,
         disabled: false,
-        click: () => {
-          const curRoot = this.props.projectRoot;
-          let newRoot = item.path;
-          if (curRoot) {
-            const temp = newRoot.replace("://", "").split("/");
-            temp.splice(0, 2);
-            newRoot = `${curRoot}/${temp.join("/")}`;
-          }
-          this.props.setProjectDirectoryRoot(newRoot);
-        }
+        click: () => this.props.setProjectDirectoryRoot(item.path)
       });
     }

jasonLaster · 2018-01-26T00:54:59Z

+export function setProjectDirectoryRoot(newRoot: string) {
+  return ({ dispatch, getState }: ThunkArgs) => {
+    const curRoot = getProjectDirectoryRoot(getState());
+    if (newRoot.length === 0) {


hmm, it would be nice to call a separate action clearProjectDirectoryRoot or pass in null

jasonLaster · 2018-01-26T00:54:59Z

    .then(([{ actualLocation }, bpClient]) => {
      actualLocation = createBreakpointLocation(location, actualLocation);
-      const id = makeLocationId(actualLocation);
+      const id = makePendingLocationId(actualLocation);


where is this coming from :)

looks like this PR desperately needs a rebase ⤴️ ⤵️

AnshulMalik · 2018-01-28T12:12:08Z

    const { dispatch, getState } = createStore();
    const projectRoot = getProjectDirectoryRoot(getState());
-    dispatch(actions.setProjectDirectoryRoot(projectRoot));
+    dispatch(actions.setProjectDirectoryRoot(projectRoot, projectRoot));


This change is not present locally to me, don't know why it's showing here

jasonLaster · 2018-01-29T18:03:35Z

started some jest tests and found some weird issues...

jasonLaster · 2018-01-29T18:05:58Z

fixed!

AnshulMalik commented Dec 21, 2017

View reviewed changes

AnshulMalik changed the title ~~[WIP] [SourcesTree] Set root directory~~ [SourcesTree] Set root directory Dec 21, 2017

jasonLaster reviewed Jan 2, 2018

View reviewed changes

AnshulMalik force-pushed the change-root-sources branch from b5502ea to 2d5f6a6 Compare January 12, 2018 16:50

lukaszsobek reviewed Jan 19, 2018

View reviewed changes

AnshulMalik force-pushed the change-root-sources branch from 05ac108 to c3ca3bf Compare January 22, 2018 10:37

jasonLaster force-pushed the change-root-sources branch from c3ca3bf to 3e00b93 Compare January 24, 2018 20:43

AnshulMalik requested a review from flodolo as a code owner January 25, 2018 17:25

jasonLaster reviewed Jan 26, 2018

View reviewed changes

AnshulMalik force-pushed the change-root-sources branch from 0e1c3ba to 70a25c1 Compare January 26, 2018 09:36

jasonLaster force-pushed the change-root-sources branch from e12e817 to 6ac1baf Compare January 26, 2018 16:18

AnshulMalik force-pushed the change-root-sources branch 2 times, most recently from 6912047 to db7a1fc Compare January 28, 2018 10:24

AnshulMalik commented Jan 28, 2018

View reviewed changes

set root directory

cd6693f

jasonLaster force-pushed the change-root-sources branch from db7a1fc to cd6693f Compare January 29, 2018 17:58

tests

c4636f7

jasonLaster force-pushed the change-root-sources branch from d9504f2 to c4636f7 Compare January 29, 2018 18:05

jasonLaster merged commit 57035f7 into firefox-devtools:master Jan 29, 2018

AnshulMalik deleted the change-root-sources branch January 29, 2018 19:04

jasonLaster pushed a commit that referenced this pull request Jan 30, 2018

[SourcesTree] Set root directory (#4962)

db87d88

Conversation

AnshulMalik commented Dec 21, 2017

Summary of Changes

Screenshots/Videos (OPTIONAL)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Dec 21, 2017

Uh oh!

jasonLaster commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnshulMalik Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Jan 5, 2018

Uh oh!

lukaszsobek Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaszsobek Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Jan 23, 2018

Uh oh!

jasonLaster commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Jan 29, 2018

Uh oh!

jasonLaster commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AnshulMalik Jan 3, 2018 •

edited

Loading

lukaszsobek Jan 19, 2018 •

edited

Loading

lukaszsobek Jan 19, 2018 •

edited

Loading