Bug 154561 – Web Inspector: TimelineRuler should have a "select all" mode

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
TimelineRuler should have a "select all" mode. When active, selection handles should be hidden, and the selection start and end properties should return the ruler's start and ends times. Clicking & dragging to create a new selection should cause the ruler to no longer "select all".

UI making use of this mode will be added in: https://bugs.webkit.org/show_bug.cgi?id=153033.

[Radar WebKit Bug Importer]

<rdar://problem/24779872>

[Matt Baker]

Created attachment 271964 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 271964 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=271964&action=review

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:265
> +    set selectAll(x)

entireRangeSelected? select, as a verb bothers me, as a property name.

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:297
> +        return this._selectAll ? this._endTime : this._selectionEndTime;

This is still a bounded time. I suspect this will cause things to filter out when you zoom in to tighten the time range. Does this result get used?

[Matt Baker]

(In reply to comment #3)
> Comment on attachment 271964 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271964&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:265
> > +    set selectAll(x)
> 
> entireRangeSelected? select, as a verb bothers me, as a property name.
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:297
> > +        return this._selectAll ? this._endTime : this._selectionEndTime;
> 
> This is still a bounded time. I suspect this will cause things to filter out
> when you zoom in to tighten the time range. Does this result get used?

We could use the default selection bounds (0 and Infinity), and make the necessary adjustments to timeline views.

[Timothy Hatcher]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 271964 [details]
> > [Patch] Proposed Fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=271964&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:265
> > > +    set selectAll(x)
> > 
> > entireRangeSelected? select, as a verb bothers me, as a property name.
> > 
> > > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:297
> > > +        return this._selectAll ? this._endTime : this._selectionEndTime;
> > 
> > This is still a bounded time. I suspect this will cause things to filter out
> > when you zoom in to tighten the time range. Does this result get used?
> 
> We could use the default selection bounds (0 and Infinity), and make the
> necessary adjustments to timeline views.

That would be easier to understand.

[Matt Baker]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 271964 [details]
> > > [Patch] Proposed Fix
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=271964&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:265
> > > > +    set selectAll(x)
> > > 
> > > entireRangeSelected? select, as a verb bothers me, as a property name.
> > > 
> > > > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:297
> > > > +        return this._selectAll ? this._endTime : this._selectionEndTime;
> > > 
> > > This is still a bounded time. I suspect this will cause things to filter out
> > > when you zoom in to tighten the time range. Does this result get used?
> > 
> > We could use the default selection bounds (0 and Infinity), and make the
> > necessary adjustments to timeline views.
> 
> That would be easier to understand.

The grid-based timeline views handle this fine, but OverviewTimelineView and MemoryTimelineView will need special handling for selectionEndTime === Infinity.

[Matt Baker]

Created attachment 272090 [details]
[Video] "select all" behavior

Doubling-clicking the ruler toggles the ruler between the user-defined selection (if any), and "everything selected".

It's a convenient interaction for the initiated, but not very discoverable. Upcoming UI (see https://webkit.org/b/153033) will make it accessible.

[Matt Baker]

Created attachment 272093 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 272093 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=272093&action=review

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:304
> +        return this._selectionStartTime === this._zeroTime && this._selectionEndTime === Number.MAX_VALUE;

You might need to add this in set zeroTime().

if (this.entireRangeSelected) { this._selectionStartTime = newZeroTime; }

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:708
> +    _handleDoubleClick(event)
> +    {
> +        if (this.entireRangeSelected) {
> +            this.selectionStartTime = this._lastSelectionStartTime;
> +            this.selectionEndTime = this._lastSelectionEndTime;
> +        } else {
> +            this._lastSelectionStartTime = this.selectionStartTime;
> +            this._lastSelectionEndTime = this.selectionEndTime;
> +
> +            this.selectEntireRange();
> +        }
> +    }

I like double click to get the entire range. I don't think double click to go back to the previous selection is as clear or compelling. The user can always just drag out a new selection.

But this is fine as-is.

[Matt Baker]

Created attachment 272115 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 272115 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 272115

Committed r197034: <http://trac.webkit.org/changeset/197034>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Comment on attachment 272115 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=272115&action=review

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:179
> +        if (this.entireRangeSelected)
> +            this.selectionStartTime = this._zeroTime;

This needed to happen before "this._zeroTime = x", otherwise the getter for entireRangeSelected will fail, because _selectionStartTime will not equal _zeroTime anymore.

[Timothy Hatcher]

I'll land a follow up fix.

[Timothy Hatcher]

Landed a follow up in https://trac.webkit.org/r197035.