1543261 - Opening conditional panel with keyboard shortcut should display previously saved condition

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Anthony Xie]

Attachment: conditional_panel_shortcut.gif

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

1. Open debugger on a page and right click on a gutter line number
2. Popup menu should appear, click "add condition"
3. Conditional panel should open, add a condition
4. Press Enter to save conditional breakpoint, conditional panel should close
5. Click on the line with conditional breakpoint in the editor
6. Now use the keyboard shortcut to open the conditional panel on that line (Ctrl+Shift+B or Cmd+Shift+B)

What happened?

Actual Result: Conditional panel opens without previously saved condition

What should have happened?

Expected Result: Conditional panel opens populated with previously saved condition

Anything else we should know?

If a conditional breakpoint is created using the keyboard shortcut, we do not see this problem

Comment 1

[David Walsh :davidwalsh]

Since we now use column breakpoints, this requires a bit more thought. A few things to think about:

• The gutter marker represents the first activated or potential breakpoint in the line.

• What do we do if there's an active column breakpoint and another existing conditional breakpoint on the line?

Comment 2

[Alex R.]

Hi David and Anthony. I've actually been working on this and came up with a potential solution to the original bug:

It involves a couple changes to src/components/Editor/ConditionalPanel.js:

• the function getBreakpointForLocation returns only the location, while we want the breakpoint object from state (which includes the condition).

• importing and replacing this with the similarly named function getBreakpointAtLocation from src/selectors/breakpointAtLocation.js solves the problem.

Here's a demo:
http://g.recordit.co/pXQd2RIzdC.gif

This causes a flow error which I am still investigating:

Cannot call `getBreakpointAtLocation` with `location` bound to `location` 
because  null [1] is incompatible with  `LineColumn` [2].Flow(InferError)
ui.js(177, 4): [1] null 
breakpointAtLocation.js(66, 65): [2] `LineColumn`

I was also thinking about the case of additional column breakpoints...
Currently (as you stated above) my solution always opens the gutter breakpoint (first breakpoint on the line).

Would it be preferable for the shortcut to open the column breakpoint closest to where the user has clicked?
Or, maybe we want to display all the column breakpoints on the line and allow the user to choose one using the keyboard?

I'm happy to continue working on this...
Thanks.

Comment 3

[Alex R.]

One approach for dealing with column breakpoints would be to select the closest to the cursor position (when conditional panel is opened).

(possibly with getBreakpointsAtLine from src/selectors/breakpointAtLocation.js.)

The cursor position is rendered to the DOM inside the Editor/SourceFooter, src/components/Editor/Footer.js, and I'm contemplating how to pass cursorPosition and line breakpoints to the Props in src/components/Editor/ConditionalPanel.js.

Does this seem like a reasonable approach for handling opening conditional panel with the keyboard shortcut?
Is it reasonable to create a new action that would get the cursor position?

Comment 4

[David Walsh :davidwalsh]

I think using the cursor position would be a good idea! I don't think we need a new action but a function in the utils/editor directory. We'll probably find some edge cases along the way. Worst case scenario is we default to the first column position and use that for the conditional add/edit.

Comment 5

[Alex R.]

Attachment: Bug 1543261 - Conditional Panel: Load correct conditions when opened with keyboard shortcut r=davidwalsh

Previously, the conditional panel did not correctly load breakpoint conditions when opened with the keyboard shortcut.
This patch loads the conditions, and selects the closest active column breakpoint on same line as cursor position in the Editor.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/458af3b79e09
Conditional Panel: Load correct conditions when opened with keyboard shortcut .Add functionality of selecting the closest active column  breakpoint based on current cursorPosition in Editor.  r=davidwalsh

Comment 7

[Cosmin Sabou [:CosminS]]

Backed out by: https://hg.mozilla.org/integration/autoland/rev/489507a3818665e2bc0992fbe90834d4fc320e2c

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1630eaa0a61c
Conditional Panel: Load correct conditions when opened with keyboard shortcut r=davidwalsh

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2f41d635be8
Backed out changeset 1630eaa0a61c for causing devtools failure on browser_dbg-breakpoints-cond-source-maps.js CLOSED TREE

Comment 10

[Arthur Iakab [arthur_iakab]]

Backed out changeset 1630eaa0a61c (bug 1543261) for causing devtools failure on browser_dbg-breakpoints-cond-source-maps.js CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/b2f41d635be8f3a5be35ae47316b7352d18b7465

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1630eaa0a61c467264fd573ba4c9fb4431c67929&searchStr=devtools

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=267284989&repo=autoland

Alex can you please take a look?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25a282bca05b
Conditional Panel: Load correct conditions when opened with keyboard shortcut r=davidwalsh

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/25a282bca05b