#4420 Context Menu "Jump to Original Location" should be disabled when not pretty-printed #4497

jainsneha23 · 2017-10-26T12:52:52Z

Associated Issue: #4420

Summary of Changes

Get if selected source is pretty-printed. If yes, disable "Reveal in tree" and "Blackbox script"
Get open tabs list. Get pretty url for the selected source. If pretty tab is open, disable "Jump to Original Location"

Test Plan

Open a tab, "Jump to Original Location" is disabled
Click on pretty print in the footer
"Jump to Original Location" is enabled
Go to the pretty printed tab
"Reveal in tree" and "Blackbox script" are disabled

Screenshots/Videos (OPTIONAL)

jasonLaster

skimmed it quickly and it looks great!

jasonLaster · 2017-10-26T17:16:32Z

src/components/Editor/EditorMenu.js


  const jumpLabel = {
    accesskey: L10N.getStr("editor.jumpToMappedLocation1.accesskey"),
-    disabled: false,
-    label: L10N.getFormatStr("editor.jumpToMappedLocation1", pairedType),
+    disabled: isGenerated ? false : !isPrettyTabOpen,


I'm a bit confused by this so i'll share the spec as i see it:

original files can not be pretty printed

generated files that have a source map cannot be pretty printed

if the source has been pretty printed, jumping should jump between the pretty and "ugly" file :) this might be new behavior...

jasonLaster · 2017-10-26T17:16:32Z

src/components/Editor/EditorMenu.js

@@ -96,7 +103,7 @@ function getMenuItems(
    id: "node-menu-blackbox",
    label: toggleBlackBoxLabel,
    accesskey: blackboxKey,
-    disabled: false,
+    disabled: isGenerated,


so... i think the comment before might help...
we want it to be disabled for pretty printed files, but nothing else...

jainsneha23 · 2017-10-27T06:27:56Z

@jasonLaster
The spec for this is:

jumpLabel = {
  isOriginal ? false : !isPrettyTabOpen
}

If the file opened is pretty printed(i.e. isOriginal), u will see "jump to generated location" always
Else if its generated, "Jump to Original Location" will be disabled if "!isPrettyTabOpen". As if its not pretty printed, you cannot jump to the original location

const blackBoxMenuItem = { disabled: isOriginal };
"Blackbox source" and "Reveal in tree" are also disabled for pretty printed (i.e, original source)

jainsneha23 · 2017-10-27T06:28:53Z

src/components/Editor/EditorMenu.js

-  const pairedType = isOriginalId(selectedLocation.sourceId)
-    ? L10N.getStr("generated")
-    : L10N.getStr("original");
+  const isOriginal = isOriginalId(selectedLocation.sourceId);


Check if the source is original, or generated ?

jainsneha23 · 2017-10-27T06:29:01Z

src/components/Editor/EditorMenu.js

-    ? L10N.getStr("generated")
-    : L10N.getStr("original");
+  const isOriginal = isOriginalId(selectedLocation.sourceId);
+  const isPrettyTabOpen =


If the source is generated, check if there is a open tab with its original

jainsneha23 · 2017-10-27T06:29:23Z

src/components/Editor/EditorMenu.js

+    label: L10N.getFormatStr(
+      "editor.jumpToMappedLocation1",
+      isOriginal ? L10N.getStr("generated") : L10N.getStr("original")
+    ),
    click: () => jumpToMappedLocation(sourceLocation)
  };


Jump to generated location is always enabled for original source

Jump to original location is enabled only if, its original location is open in a tab

jainsneha23 · 2017-10-27T06:30:08Z

src/components/Editor/EditorMenu.js

