Closed Bug 1519315 Opened 8 months ago Closed 2 months ago

Display CodeMirror's gutter in console input when devtools.webconsole.input.editor is true

Categories

(DevTools :: Console, task, P1)

65 Branch
task

Tracking

(firefox69 fixed)

RESOLVED FIXED
Firefox 69
Tracking Status
firefox69 --- fixed

People

(Reporter: nchevobbe, Assigned: transfusion, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [console-editor-mvp])

Attachments

(1 file, 1 obsolete file)

No description provided.

Hi Nicolas, I'd love to take on this bug (thanks for the suggestion)! I'll get started on setting up the work environment today and read up on CodeMirror to get some background knowledge before diving into the code :)

Hello George, I assigned the bug to you.
Don't hesitate to ask questions here or on Slack :)

Assignee: nobody → georgema86
Status: NEW → ASSIGNED

Hi Nicolas, first off sorry for the delay but had a chance to finally finish setting up my work environment tonight (had to use a VM), and began looking over the devtools code related to this bug. I have a couple of questions that I wanted to clarify with the task at hand if that's alright-

When you refer to console input, are you referring to this? And in that case, is my task to implement a way to display the CodeMirror gutters in a fashion similar to this video? (not mine)

Thanks in advance!
-George

Hello George,

What we want to achieve here is the same thing than in the debugger or in the style editor, i.e. having the line number on the left.

In the devtools, we are using a shared component to wrap codeMirror, which we call the source editor (devtools/client/shared/sourceeditor/editor.js).

And by looking at the code there, you might get a sense of how it's working.
Also, this bug has already a patch attached that you can have a look at :) https://phabricator.services.mozilla.com/D17099

Hi Nicolas, thanks for directing me to that shared component! I was able to find this line that helped me make sense of the code in the patch.

if (this.config.lineNumbers && !this.config.gutters.includes("CodeMirror-linenumbers")) {
	this.config.gutters.push("CodeMirror-linenumbers");
}

On my end, I got the console to show but only when setting in the Editor constructor, lineNumbers=true.

I tried the approach of adding an editorMode: PropTypes.bool field to propTypes() and setting lineNumbers=this.props instead, but realized that the reason this does not work is because I am not setting the field anywhere.

I will take a further look into where I can 1. check devtools.webconsole.input.editor=true, and then 2. set the lineNumbers field, tomorrow morning.

(any advice is welcome, especially if I’m off track completely)
Thanks! :)

-George

Hello George, I think you're on the right track!
So, to get the editorMode prop in the JsTerm component, you need to pass it from the parent components.
I had a patch where I did this, https://phabricator.services.mozilla.com/D17096.
You can try to take some inspiration from there (only to retrieve and pipe down the editorMode prop).

Hi Nicolas!

I’ve taken a look over that patch you sent(D17096), and I’ve tried to add some of the lines regarding editorMode similar to that of your App.js file.

In mapStateToProps(state) of JSTerm.js, I’ve added the line the following line:

// JSTerm.js

// Redux connect
function mapStateToProps(state){
    //...
    editorMode: state.ui.editor,
};

and looking further into what a redux connect statement is, (connects a React component to a Redux store), I added the corresponding line to the UiState of store.js, and to constants.js, similar to the code you added in that patch.

// store.js

ui: uiState({
    //...
	editor: getBoolPref(PREFS.UI.EDITOR),
}),
// constants.js

UI: {
    //...
    EDITOR: "devtools.webconsole.input.editor",
},

However, this approach did not display the lines in the gutter as needed, and I’m currently a little stumped (primarily due to my lack of react knowledge).

My current idea of what I could be missing is that I may need to add something to the render() function of JSTerm.js, as I’ve looked over the patch and noticed that in both theEditorToolbar.js and App.js file of the patch, you manipulate the editorMode variable (but do not explicitly set it).

Hopefully I conveyed my issue in a way that you can understand, but essentially I just need some guidance to what I’m missing regarding my attempt to pass down the editorMode prop from the parent component.

Thanks! :)

-George

Thanks a lot George, looks like you're on the right track!
Could you push your commit to Phabricator so I can see what you might be missing? You can read https://docs.firefox-dev.tools/contributing/code-reviews-setup.html if you did not set it up yet (make sure to use moz-phab as it makes things easier).

Attachment #9037903 - Attachment is obsolete: true

Hello George, how are you doing?

It looks like Bug 1519313 will land soon, and it has all the piping to get the editorMode prop into the JSTerm, so that's a part you don't have to do :)

If you want to test it right away, before it's on mozilla-central, you can do

hg import https://hg.mozilla.org/integration/autoland/raw-rev/90de1c90ae69

Then you can rebase your work on top of that and see if the gutter is here when in editor mode :)

Hi Nicolas! I checked out the bug and it looks like I was indeed on the right track, glad to know that my discussion with you most likely helped out Sola with the fixes! I believe the part I was missing on my end was the step where I had to run ./mach run with the necessary flags: ./mach run --devtools --setpref devtools.webconsole.input.editor=true. Running it with these flags and making a few small changes to get my codebase looking exactly like Sola's fix (only really had to add a few lines to App.js), makes the gutters show as intended now!

I rebased the code directly from Sola's bug as I couldn't get that hg import statement working, (the only line that needs to be added is the line found on this bug commit), and the gutters now seem to be working as intended. Let me know if this added line to his commit works for you, and thanks for all the help!

Okay, this is perfect!
So now, what we want is a test to make sure we do show the gutter when in editor mode.
Again, you can check Sola's patch where he added a test as well.
You probably don't need to execute anything, but you will have to check if the gutter element is displayed. Let me know if something isn't clear enough!

Hello George, how is it going on? Is anything blocking you?

Flags: needinfo?(georgema86)

clearing assignee

Assignee: georgema86 → nobody
Mentor: nchevobbe
Status: ASSIGNED → NEW
Type: defect → task
Flags: needinfo?(georgema86)
Keywords: good-first-bug

Hello Bryan,

Would you be interested working on this bug? It's about the same feature you worked already :)

Flags: needinfo?(bryan.wyern1)

Assigning to Bryan as discussed on Slack

Assignee: nobody → bryan.wyern1
Status: NEW → ASSIGNED
Flags: needinfo?(bryan.wyern1)
Priority: P2 → P1
Whiteboard: [console-editor-mvp]

Hello Bryan, how is this going?

Flags: needinfo?(nchevobbe)
Flags: needinfo?(nchevobbe) → needinfo?(bryan.wyern1)
Attachment #9070320 - Attachment description: Bug 1519315 - Display CodeMirror's gutter in console input when devtools.webconsole.input.editor is true r=nchevobbe → Bug 1519315 - Display CodeMirror's gutter in console input when devtools.webconsole.input.editor is true. r=nchevobbe.

Hi, really sorry about falling off the radar. Thanks for pushing it over the finish line!

Flags: needinfo?(bryan.wyern1)

sure! thanks for the patch you wrote! You'll still be credited as the author of the commit, so you can say you contributed to Firefox :)

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/938ba43a3350
Display CodeMirror's gutter in console input when devtools.webconsole.input.editor is true. r=Honza.
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
You need to log in before you can comment on or make changes to this bug.