1070719 - Auto save all files on press "Play"

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[André Fiedler]

If I press the "Play" button, all unsaved files should be saved before they get pushed to the device.

Comment 1

[Paul Rouget [:paul]]

I think that makes sense. I keep forgetting about saving my files.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Jen, this bug may be a good way to learn about the editor portion of WebIDE.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I don't believe Jen is currently working on this.

Comment 4

[Rocik]

I have made such a tweak for myself already. Can someone assign me to this bug while I prepare the patch? Thanks.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to rrocik from comment #4)
> I have made such a tweak for myself already. Can someone assign me to this
> bug while I prepare the patch? Thanks.

Typically for new contributors, we'll wait for the first patch to be attached before assigning.  Feel free to begin working on it!

Comment 6

[Rocik]

Attachment: bug1070719_autosavefiles.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) 
> Typically for new contributors, we'll wait for the first patch to be
> attached before assigning.  Feel free to begin working on it!

Oh I didn't know it works that way. I must have overlooked something.

There is a checkbox in properies. The other thing is "validateAndUpdateProject" will be called as many times as there are files to save, it sounds like a small performance flaw.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Rocik from comment #6)
> Created attachment 8651125 [details] [diff] [review]
> bug1070719_autosavefiles.patch
> 
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) 
> > Typically for new contributors, we'll wait for the first patch to be
> > attached before assigning.  Feel free to begin working on it!
> 
> Oh I didn't know it works that way. I must have overlooked something.

I think this is just a recent quirk that may not be written down, so no worries!  Also, it may be different per team, so that makes it extra confusing.  Anyway, I've just updated the "How to Submit a Patch" page[1] to note that some teams do this.

I'll review your patch soon!

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Preparation

Comment 8

[Rocik]

I noticed my editor used tabs instead of spaces, also there is no longer needed if statement. Should I submit a new patch?

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Rocik from comment #8)
> I noticed my editor used tabs instead of spaces, also there is no longer
> needed if statement. Should I submit a new patch?

Yes, please do!  You can mark the current one as "obsolete" when uploading, and request a new review on the new patch.

Comment 10

[Rocik]

Attachment: bug1070719_autosavefiles.patch

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8651914 [details] [diff] [review]
bug1070719_autosavefiles.patch

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

Looks like your patch commit message has a typo "Autsave".  Also, please add "r=jryans" to the end of the message.

About "validateAndUpdateProject", you're right it would be best to skip doing that for each file.  For the moment, I would guess it's not a big issue, as you're unlikely to have a large set of edited files.

In my testing, there was a larger issue though.  Here's what I saw:

1. Edit file A
2. Edit file B
3. Press play
4. Contents of file A is saved to both A and B

Are you able to reproduce this?

::: browser/devtools/projecteditor/lib/projecteditor.js
@@ +760,5 @@
>      }
>  
>      return true;
> +  },
> +  

Nit: Trailing whitespace, you may need to change some editor settings to remove it