@@ -96,7 +102,7 @@ function getMenuItems(
    id: "node-menu-blackbox",
    label: toggleBlackBoxLabel,
    accesskey: blackboxKey,
-    disabled: false,
+    disabled: isOriginal,
    click: () => toggleBlackBox(selectedSource.toJS())


Blackbox menu is disabled for original source (prettified)

jasonLaster · 2017-10-27T14:04:11Z

Hey @jainsneha23 lets discuss O/G here:

https://github.com/devtools-html/debugger.html/pull/4512/files?short_path=c474cab#diff-c474cab07b2dd9f949f47376f7d47fd2

i'm still a bit confused about how you're using the term... we might also be inconsistent in the app

jainsneha23 · 2017-10-28T07:00:03Z

@jasonLaster Even I am confused here for terminology. For me the pretty printed file is generated, and the one i open first is original. But the terminology used in the same file before was the opposite. So, I sticked to it. If the code looks fine apart from the terminology for generated/original, please suggest the proper names. I will add it.

jasonLaster · 2017-10-28T14:00:41Z

^ haha - let me check to see what is going on in the debugger. the docs were my assumptions

jasonLaster · 2017-10-30T13:23:23Z

I just looked into it and found that the pretty source is in fact the original source. Super confusing :P

appStore.getState().sources.sources.valueSeq().toJS()
  .filter(s => s.url && s.url.includes("local"))
  .map(({id, url}) => ({id, url}))

jainsneha23 · 2017-10-31T08:08:31Z

So, if the pretty source is original , the terminology being used right now is correct. Then the code can is ready to review?

jasonLaster · 2017-10-31T13:24:40Z

src/components/Editor/EditorMenu.js

-    : L10N.getStr("original");
+  const isOriginal = isOriginalId(selectedLocation.sourceId);
+  const isPrettyTabOpen =
+    !isOriginal && tabs.includes(getPrettySourceURL(selectedSource.get("url")));


would this fail if we were looking at a different original file?

I think we should be able to get the selectedSource, which would have the URL and ignore the tab stuff

jasonLaster · 2017-10-31T13:24:40Z

src/components/Editor/EditorMenu.js

@@ -96,7 +102,7 @@ function getMenuItems(
    id: "node-menu-blackbox",
    label: toggleBlackBoxLabel,
    accesskey: blackboxKey,
-    disabled: false,
+    disabled: isOriginal,
    click: () => toggleBlackBox(selectedSource.toJS())


true, we don't want to blackbox original sources. This is because we don't know how to blackbox an original source that is part of a larger bundled (generated) source

jasonLaster · 2017-10-31T13:24:40Z

src/components/Editor/EditorMenu.js

@@ -107,7 +113,7 @@ function getMenuItems(
    id: "node-menu-show-source",
    label: revealInTreeLabel,
    accesskey: revealInTreeKey,
-    disabled: false,
+    disabled: isOriginal,


pretty printed files are not in the tree, but other original files that come from a bundle are.

Correct. I will fix this.

jasonLaster · 2017-10-31T13:24:40Z

src/components/Editor/EditorMenu.js

@@ -168,9 +174,11 @@ class EditorMenu extends PureComponent {
 export default connect(
  state => {
    const selectedSource = getSelectedSource(state);
+    const tabs = getSourceTabs(state);
    return {
      selectedLocation: getSelectedLocation(state),
      selectedSource,


oh good, we have a selectedSource

jasonLaster · 2017-10-31T13:24:40Z

src/components/Editor/EditorMenu.js

-    ? L10N.getStr("generated")
-    : L10N.getStr("original");
+  const isOriginal = isOriginalId(selectedLocation.sourceId);
+  const isPrettyTabOpen =


i like the idea of having an isPrettyPrinted variable

Yes, we will need both isOriginal and isPrettyPrinted. As they can be different.

bomsy · 2017-11-09T00:43:12Z

src/components/Editor/EditorMenu.js


  const jumpLabel = {
    accesskey: L10N.getStr("editor.jumpToMappedLocation1.accesskey"),
-    disabled: false,
-    label: L10N.getFormatStr("editor.jumpToMappedLocation1", pairedType),
+    disabled: isOriginal || hasSourceMap ? false : !isPrettyTabOpen,


Can we use !isPrettyPrinted here instead of !isPrettyTabOpen
then we won't need line 82??

jasonLaster · 2017-11-09T17:22:54Z

did a rebase here: https://github.com/jasonLaster/debugger.html/tree/jainsneha23-bug-4420

sorry, i couldn't push to your branch

jasonLaster · 2017-11-09T17:36:39Z

I think this is good to go. Sorry for the delay on reviewing.

I ended up doing some small tweaks to clean it up as it's super confusing

jainsneha23 · 2017-11-10T04:21:41Z

Great

… be disabled when not pretty-printed (firefox-devtools#4497) * Bug 4420 fix * tweaks

jasonLaster · 2017-11-14T16:16:32Z

jasonLaster reviewed Oct 26, 2017

View reviewed changes

jasonLaster added the 📚 pr-wip label Oct 26, 2017

jainsneha23 commented Oct 27, 2017

View reviewed changes

jasonLaster removed the 📚 pr-wip label Oct 27, 2017

jasonLaster added the 📚 pr-wip label Oct 30, 2017

jasonLaster added 📚 pr-needs-review and removed 📚 pr-wip labels Oct 31, 2017

jasonLaster reviewed Oct 31, 2017

View reviewed changes

bomsy reviewed Nov 9, 2017

View reviewed changes

Bug 4420 fix

1bc7af7

jasonLaster force-pushed the bug-4420 branch from 9292258 to 1bc7af7 Compare November 9, 2017 17:24

tweaks

0da9d9f

jasonLaster merged commit 26e1b1d into firefox-devtools:master Nov 9, 2017

khal0988 pushed a commit to khal0988/debugger.html that referenced this pull request Nov 12, 2017

firefox-devtools#4420 Context Menu "Jump to Original Location" should…

85f448d

… be disabled when not pretty-printed (firefox-devtools#4497) * Bug 4420 fix * tweaks

jainsneha23 deleted the bug-4420 branch November 12, 2017 13:35

jasonLaster mentioned this pull request Nov 14, 2017

[PrettyPrint] enable “Jump to Generated Location" #4690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4420 Context Menu "Jump to Original Location" should be disabled when not pretty-printed #4497

#4420 Context Menu "Jump to Original Location" should be disabled when not pretty-printed #4497

jainsneha23 commented Oct 26, 2017

jasonLaster left a comment

jasonLaster Oct 26, 2017

jasonLaster Oct 26, 2017

jainsneha23 commented Oct 27, 2017

jainsneha23 Oct 27, 2017

jainsneha23 Oct 27, 2017

jainsneha23 Oct 27, 2017

jainsneha23 Oct 27, 2017

jasonLaster commented Oct 27, 2017

jainsneha23 commented Oct 28, 2017

jasonLaster commented Oct 28, 2017 •

edited

Loading

jasonLaster commented Oct 30, 2017 •

edited

Loading

jainsneha23 commented Oct 31, 2017

jasonLaster Oct 31, 2017

jasonLaster Oct 31, 2017

jasonLaster Oct 31, 2017

jainsneha23 Nov 1, 2017

jasonLaster Oct 31, 2017

jasonLaster Oct 31, 2017

jainsneha23 Nov 1, 2017

bomsy Nov 9, 2017

jasonLaster commented Nov 9, 2017

jasonLaster commented Nov 9, 2017

jainsneha23 commented Nov 10, 2017

jasonLaster commented Nov 14, 2017

#4420 Context Menu "Jump to Original Location" should be disabled when not pretty-printed #4497

#4420 Context Menu "Jump to Original Location" should be disabled when not pretty-printed #4497

Conversation

jainsneha23 commented Oct 26, 2017

Summary of Changes

Test Plan

Screenshots/Videos (OPTIONAL)

jasonLaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainsneha23 commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonLaster commented Oct 27, 2017

jainsneha23 commented Oct 28, 2017

jasonLaster commented Oct 28, 2017 • edited Loading

jasonLaster commented Oct 30, 2017 • edited Loading

jainsneha23 commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonLaster commented Nov 9, 2017

jasonLaster commented Nov 9, 2017

jainsneha23 commented Nov 10, 2017

jasonLaster commented Nov 14, 2017

jasonLaster commented Oct 28, 2017 •

edited

Loading

jasonLaster commented Oct 30, 2017 •

edited

Loading