Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

[Sources] Auto expand path of selected source#5716

Merged
darkwing merged 2 commits intofirefox-devtools:masterfrom
AnshulMalik:auto-focus-tree
Apr 6, 2018
Merged

[Sources] Auto expand path of selected source#5716
darkwing merged 2 commits intofirefox-devtools:masterfrom
AnshulMalik:auto-focus-tree

Conversation

@AnshulMalik
Copy link
Copy Markdown
Contributor

@AnshulMalik AnshulMalik commented Mar 19, 2018

Summary of Changes

  • Expand all nodes in the path of the selected source in sources tree
  • write tests for this

Screenshots/Videos (OPTIONAL)

auto-expand

@AnshulMalik AnshulMalik force-pushed the auto-focus-tree branch 2 times, most recently from 9cc7356 to ea2abe9 Compare March 19, 2018 14:52
while (index <= reversedItems.length - 2) {
this.setExpanded(reversedItems[index], true, false);
index++;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, can you discuss what this does?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment to make it more clear

Comment thread src/reducers/source-tree.js Outdated
function InitialState(): Record<SourceTreeState> {
return makeRecord({
expanded: null
expanded: new Set()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@darkwing
Copy link
Copy Markdown
Contributor

Kicked Travis, let's see if we pass now :)

@darkwing
Copy link
Copy Markdown
Contributor

We'll want a mochitest or two for this but I looooove this functionality. Keep up the great work @AnshulMalik !

@AnshulMalik
Copy link
Copy Markdown
Contributor Author

I am confused with what's going on in browser_dbg-asm.js

diff --git a/src/test/mochitest/head.js b/src/test/mochitest/head.js
index 873d1ada..f8dd68ce 100644
--- a/src/test/mochitest/head.js
+++ b/src/test/mochitest/head.js
@@ -212,8 +212,12 @@ function waitForSources(dbg, ...sources) {
       }

       if (!sourceExists(store.getState())) {
+        info(`WAITING FOR ${url}`);
         return waitForState(dbg, sourceExists, `source ${url}`);
       }
+
+      info('RESOLVING DIRECTLY, ALREADY EXISTS');
+      return true;
     })
   );
 }

It says, wait for two sources, but the sources are already loaded, Promise.all() should resolve immediately (I see RESOLVING DIRECTLY, ALREADY EXISTS twice for the two sources).
But it's not resolving. Not sure what's the problem here

@darkwing
Copy link
Copy Markdown
Contributor

Been trying to figure out the issue here but having no luck so far. Oddly enough, the tests pass for me locally.

@jasonLaster
Copy link
Copy Markdown
Contributor

probably unrelated, but a rebase is always healthy

@jasonLaster jasonLaster changed the title Auto expand path of selected source [Sources] Auto expand path of selected source Mar 27, 2018
@darkwing darkwing force-pushed the auto-focus-tree branch 2 times, most recently from c958a1c to c10d7a7 Compare April 2, 2018 21:10
Comment thread src/components/shared/ManagedTree.js Outdated

let index = highlightItems.length - 2;

if (this.props.autoExpandOnHighlight) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I turned autoExpandOnHighlight off and it didn't change the fact that the test failed.

Comment thread src/components/shared/ManagedTree.js Outdated
return this.focusItem(highlightItems[0]);
}

let index = highlightItems.length - 2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should comment this; the -2 value is a mystery

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment in the while loop below sheds some light. I should move it up

@AnshulMalik AnshulMalik force-pushed the auto-focus-tree branch 3 times, most recently from 01d450c to 356d9d1 Compare April 6, 2018 05:10
Copy link
Copy Markdown
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @AnshulMalik !!

@darkwing darkwing merged commit f62e34d into firefox-devtools:master Apr 6, 2018
@AnshulMalik AnshulMalik deleted the auto-focus-tree branch April 6, 2018 16:50
jasonLaster pushed a commit that referenced this pull request Apr 10, 2018
* auto expand path of selected source

* add test
jasonLaster added a commit to jasonLaster/debugger.html that referenced this pull request Apr 12, 2018
darkwing pushed a commit that referenced this pull request Apr 12, 2018
jasonLaster added a commit that referenced this pull request Apr 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants