From 3077ef11690d8e947247f6fa690be58f23e506d9 Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Wed, 21 Feb 2024 08:59:21 +0000 Subject: [PATCH] Bug 1878698 - [devtools] Avoid highlighting the location on cursor changes. r=devtools-reviewers,nchevobbe This allows select text without having the highlighting going in the way. It also prevents flashing the line when clicking around in the editor. But we still update the location in the reducer in order to update the footer and outline panel accordingly to the new cursor location. Differential Revision: https://phabricator.services.mozilla.com/D200726 --- .../debugger/src/actions/sources/select.js | 16 +++++++++++++--- .../src/components/Editor/HighlightLine.js | 15 ++++++++++----- .../debugger/src/components/Editor/index.js | 3 +++ devtools/client/debugger/src/reducers/sources.js | 8 ++++++++ .../client/debugger/src/selectors/sources.js | 4 ++++ .../test/mochitest/browser_dbg-editor-select.js | 8 ++++++++ 6 files changed, 46 insertions(+), 8 deletions(-) diff --git a/devtools/client/debugger/src/actions/sources/select.js b/devtools/client/debugger/src/actions/sources/select.js index abdee38d1398a..f25267374b692 100644 --- a/devtools/client/debugger/src/actions/sources/select.js +++ b/devtools/client/debugger/src/actions/sources/select.js @@ -41,11 +41,13 @@ import { // This is only used by jest tests (and within this module) export const setSelectedLocation = ( location, - shouldSelectOriginalLocation + shouldSelectOriginalLocation, + shouldHighlightSelectedLocation ) => ({ type: "SET_SELECTED_LOCATION", location, shouldSelectOriginalLocation, + shouldHighlightSelectedLocation, }); // This is only used by jest tests (and within this module) @@ -192,8 +194,14 @@ async function mayBeSelectMappedSource(location, keepContext, thunkArgs) { * If false, this will ignore the currently selected source * and select the generated or original location, even if we * were currently selecting the other source type. + * @param {boolean} options.highlight + * True by default. To be set to false in order to preveng highlighting the selected location in the editor. + * We will only show the location, but do not put a special background on the line. */ -export function selectLocation(location, { keepContext = true } = {}) { +export function selectLocation( + location, + { keepContext = true, highlight = true } = {} +) { return async thunkArgs => { const { dispatch, getState, client } = thunkArgs; @@ -245,7 +253,9 @@ export function selectLocation(location, { keepContext = true } = {}) { dispatch(addTab(source, sourceActor)); } - dispatch(setSelectedLocation(location, shouldSelectOriginalLocation)); + dispatch( + setSelectedLocation(location, shouldSelectOriginalLocation, highlight) + ); await dispatch(loadSourceText(source, sourceActor)); diff --git a/devtools/client/debugger/src/components/Editor/HighlightLine.js b/devtools/client/debugger/src/components/Editor/HighlightLine.js index 841193fbb68b9..8639128905900 100644 --- a/devtools/client/debugger/src/components/Editor/HighlightLine.js +++ b/devtools/client/debugger/src/components/Editor/HighlightLine.js @@ -18,6 +18,7 @@ import { getSelectedSourceTextContent, getPauseCommand, getCurrentThread, + getShouldHighlightSelectedLocation, } from "../../selectors/index"; function isDebugLine(selectedFrame, selectedLocation) { @@ -95,6 +96,7 @@ export class HighlightLine extends Component { selectedLocation, selectedFrame, selectedSourceTextContent, + shouldHighlightSelectedLocation, } = this.props; if (pauseCommand) { this.isStepping = true; @@ -107,11 +109,13 @@ export class HighlightLine extends Component { prevProps.selectedSourceTextContent ); } - this.setHighlightLine( - selectedLocation, - selectedFrame, - selectedSourceTextContent - ); + if (shouldHighlightSelectedLocation) { + this.setHighlightLine( + selectedLocation, + selectedFrame, + selectedSourceTextContent + ); + } endOperation(); } @@ -182,6 +186,7 @@ export default connect(state => { } return { pauseCommand: getPauseCommand(state, getCurrentThread(state)), + shouldHighlightSelectedLocation: getShouldHighlightSelectedLocation(state), selectedFrame: getVisibleSelectedFrame(state), selectedLocation, selectedSourceTextContent: getSelectedSourceTextContent(state), diff --git a/devtools/client/debugger/src/components/Editor/index.js b/devtools/client/debugger/src/components/Editor/index.js index 2336b9401da29..c659de77d2c64 100644 --- a/devtools/client/debugger/src/components/Editor/index.js +++ b/devtools/client/debugger/src/components/Editor/index.js @@ -453,6 +453,9 @@ class Editor extends PureComponent { // Reset the context, so that we don't switch to original // while moving the cursor within a bundle keepContext: false, + + // Avoid highlighting the selected line + highlight: false, } ); }; diff --git a/devtools/client/debugger/src/reducers/sources.js b/devtools/client/debugger/src/reducers/sources.js index 8a86229c7fb55..6d90fd5c9111c 100644 --- a/devtools/client/debugger/src/reducers/sources.js +++ b/devtools/client/debugger/src/reducers/sources.js @@ -109,6 +109,13 @@ export function initialSourcesState(state) { */ selectedOriginalLocation: UNDEFINED_LOCATION, + /** + * By default, the `selectedLocation` should be highlighted in the editor with a special background. + * On demand, this flag can be set to false in order to prevent this. + * The location will be shown, but not highlighted. + */ + shouldHighlightSelectedLocation: true, + /** * By default, if we have a source-mapped source, we would automatically try * to select and show the content of the original source. But, if we explicitly @@ -149,6 +156,7 @@ function update(state = initialSourcesState(), action) { selectedOriginalLocation: UNDEFINED_LOCATION, pendingSelectedLocation, shouldSelectOriginalLocation: action.shouldSelectOriginalLocation, + shouldHighlightSelectedLocation: action.shouldHighlightSelectedLocation, }; } diff --git a/devtools/client/debugger/src/selectors/sources.js b/devtools/client/debugger/src/selectors/sources.js index cf524d507c727..6a67cf1a328c6 100644 --- a/devtools/client/debugger/src/selectors/sources.js +++ b/devtools/client/debugger/src/selectors/sources.js @@ -194,6 +194,10 @@ export function getShouldSelectOriginalLocation(state) { return state.sources.shouldSelectOriginalLocation; } +export function getShouldHighlightSelectedLocation(state) { + return state.sources.shouldHighlightSelectedLocation; +} + /** * Gets the first source actor for the source and/or thread * provided. diff --git a/devtools/client/debugger/test/mochitest/browser_dbg-editor-select.js b/devtools/client/debugger/test/mochitest/browser_dbg-editor-select.js index d9dd9982fd981..85467d2347fb1 100644 --- a/devtools/client/debugger/test/mochitest/browser_dbg-editor-select.js +++ b/devtools/client/debugger/test/mochitest/browser_dbg-editor-select.js @@ -28,6 +28,8 @@ add_task(async function () { 2, "when passing an explicit line number, the position is displayed" ); + assertHighlightLocation(dbg, "simple1.js", 1); + // Note that CodeMirror is 0-based while the footer displays 1-based getCM(dbg).setCursor({ line: 1, ch: 0 }); await waitForCursorPosition(dbg, 2); @@ -37,6 +39,11 @@ add_task(async function () { 1, "when moving the cursor, the position footer updates" ); + ok( + !findElement(dbg, "highlightLine"), + "Moving the cursor resets the highlighted line" + ); + getCM(dbg).setCursor({ line: 2, ch: 0 }); await waitForCursorPosition(dbg, 3); assertCursorPosition( @@ -56,6 +63,7 @@ add_task(async function () { 16, "when moving the cursor a second time, the position footer still updates" ); + assertHighlightLocation(dbg, "simple1.js", 4); info("Call the function that we set a breakpoint in."); invokeInTab("main");