Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565113002:
DevTools: update console viewport scroll when prompt is resized (Closed)
|
Created:
4 years ago by luoe Modified:
3 years, 8 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: update console viewport scroll when prompt is resized
Console's viewport used to jump to bottom when prompt input happens. This misses
cases when prompt text changes after the 'input' event or the 'input' event
does not fire:
- Press 'Up' when not scrolled to bottom to load multiline text
- Start typing a multiline command from history and use autocomplete
Now, it will listen to a more accurate event from the text editor.
BUG=674374, 698428
Review-Url: https://codereview.chromium.org/2565113002
Cr-Commit-Position: refs/heads/master@{#461921}
Committed: https://chromium.googlesource.com/chromium/src/+/c1faecf64579d2d1a8525e775913fa7b6df43de6
Patch Set 1 #
Total comments: 4
Patch Set 2 : DevTools: console should update viewport when codemirror resizes #
Total comments: 3
Patch Set 3 : ac #Patch Set 4 : return to first approach #
Total comments: 2
Patch Set 5 : rebase #Patch Set 6 : update with events! #Patch Set 7 : update promptinput to promptContentChanged #
Total comments: 5
Patch Set 8 : SourcesTE listen to same event #
Total comments: 4
Patch Set 9 : yay #
Total comments: 5
Patch Set 10 : ac and move mute into SourceFrame #
Total comments: 6
Patch Set 11 : ac #Patch Set 12 : rebaseline #Messages
Total messages: 50 (24 generated)
The CQ bit was checked by luoe@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...
luoe@chromium.org changed reviewers: + einbinder@chromium.org
Looking for feedback on a new event EditorResized to tell the console to update its viewport. Ptal.
https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:35: var consoleView = Console.ConsoleView.instance(); Instead of finding a consoleView instance, can we do this.invalidateSize(); It will call doResize in ConsoleView. https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1027: this.widget().dispatchEventToListeners(UI.TextEditor.Events.EditorResized); invalidateSize again here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:35: var consoleView = Console.ConsoleView.instance(); On 2016/12/12 20:35:20, einbinder wrote: > Instead of finding a consoleView instance, can we do this.invalidateSize(); > It will call doResize in ConsoleView. Done, except it will call onLayout in ConsoleView. https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1027: this.widget().dispatchEventToListeners(UI.TextEditor.Events.EditorResized); On 2016/12/12 20:35:20, einbinder wrote: > invalidateSize again here? Done.
lgtm https://codereview.chromium.org/2565113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1027: this.widget().invalidateSize(); this.widget() actually returns this. this.invalidateSize();
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2565113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:50: this.invalidateSize(); What is the semantics of this code? When do you get layed out and how can you request re-layout upon that signal?
Description was changed from ========== DevTools: update console viewport scroll when prompt is resized BUG=667072 ========== to ========== DevTools: update console viewport scroll when prompt is resized When the user changes the height of the console prompt via pressing 'Up' for the previous command, stick to bottom logic should be invoked. This CL makes the console viewport update in these cases, since it currently listens to updates in the main message area excluding the prompt. BUG=667072 ==========
https://codereview.chromium.org/2565113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:50: this.invalidateSize(); On 2016/12/13 20:06:39, pfeldman wrote: > What is the semantics of this code? When do you get layed out and how can you > request re-layout upon that signal? Semantics are as follows: user changes the height of the prompt by pressing up to load multiline text from history. CodeMirrorTextEditor (CMTE)'s "changes" event fires, and invalidateSize() triggers a chain of onLayout's: CMTE > ConsolePrompt > SearchableView > ConsoleView Only CMTE knows when it resizes itself and only ConsoleView knows how to update its viewport. Setting up this chain is one way to request the layout. What this really is is a way to say: 'when the viewport's scrollHeight changes, update the viewport' without checking differences in scrollHeight.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2565113002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:35: var consoleView = Console.ConsoleView.instance(); This looks bad - ConsolePrompt should not reach into higher level component ConsoleView. https://codereview.chromium.org/2565113002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2565113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:422: scheduleViewportRefreshForTest(muted) { Why made this public? It's not intended to be used by anyone except sniffers.
Description was changed from ========== DevTools: update console viewport scroll when prompt is resized When the user changes the height of the console prompt via pressing 'Up' for the previous command, stick to bottom logic should be invoked. This CL makes the console viewport update in these cases, since it currently listens to updates in the main message area excluding the prompt. BUG=667072 ========== to ========== DevTools: update console viewport scroll when prompt is resized When the user changes the height of the console prompt via selecting a multiline autocomplete suggestion, stick to bottom logic should be invoked. Currently it listens to updates in the main message area excluding the prompt. BUG=674374 ==========
Description was changed from ========== DevTools: update console viewport scroll when prompt is resized When the user changes the height of the console prompt via selecting a multiline autocomplete suggestion, stick to bottom logic should be invoked. Currently it listens to updates in the main message area excluding the prompt. BUG=674374 ========== to ========== DevTools: update console viewport scroll when prompt is resized When the user changes the height of the console prompt via selecting a multiline autocomplete suggestion, stick to bottom logic should be invoked. Currently it listens to updates in the main message area excluding the prompt. BUG=674374 ==========
ping
ptal
Description was changed from ========== DevTools: update console viewport scroll when prompt is resized When the user changes the height of the console prompt via selecting a multiline autocomplete suggestion, stick to bottom logic should be invoked. Currently it listens to updates in the main message area excluding the prompt. BUG=674374 ========== to ========== DevTools: update console viewport scroll when prompt is resized Console's viewport used to jump to bottom when prompt input happens. This misses cases when prompt text changes after the 'input' event or the 'input' event does not fire: - Press 'Up' when not scrolled to bottom to load multiline text - Start typing a multiline command from history and use autocomplete Now, it will listen to a more accurate event from the text editor. BUG=674374, 698428 ==========
Pleeeease take another look
On 2017/03/11 01:12:08, luoe wrote: > Pleeeease take another look @einbinder, @lushnikov specifically. The approach has changed from (Console refreshing when the prompt text editor resizes) to (jumping to bottom when text editor content changes).
https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1053: this.widget().emit(new UI.TextEditor.ContentChangedEvent()); this.widget() is just a hack for the interface. this.widget() === this https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/TextEditor.js (right): https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/TextEditor.js:77: UI.TextEditor.ContentChangedEvent = class {}; @lushnikov SourcesTextEditor is firing a TextChanged event, but it sends an event for every edit and builds TextRanges, so I feel ok with having this simple event here.
https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1053: this.widget().emit(new UI.TextEditor.ContentChangedEvent()); On 2017/03/13 18:08:18, einbinder wrote: > this.widget() is just a hack for the interface. this.widget() === this Let's move SourceFrame.SourcesTextEditor.Events.TextChanged to the TextEditor?
Made SourcesTextEditor listen to the ContentChanged event instead of the codeMirror 'changes' event. Please take another look https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1053: this.widget().emit(new UI.TextEditor.ContentChangedEvent()); On 2017/03/14 21:43:39, lushnikov wrote: > On 2017/03/13 18:08:18, einbinder wrote: > > this.widget() is just a hack for the interface. this.widget() === this > > Let's move SourceFrame.SourcesTextEditor.Events.TextChanged to the TextEditor? Done. https://codereview.chromium.org/2565113002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1053: this.widget().emit(new UI.TextEditor.ContentChangedEvent()); On 2017/03/13 18:08:18, einbinder wrote: > this.widget() is just a hack for the interface. this.widget() === this Done.
https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js:402: this.dispatchEventToListeners(SourceFrame.SourcesTextEditor.Events.TextChanged, edits[i]); can we move this method up to CMTE? https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/TextEditor.js (right): https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/TextEditor.js:77: UI.TextEditor.ContentChangedEvent = class { we should move the SourceFrame.SourcesTextEditor.Events.TextChanged here
ptal https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js:402: this.dispatchEventToListeners(SourceFrame.SourcesTextEditor.Events.TextChanged, edits[i]); Yes we can! https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/TextEditor.js (right): https://codereview.chromium.org/2565113002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/TextEditor.js:77: UI.TextEditor.ContentChangedEvent = class { On 2017/03/17 01:33:36, lushnikov wrote: > we should move the SourceFrame.SourcesTextEditor.Events.TextChanged here Done.
lgtm
The CQ bit was checked by luoe@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:35: this._editor.widget().on(UI.TextEditor.TextChangedEvent, this._onTextChanged, this); You don't know that the widget is editor, this callback should have been in the configureAutocomplete, right? https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js (right): https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:56: this._textEditor.on(UI.TextEditor.TextChangedEvent, event => this.onTextChanged(event.oldRange, event.newRange)); So I encapsulate this._textEditor, I call editRange on it and I get notified upon TextChangeEvents?
pfeldman@, please let me know if this looks good to you! https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:35: this._editor.widget().on(UI.TextEditor.TextChangedEvent, this._onTextChanged, this); On 2017/03/24 22:28:33, pfeldman wrote: > You don't know that the widget is editor, this callback should have been in the > configureAutocomplete, right? einbinder@, wdyt about adding a text changed handler into a configuration object? TextEditors are Widgets, but also an interface. Sounds like it was originally done to appease closure compiler. einbinder@ has a separate CL to make TextEditor an abstract class that extends widget: https://codereview.chromium.org/2778873003/ https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js (right): https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:56: this._textEditor.on(UI.TextEditor.TextChangedEvent, event => this.onTextChanged(event.oldRange, event.newRange)); On 2017/03/24 22:28:33, pfeldman wrote: > So I encapsulate this._textEditor, I call editRange on it and I get notified > upon TextChangeEvents? We want to redo the search highlight when text changes. SourceFrame's calls to editRange() are made on clicks to the 'Replace' or 'Replace All' buttons, but not regular user typing.
Thanks for helping me understand better how things work. I've moved muting up to SF. Please take another look! https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js (right): https://codereview.chromium.org/2565113002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsolePrompt.js:35: this._editor.widget().on(UI.TextEditor.TextChangedEvent, this._onTextChanged, this); Done, Updated TextEditor to extend Common.EventTarget. https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js (right): https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:61: this._muteChangeEventsForSetContent; AFAICT, the reason why we had muteTextChangedEvents at all was because we didn't want to do things like set working copy, update last modified time, etc when opening a file and calling setContent(). I think it makes more sense to move the mute logic from the low level CMTE up to SourceFrame, where the setContent() exists. Is wrapping the onTextChanged callback in an 'if' check too strange? After this patch, CodeMirror will always emit a UI.TextEditor.TextChanged event when it changes, and SourceFrame, the only one that cares about muting, will be the only place that knows about this muting logic at all.
Gentle ping
lgtm @ nits https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js (right): https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:61: this._muteChangeEventsForSetContent; Makes sense to me. https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js:308: this.emit(new UI.TextEditor.TextChangedEvent(range, newRange)); You should not emit text editor's events on sources text editor, it probably should go into the editRange on the super? https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1069: this.emit(new UI.TextEditor.TextChangedEvent(edits[i].oldRange, edits[i].newRange)); I believe dgozman@ removed the typed events.
Thanks for the reviews! https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js:308: this.emit(new UI.TextEditor.TextChangedEvent(range, newRange)); On 2017/04/04 20:21:09, pfeldman wrote: > You should not emit text editor's events on sources text editor, it probably > should go into the editRange on the super? Done. https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2565113002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1069: this.emit(new UI.TextEditor.TextChangedEvent(edits[i].oldRange, edits[i].newRange)); On 2017/04/04 20:21:09, pfeldman wrote: > I believe dgozman@ removed the typed events. Rebased.
The CQ bit was checked by luoe@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from einbinder@chromium.org, lushnikov@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2565113002/#ps220001 (title: "rebaseline")
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": 220001, "attempt_start_ts": 1491352491222810,
"parent_rev": "e624054d06931cb30017455519b924027d97320c", "commit_rev":
"c1faecf64579d2d1a8525e775913fa7b6df43de6"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: update console viewport scroll when prompt is resized Console's viewport used to jump to bottom when prompt input happens. This misses cases when prompt text changes after the 'input' event or the 'input' event does not fire: - Press 'Up' when not scrolled to bottom to load multiline text - Start typing a multiline command from history and use autocomplete Now, it will listen to a more accurate event from the text editor. BUG=674374, 698428 ========== to ========== DevTools: update console viewport scroll when prompt is resized Console's viewport used to jump to bottom when prompt input happens. This misses cases when prompt text changes after the 'input' event or the 'input' event does not fire: - Press 'Up' when not scrolled to bottom to load multiline text - Start typing a multiline command from history and use autocomplete Now, it will listen to a more accurate event from the text editor. BUG=674374, 698428 Review-Url: https://codereview.chromium.org/2565113002 Cr-Commit-Position: refs/heads/master@{#461921} Committed: https://chromium.googlesource.com/chromium/src/+/c1faecf64579d2d1a8525e775913... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/c1faecf64579d2d1a8525e775913... |
