add a GotoLineModal

[jfo]

Associated Issue: #4210

Summary of Changes

This commit adds a gotoline modal/functionality triggered with : in
the main debugger source window.

• Cmd+: opens the modal
• Entering a line number takes you to that line in the current source
• Clicking "x" closes the panel
• unusable input (i.e. letters) result in a noop

This is not ready to merge but it is ready for feedback. I've commented on the diff with some things! Also needs tests, would like some guidance on that via SymbolModal.spec.js, I assume.

[jfo]

idk if this is the right place for this. Also the key is a placeholder, the suggested Cmd+G is already in use.

[jfo]

this whole pattern is from @codehag's diff on the associated issue.

It feels like these (SymbolModal, ShortcutModal, GotoLineModal) should all be the same thing ultimately, but I thought this would be a good start before merging them together.

[jasonLaster]

Yeah... we do want to consolidate the symbol + gotoline modal and search for file.

• cmd+p
• cmd+:
• cmd+shift+o

[jfo]

• cmd+p
• cmd+shift+o

For the goto one... CmdOrCtrl+Shift+; will work, but CmdOrCtrl+: does not work, nor does CmdOrCtrl+Shift+:. Is this correct? This could be revisted when you unify them, whatever you'd like for right now is fine by me :)

[jfo]

I'm pretty sure this file is actually not being used anywhere (it's also called in SymbolModal.js but the .symbol-modal-wrapper and .symbol-modal classes are never applied afaict). I might suggest merging the remaining rules into shared/Modal.css instead but that's just outside the scope of this pr.

[wldcordeiro]

It looks the those rules are applied?

https://github.com/devtools-html/debugger.html/blob/master/src/components/SymbolModal.js#L227 .input-wrapper

and then the result list? https://github.com/devtools-html/debugger.html/blob/master/src/components/SymbolModal.js#L213

[jfo]

Sorry, I wasn't clear, I meant that only .symbol-modal-wrapper and .symbol-modal classes don't seem to be applied and the remaining rules that I linked to might be incorporated into Modal.css in the process of merging the modals together more, but I'm not planning to do that on this PR.

[flodolo]

Use …, not ...

[jfo]

It took me a while to figure out what you meant by this, that the first is a unicode character is hard to discern from the text. Putting them in pre fences would help distinguish them i.e.
... vs ….

[flodolo]

Good point, I'll keep it in mind.

(in case you're seeing this comment twice, I re-added comment in the right thread).

[flodolo]

Please stop copying localization comments without updating them

[codehag]

A more constructive way to say this would be "Please update this localization note to reflect the new key"

[flodolo]

It wasn't meant as an hostile comment towards the author, if it came across like that, I apologize.

For context: this is happening in several PRs every week, so I think there's a general issue to solve. Commenting strings is not mandatory, comments are there to give context to translators. On the other hand, I have the feeling most developers think they are, but copying and paste an existing comment is not the solution.

[jfo]

It did come across like that, so thank you for apologizing. I have no knowledge of what other PR's look like (this is my very first one of any substance) or that the comments are optional, I'm just trying to match what's around the usage with the expectation of being corrected if necessary, so thanks for catching it, it was an oversight.

[flodolo]

Does this even work? IIRC you need to press shift to get to that keyboard in English, and that's a modifier.

[jfo]

It does work. There is an attached gif.

[codehag]

Hi @flodolo , thanks for helping our contributors with their prs. I would like to point out that the wording of this comment can be read as aggressive, and we want our contributors to feel safe and that their changes are welcome. Phrasing such as "Does this even work" can be off-putting for a contributor, and can also turn away potential contributors who are looking through our PRs to get a sense of the community. It is especially problematic to have this on something that is clearly labelled as a work in progress, since the person may be looking for feedback.

If you find something amiss, think of how to word things in a way that are not judgemental or polarizing.

[jfo]

Sorry. Yes. You do have to press shift to get to that symbol, but this syntax works to trigger that. In any case, it's a temporary key that can be changed to anything that makes the most sense.

[flodolo]

As above, this comment wasn't meant as hostile or derogatory.

I can't fully understand in which state a PR is when it's open (I'm not a developer, I work on localization), if it's testable or just an early WIP (it looked like that), hence the question "Does : even work?". There was no underlying implication, i.e. "are you even testing your code?", in my question.

I don't know exactly how devtools or debugger manage shortcuts internally, but I don't think it would work for a feature in Firefox, since we stumbled upon it for shortcuts on special characters like [ or ]

[codehag]

thanks for clarifying! And again thanks for helping :)

[flodolo]

You still need to update the comment itself (you only updated the reference to the string ID, but the text describes a different function).

At this point I would suggest to drop this comment completely, put this string near the one above (that already has a comment explaining the function), and call this string gotoLineModal.commandkey to avoid any potential confusion.

There's no rule set in stone for key naming, but .key is more prone to misinterpretation that .commandkey.

[jfo]

Oh that's a good idea.

To reiterate, Cmd+: is definitely temporary, just needed something to use for testing b/c the suggested (in the linked issue) Cmd+g is already being used. I think ultimately just : would be a good shortcut from an already opened modal but definitely not as a global shortcut key.

[jasonLaster]

Looks great! very minor tweaks around the edges

[jasonLaster]

I think we might want to handle "Escape" too

[jfo]

Escape already closes out the modal in both SymbolModal and GotoLineModal, it's handled more globally here via here and here.

[jasonLaster]

this is just a style thing, but i tend to prefer early returns

if (e.key === "Enter" && query != null) {
 if (!isNaN(linenumber)) {
   selectSource(selectedSource.get("id"), { line: linenumber })
 }
 return this.closeModal();
}

if (e.key === "Tab") {
  return this.closeModal();
}

[jfo]

I'll defer to you on that 😃 ! But, as those return values aren't being dispatched on in the caller, I'd prefer

if (e.key === "Enter" && query != null) {
  const linenumber = parseInt(query.replace(/[^\d+]/g, ""), 10);
  if (!isNaN(linenumber)) {
    selectSource(selectedSource.get("id"), { line: linenumber })
  }
  this.closeModal();
  return;
}

if (e.key === "Tab") {
  this.closeModal();
  return;
}

When I was moving some logic to this from SymbolModal.js I had trouble grokking this line, because I assumed the return value was significant. It can't be, since updateResults always returns undefined, but semantically that was unclear.

Is there a technical reason to avoid else if in favor of multiple if blocks? Like a branching implementation thing?

[jasonLaster]

I'm surprised that this is not cmd+:

[jasonLaster]

Yeah... we do want to consolidate the symbol + gotoline modal and search for file.

• cmd+p
• cmd+:
• cmd+shift+o

[jasonLaster]

we'll likely need to update the flow definition for the possible search types in reducers/ui.js

[jfo]

do you mean here? I am surprised flow did not catch that for me in a warning... I guess I don't really know how flow works 😃

[jasonLaster]

by the way, we have an open PR to fix the position of the modal: #4277

it should always be pinned to the top...

[jfo]

Thanks all for the good feedback! Will this require tests?

[jasonLaster]

@jfo i don't think so

[wldcordeiro]

Great job! I'm digging this and I'm really excited for the later consolidation of the modals. 😄 Just the tweaks that @jasonLaster mentioned but I think this is great.

[jfo]

Hi all, I've addressed the outstanding reviews and rebased against master, I think this should be ready to merge now but let me know if there is anything else needed.

Thanks a lot! 🙌

[jasonLaster]

🎁