Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781863003:
[DevTools] Filtered list enhancements (Closed)
|
Created:
3 years, 8 months ago by eostroukhov Modified:
3 years, 8 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Filtered list enhancements
Some enhancements that are needed for the frames menu but also seem
useful for other clients of the FilteredListWidget.
1. Selection is preserved while the filter string is being updated.
2. Items are highlighted on hover. Provider is notified when mouse
moves over the item.
3. It is now possible to set the selection.
BUG=698027
Review-Url: https://codereview.chromium.org/2781863003
Cr-Commit-Position: refs/heads/master@{#460799}
Committed: https://chromium.googlesource.com/chromium/src/+/76c041dbcc4de828135d0af8c39256c4861b1a01
Patch Set 1 #
Total comments: 14
Patch Set 2 : [DevTools] Enhancements for a filtered list #
Total comments: 6
Patch Set 3 : [DevTools] Enhancements for a filtered list #
Total comments: 4
Patch Set 4 : [DevTools] Enhancements for a filtered list #
Total comments: 2
Messages
Total messages: 30 (17 generated)
Description was changed from
==========
[DevTools] Enhancements for a filtered list
Some enhancements that are needed for the frames menu but also seem
useful for other clients of the FilteredListWidget.
1. Selection is preserved while the filter string is being updated.
2. Items are highlighted on hover. Provider is notified when mouse
moves over the item.
3. It is now possible to set the selection.
BUG=698027
==========
to
==========
[DevTools] Enhancements for a filtered list
Some enhancements that are needed for the frames menu but also seem
useful for other clients of the FilteredListWidget.
1. Selection is preserved while the filter string is being updated.
2. Items are highlighted on hover. Provider is notified when mouse
moves over the item.
3. It is now possible to set the selection.
BUG=698027
==========
Description was changed from
==========
[DevTools] Enhancements for a filtered list
Some enhancements that are needed for the frames menu but also seem
useful for other clients of the FilteredListWidget.
1. Selection is preserved while the filter string is being updated.
2. Items are highlighted on hover. Provider is notified when mouse
moves over the item.
3. It is now possible to set the selection.
BUG=698027
==========
to
==========
[DevTools] Filtered list enhancements
Some enhancements that are needed for the frames menu but also seem
useful for other clients of the FilteredListWidget.
1. Selection is preserved while the filter string is being updated.
2. Items are highlighted on hover. Provider is notified when mouse
moves over the item.
3. It is now possible to set the selection.
BUG=698027
==========
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org, dgozman@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:51: this._hoveredItem = null; nit: this._hoveredItemIndex https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:54: this._selectedItem = null; ...Index as well? https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:233: itemElement.addEventListener('mouseenter', () => { Perhaps add just one handler for the entire widget? https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:283: this._selectedItem = item; We'll still have selectedItemChanged() wired, will we? So perhaps live this one to it. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:285: this._list.selectItem(this._selectedItem, true); I think this is supposed to use item, not index. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:432: this._selectedItem = preservedSelection; perhaps just select() and let selectedItemChanged do the work? https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:583: mouseLeave(itemIndex) { Should we just have hoverItem instead of these two?
The CQ bit was unchecked by eostroukhov@chromium.org
Please take another look. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:51: this._hoveredItem = null; On 2017/03/28 23:35:54, caseq wrote: > nit: this._hoveredItemIndex Done. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:54: this._selectedItem = null; On 2017/03/28 23:35:54, caseq wrote: > ...Index as well? Done. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:233: itemElement.addEventListener('mouseenter', () => { On 2017/03/28 23:35:54, caseq wrote: > Perhaps add just one handler for the entire widget? Done. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:283: this._selectedItem = item; On 2017/03/28 23:35:54, caseq wrote: > We'll still have selectedItemChanged() wired, will we? So perhaps live this one > to it. This can be called before the list is initialized. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:285: this._list.selectItem(this._selectedItem, true); On 2017/03/28 23:35:54, caseq wrote: > I think this is supposed to use item, not index. As far as ListControl is concerned, FiltereListWidget's "item index" is the item. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:432: this._selectedItem = preservedSelection; On 2017/03/28 23:35:54, caseq wrote: > perhaps just select() and let selectedItemChanged do the work? replaceAllItems above resets the selection (it deletes all the items in ListControl and then readds them), hence the need to explicitly preserve it. https://codereview.chromium.org/2781863003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:583: mouseLeave(itemIndex) { On 2017/03/28 23:35:54, caseq wrote: > Should we just have hoverItem instead of these two? Done.
Code updated, please take another look.
https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:40: this._itemElementsContainer.addEventListener('mousemove', event => this._updateHover(event)); You usually need one for mouseout as well. https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:164: if (!listItemElement) should't we properly remove hover in this case? https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:167: if (item === null || item === this._hoveredItemIndex) ditto -- perhaps remove item === null clause and let the below code do its job?
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Updated, please take another look. https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:40: this._itemElementsContainer.addEventListener('mousemove', event => this._updateHover(event)); On 2017/03/29 00:13:22, caseq wrote: > You usually need one for mouseout as well. Done. https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:164: if (!listItemElement) On 2017/03/29 00:13:22, caseq wrote: > should't we properly remove hover in this case? Done. https://codereview.chromium.org/2781863003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:167: if (item === null || item === this._hoveredItemIndex) On 2017/03/29 00:13:22, caseq wrote: > ditto -- perhaps remove item === null clause and let the below code do its job? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2781863003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:41: this._itemElementsContainer.addEventListener('mouseout', () => this._removeHover()); With a properly written updateHover() you shouldn't need anything special for "mouseout". https://codereview.chromium.org/2781863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:166: if (!listItemElement) So if mouse does not point to an element of the list, presumably we should clear hover from the previously hovered element?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for the review. I updated the CL, please take another look. https://codereview.chromium.org/2781863003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:41: this._itemElementsContainer.addEventListener('mouseout', () => this._removeHover()); On 2017/03/29 00:46:21, caseq wrote: > With a properly written updateHover() you shouldn't need anything special for > "mouseout". Done. https://codereview.chromium.org/2781863003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:166: if (!listItemElement) On 2017/03/29 00:46:21, caseq wrote: > So if mouse does not point to an element of the list, presumably we should clear > hover from the previously hovered element? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2781863003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:457: if (this._selectedItemIndex !== null) { nit: drop !== null
Thank you for the review. https://codereview.chromium.org/2781863003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2781863003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:457: if (this._selectedItemIndex !== null) { On 2017/03/30 16:26:47, caseq wrote: > nit: drop !== null I can't - it is zero based... But it is not necessarily sorted in ascension order so relying on it being the top item also will not work.
The CQ bit was checked by eostroukhov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490892678824860,
"parent_rev": "960575f6f8da2a8557c04adbeccc045cd51034b1", "commit_rev":
"76c041dbcc4de828135d0af8c39256c4861b1a01"}
Message was sent while issue was closed.
Description was changed from
==========
[DevTools] Filtered list enhancements
Some enhancements that are needed for the frames menu but also seem
useful for other clients of the FilteredListWidget.
1. Selection is preserved while the filter string is being updated.
2. Items are highlighted on hover. Provider is notified when mouse
moves over the item.
3. It is now possible to set the selection.
BUG=698027
==========
to
==========
[DevTools] Filtered list enhancements
Some enhancements that are needed for the frames menu but also seem
useful for other clients of the FilteredListWidget.
1. Selection is preserved while the filter string is being updated.
2. Items are highlighted on hover. Provider is notified when mouse
moves over the item.
3. It is now possible to set the selection.
BUG=698027
Review-Url: https://codereview.chromium.org/2781863003
Cr-Commit-Position: refs/heads/master@{#460799}
Committed:
https://chromium.googlesource.com/chromium/src/+/76c041dbcc4de828135d0af8c392...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/76c041dbcc4de828135d0af8c392...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2793483002/ by pfeldman@chromium.org. The reason for reverting is: Breaks selection, don't think was necessary at first place!. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
