Closed Bug 1046049 Opened 8 years ago Closed 7 years ago

[project editor] in Vim mode, `:w` doesn't save the file

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
All
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: paul, Assigned: mitchfriedman5, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

No description provided.
Looks like we may need to define a save command for the keymap to work as expected: https://github.com/marijnh/CodeMirror/blob/41bd14d66caf5ae948234fd2f9b0c6dbc02413fa/keymap/vim.js#L4283
Right, the Vim demo does:

CodeMirror.commands.save = function(){ alert("Saving"); };

which is triggered correctly.
Mentor: jryans
Whiteboard: [good first bug][lang=js]
hi. I would like to work on this .
Could you please assign it to me ?
Sorry for the long delay!  In the future, it's best to use the "needinfo" feature below set to "mentor" to ensure a fast reply.

To get started with the code base, check our hacking[1] page.  I'll assign the bug once a first patch is attached.

If you have specific questions, you can use needinfo in this bug, or ask in #devtools on IRC.

[1]: https://wiki.mozilla.org/DevTools/Hacking
tracking-win: --- → ?
tracking-win: ? → ---
Attached patch Fix saving in vim mode with :w (obsolete) — Splinter Review
So this will fix the actual bug, which is saving with the :w command in vim mode, but I believe it introduces a new one.  It appears there is a circular save event being emitted, starting immediately after the first save, if the line that I commented out in projecteditor/lib/editors.js/save().  

I believe it's because it emits the save event in that method, but at the same time that's necessary so that the webide components will pick it up and update the project, etc.  A solution might be to rename some of the save events, maybe editors.js should emit a "cmd-save" event to be consistent with the way the "File > Save" button would handle it (I think it emiits a cmd-save, correct me if I am wrong).


So as of this patch it can save files, but I commented out that emit, looking for some guidance on how to approach the circular event emission.
Attachment #8649295 - Flags: review?(jryans)
Comment on attachment 8649295 [details] [diff] [review]
Fix saving in vim mode with :w

Review of attachment 8649295 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this!

The current `save` event means "saved completed", and I think it's best to preserve that semantic as-is.

So, let's create a new `saveRequested` event in response to the editor.  That should avoid any double event issues.

::: browser/devtools/projecteditor/lib/editors.js
@@ +180,5 @@
>      this.editor.on("focus", (...args) => {
>        this.emit("focus", ...args);
>      });
> +    this.editor.on("save", (...args) => {
> +      this.emit("save", ...args);

Over here, let's emit a new `saveRequested` event.

You should also update `_onEditorCreated` in projecteditor.js to dispatch this event to plugins as an `onEditorSaveRequested` method.

@@ +235,5 @@
>    save: function(resource) {
>      let newText = this.editor.getText();
>      return resource.save(newText).then(() => {
>        this._savedResourceContents = newText;
> +      //this.emit("save", resource);

With the new event in place, you can revert this change.

::: browser/devtools/sourceeditor/editor.js
@@ +284,5 @@
>        scssSpec.colorKeywords = cssColors;
>        scssSpec.valueKeywords = cssValues;
>        win.CodeMirror.defineMIME("text/x-scss", scssSpec);
>  
> +      win.CodeMirror.commands.save = () => {

I think it's safe to leave this block alone, unless this was something ESLint was warning about.
Attachment #8649295 - Flags: review?(jryans)
Thanks for getting back to me with some feedback.  This has been a nasty bug trying to follow all the javascript events and who is picking up on each of them.


I used some of the feedback you gave me and I created another patch, this one works fully.  The only thing I don't really like about my solution is that I have two event listeners for "saving" now in projecteditor.js.  I have one for the saveRequest, which comes from editors.js (which comes from CodeMirror). This is picked up by the save.js plugin, which then goes around to emit the `save` completed event.

What follows is that I have another listener there for `save`, which as you said, is save completed, which emits an `onEditorSave` which is picked up by the webide. 

I found a need to separate these two events as one needs to go to save.js plugin and actually write the saved file to disk, the other needs to go to webide.js and be used in updating the project information, so there seems to be a clear separation of concerns. 


Let me know what you think of this solution, and if you think of a way to not have two mapped listeners in projecteditor that would be cool. 

Thanks :)
Attachment #8650085 - Flags: review?(jryans)
Comment on attachment 8650085 [details] [diff] [review]
Use code review suggestions to create another patch

Review of attachment 8650085 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, the project editor is confusing with so many modules and plugins and events.  It looks like you're making sense of it though, good work! :D

Seems like you are almost there.  Have you been able to run tests to see if they are passing?  Check the Hacking page[1] about this.  

We'll need to run your patch through the Try server (our testing system) before landing as well.  I can help with that if you don't have access yet.  You are welcome to apply for access too[2].

[1]: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests
[2]: https://www.mozilla.org/en-US/about/governance/policies/commit/

::: browser/devtools/projecteditor/lib/editors.js
@@ +235,5 @@
>    save: function(resource) {
>      let newText = this.editor.getText();
>      return resource.save(newText).then(() => {
>        this._savedResourceContents = newText;
> +      this.emit("save", resource);

Seems like this shouldn't be in the patch, since you're just reverting back to what already exists.

::: browser/devtools/sourceeditor/editor.js
@@ +284,5 @@
>        scssSpec.colorKeywords = cssColors;
>        scssSpec.valueKeywords = cssValues;
>        win.CodeMirror.defineMIME("text/x-scss", scssSpec);
>  
> +      win.CodeMirror.commands.save = () => this.emit("saveRequested");

This files in the `sourceeditor` module affect all users of our editor component, so scratchpad, debugger, project editor, style editor, etc.

So, it's fine to change this to `saveRequested` here, but we'll need to find other "save" listeners and update them.  Searching DXR[1] should help find what's needed.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=%28%22save%22+path%3Abrowser%2Fdevtools&redirect=false&case=true&limit=78&offset=0
Attachment #8650085 - Flags: review?(jryans) → feedback+
Thanks for the feedback.  This patch contains some of those fixes as well as removing the reverted changes introduced in the previous patch. Let me know if this patch needs some more work, I believe it should be sufficient.

Thanks!
Attachment #8650644 - Flags: review?(jryans)
Comment on attachment 8650644 [details] [diff] [review]
Update with code review fixes

Review of attachment 8650644 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this looks good to me!

I'll push this to try in a moment.
Attachment #8650644 - Flags: review?(jryans) → review+
https://hg.mozilla.org/mozilla-central/rev/db828e0df6ef
Assignee: nobody → mitchfriedman5
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.