::: browser/devtools/webide/content/webide.js
@@ +1174,5 @@
>      let busy;
>      switch(AppManager.selectedProject.type) {
>        case "packaged":
> +      	
> +      	let autosave = Services.prefs.getBoolPref("devtools.webide.autosavefiles");

Nit: There might be tabs here

@@ +1175,5 @@
>      switch(AppManager.selectedProject.type) {
>        case "packaged":
> +      	
> +      	let autosave = Services.prefs.getBoolPref("devtools.webide.autosavefiles");
> +      	if (autosave && UI.projecteditor) {

Nit: There might be tabs here

::: browser/devtools/webide/webide-prefs.js
@@ +32,5 @@
>  #endif
>  pref("devtools.webide.zoom", "1");
>  pref("devtools.webide.busyTimeout", 10000);
>  pref("devtools.webide.sidebars", false);
> +pref("devtools.webide.autosavefiles", false);

Do you think most users would want this feature on by default?

Nit: We seem to be using variable-style casing in pref names so, let's change "autosavefiles" -> "autosaveFiles"

::: browser/locales/en-US/chrome/browser/devtools/webide.dtd
@@ +136,5 @@
>  <!ENTITY prefs_options_autoclosebrackets_tooltip "Automatically insert closing brackets">
>  <!ENTITY prefs_options_keybindings "Keybindings">
>  <!ENTITY prefs_options_keybindings_default "Default">
> +<!ENTITY prefs_options_autosavefiles "Autosave files">
> +<!ENTITY prefs_options_autosavefiles_tooltip "Automatically save progress before running project">

Nit: Let's change "progress" to "edited files"

Comment 12

[Rocik]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> In my testing, there was a larger issue though.  Here's what I saw:
> 
> 1. Edit file A
> 2. Edit file B
> 3. Press play
> 4. Contents of file A is saved to both A and B
> 
> Are you able to reproduce this?

I had such issues without: let editor = this.editorFor(resource);
It worked fine, and still is (checked with latest changes).
I'm using the fx-team repo so I think our builds should be the same, am I wrong?
Maybe you took some other steps I am not checking.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Rocik from comment #12)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> > In my testing, there was a larger issue though.  Here's what I saw:
> > 
> > 1. Edit file A
> > 2. Edit file B
> > 3. Press play
> > 4. Contents of file A is saved to both A and B
> > 
> > Are you able to reproduce this?
> 
> I had such issues without: let editor = this.editorFor(resource);
> It worked fine, and still is (checked with latest changes).
> I'm using the fx-team repo so I think our builds should be the same, am I
> wrong?
> Maybe you took some other steps I am not checking.

Hmm, I should be seeing the same thing as you after applying the patch...

I'll leave a ni? for myself to investigate later.  If you can figure out details in the mean time, leave a comment!

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It may be a little while before I am able to investigate this more deeply.  Keeping ni? until then.

Comment 15

[Rocik]

I checked this in many various ways, whatever I could think of and it's still working. So take you time. If you'll get some clues why it does happen, let me know.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I spent some more time on this today, but still wasn't able to fix the problem.  I'll attach an updated patch and ask for some help.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: auto-save-files

:bgrins, I am hoping you can help me figure out what's wrong here.  Maybe it's something obvious.

This patch **almost** works for saving all unsaved files, but has a major flaw:

1. Edit file A
2. Edit file B
3. Press play
4. Contents of file A is saved to both A and B (verify file content from terminal, not WebIDE)

I thought that using a task to wait for file A to finish (even it doesn't seem like it should be required) would be enough to fix it, but the problem still occurs.  Before I added the tasks, sometimes I would see the concatenated context of A + B written to both files.

I started tracing down into OS.File, and it seems like things are happening as expected.  It's passing around the correct data for each file separately.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: auto-save-files

Updated to enable the saving feature by default.

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: projecteditor-saveall.patch

I couldn't reproduce the issue in Comment 17 either when running in WebIDE or in a test case (which I've written and included in this patch)

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8669955 [details] [diff] [review]
auto-save-files

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

Does the test in Attachment 8670494 [details] [diff] fail for you?

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Does the test in Attachment 8670494 [details] [diff] fail for you?

The test passes... I finally figured out what was wrong.  It was specific to the app I happened to test.  It's a random test app full of junk that's accumulated.  And... out of the two files I would make changes to when testing, one was a symlink to the other! x_x

So, we should basically be good to go here.  I think I may revert my changes to serialize the file saving, since that will just make it slower for no reason.

Comment 22

[Rocik]

That's good to hear. If you'd like I can send the revised patch, but not until tomorrow. I have it on my second computer.

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Rocik from comment #22)
> That's good to hear. If you'd like I can send the revised patch, but not
> until tomorrow. I have it on my second computer.

It might be simpler at this stage to adapt from :bgrins patch, since he's added a test and such.  I'll make sure to set the author to you since you started the whole effort!

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Hmm, I think we do actually want the tasks in there, to ensure the files are saved before pushing the app.

So, I guess I'll update the author on the one with a test and mark it r+.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: auto-save-files

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff211d685c57

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ed5d727bb9f3

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Rocik, thanks again for working on this!  I'm sorry it took me so long to find out why it was behaving strangely for me.

If you're interested in working on some more bugs, check out http://firefox-dev.tools/ for some more that would be good to take a look at.

A sheriff will resolve this bug once it moves to mozilla-central.

Comment 28

[Rocik]

This site is great. Way better than the other ones, thanks for letting me know.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed5d727bb9f3