919707 - Cannot select whole lines by double-clicking the gutter

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P2)

Description

[Anton Kovalyov (:anton)]

This is a regression from 912260.

Comment 1

[Anton Kovalyov (:anton)]

Attachment: WIP 1/2: Double click to select the whole line (delay with breakpoints)

Comment 2

[Anton Kovalyov (:anton)]

Attachment: WIP 1/1: Click on the line numbers gutter selects a line, click on the breakpoints gutter sets a breakpoint

Comment 3

[Anton Kovalyov (:anton)]

Implemented two approaches, not sure which one is better. With double-clicking there's a problem (which was also a problem before CM) where double clicking a gutter in the debugger quickly adds and removes a breakpoint in addition to selecting a string. My implementation solves that but it introduces a 500ms delay between the click event and when the breakpoint appears since we have to wait to see if its a click or a double click.

Second implementation is much simpler and uses single clicks for everything. Clicking on a line numbers gutter selects a line, clicking on a breakpoints gutter sets a breakpoint. This is how Sublime does this.

I like second implementation but I'm interested in other opinions.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Have you tried doing this with a dblclick handler rather than trying to implement your own?

Second option sounds better to me, but am happy to hear other options as well.

I personally find the behavior in sublime a little confusing, fwiw. I have had the occasional frustrating experience trying to set a bookmark on a line and highlighting, then hunting around for the shortcut key. Would like to spare our users that if at all possible.

Maybe we need a special Breakpoint Only gutter to avoid confusion?

Comment 5

[Anton Kovalyov (:anton)]

> Have you tried doing this with a dblclick handler rather than trying to implement your own?

You can't with CodeMirror. Due to its implementation it exposes only gutterClick event which is actually a mousedown.

> Maybe we need a special Breakpoint Only gutter to avoid confusion?

We have a breakpoint only gutter? This is what the second implementation is about.

Comment 6

[Victor Porof [:vporof][:vp]]

If I'm reading this correctly, the second implementation implies that clicking on line numbers doesn't add breakpoints anymore. This would be a big UX problem.

In the past, we've had significant anger (and snark) from users that thought our debugger is useless because they didn't know how to add breakpoints. There was no (visual or otherwise) indication that clicking on the "empty space in the gutter, on the left of the line numbers" adds a breakpoint. Almost everybody expects that clicking on line numbers adds breakpoints. A "gutter" means "where the line numbers are" for most if not all people, not differentiating between "the breakpoints gutter" and "the line numbers gutter".

We've had so (so!) many bugs and dupes filed about this that It's almost embarrassing. It would make me extremely sad to see us going through that again.

I don't think comparing to Sublime Text's approach is fair, because it's not an IDE, so you don't add breakpoints. I'm willing to bet that adding code markers is a significantly less used functionality than breakpoints. As a parallel, we're not using Sublime's keyboard shortcuts, we're using Chrome's instead, because of familiarity reasons. The same argumentation should be for UX (not UI), especially with a piece of functionality like adding breakpoints, which is deeply engrained in people's workflows and muscle memory.

The way I understand it:

* The first implementation keeps the current behavior, but adds a 500ms delay; while I understand the reasoning, it can't possibly be a good idea to regress responsiveness/snappiness for a piece of functionality (select whole lines by double clicking) that is far, far from widely used, especially in a debugger.

* The second approach gets us back to the debugger's dark ages, in which people didn't use our tool because they had no idea how to add breakpoints. This issue was extensively discussed and argued upon, and I find no new arguments for us to revert to the old approach.

I don't see why double clicking and adding/removing breakpoints in the process is a bad idea. The frontend/backend is already suited for handling such things, and nothing breaks. The only awkward UX-y problem might be the icon flashing in the gutter, but I'm very much in favor of that very minor inconvenience that loosing users because they don't know how to add breakpoints.

Please let's not regress this.

Comment 7

[Mihai Sucan [:msucan]]

What I would suggest:

- no breakpoints-specific gutter versus line numbers gutter. we don't need the confusion between the two.
- single click on a line number toggles a breakpoint.
- dblclick event on a line number for whole line selection. you could make an upstream patch to expose the dblclick event.
- if the dblclick event can't be added, then simply use shift-click for whole line selection.

I don't like the idea of adding an arbitrary delay for detection of double click (it's an ugly hack).

Comment 8

[Anton Kovalyov (:anton)]

Attachment: Shift-Click to select whole lines

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8338837 [details] [diff] [review]
Shift-Click to select whole lines

Review of attachment 8338837 [details] [diff] [review]:
-----------------------------------------------------------------

I'm ok with this implementation if Victor and Mihai will allow it.

Comment 10

[Anton Kovalyov (:anton)]

That was Mihai's suggestion per comment 7. As for Victor, he asked not to regress breakpoints so my patch honors this as well.

Comment 11

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/31f4ad17dddb

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Anton Kovalyov (:anton, @valueof) from comment #10)
> That was Mihai's suggestion per comment 7. As for Victor, he asked not to
> regress breakpoints so my patch honors this as well.

Thank you.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31f4ad17dddb