937607 - Manifest Editor erases local modifications

Status: VERIFIED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

STR:

- add an app
- with your own editor, change the manifest file
- click update
- your modifications are lost

I'm not sure what's the best strategy is.

Should make sure that the file hasn't changed before saving it? Should we only save the file on a user action? Should we handle conflicts? And which file do we push the device? The one showed in the manifest editor or the one from the disk?

Comment 1

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

I think the best approach is the one that will be least surprising to the user.  If they have made lots of edits to their manifest outside the App Manager, I can imagine they will be very sad and / or not even realize that "Update" currently overwrites that file with the contents of the Manifest Editor.

As we discussed on IRC, I'll add an explicit "Save Manifest" button for now.  Feel free to rework / refine this further in bug 919502.

Comment 2

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

Attachment: Add explicit save button for the manifest editor

This adds an explicit save button, so it is clearer when the manifest will be written to disk.

Additionally, the update button re-reads (and re-displays) the manifest from disk, so it will pull in any updates if you edit the manifest outside of the App Manager.

Try: https://tbpl.mozilla.org/?tree=Try&rev=5684b0fc904b

Comment 3

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

Attachment: Add explicit save button for the manifest editor v2

* Moved Save button into position based on Paul's button mockup
* Button is now hidden is manifest editor is disabled
* I assumed you would add "Reload" when implementing the mockup later, so it's not here currently

Try: https://tbpl.mozilla.org/?tree=Try&rev=a99b16e1c279

Comment 4

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

Try on Linux Debug kept coming back orange, but I can't imagine how this change would cause the errors reported there, so this seems ready to go.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8653cd5f7b38

Comment 8

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

Attachment: Patch for Aurora

As shown in bug 942180, the behavior is present in Aurora and confusing there.

The original patch here includes string changes, so this is a more minimal patch that disables the saving functionality when the editor is not shown.

Comment 9

[Paul Rouget [:paul]]

Comment on attachment 8337490 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature (manifest editor)
User impact if declined: manifest file is modified even if the user didn't change the code
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 10

[bhavana bajaj [:bajaj]]

Comment on attachment 8337490 [details] [diff] [review]
Patch for Aurora

Approving the patch without string changes on aurora at this time given the low risk here .

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a3e0cf45dbf

Comment 12

[Ton [:Tonnes]]

(In reply to :Ms2ger from comment #6)
> https://hg.mozilla.org/mozilla-central/rev/8653cd5f7b38
> 
> +<!ENTITY projects.saveManifestTooltip "Save the contents of the Manifest Editor below.">

Sorry to mention it here, but I spot a lot of tooltips (9) in the app-manager.dtd file ending with a period, while the policy for tooltips is not to use periods at all (as they're always infinitive). Maybe worth fixing it here?

Comment 13

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

(In reply to Ton from comment #12)
> (In reply to :Ms2ger from comment #6)
> > https://hg.mozilla.org/mozilla-central/rev/8653cd5f7b38
> > 
> > +<!ENTITY projects.saveManifestTooltip "Save the contents of the Manifest Editor below.">
> 
> Sorry to mention it here, but I spot a lot of tooltips (9) in the
> app-manager.dtd file ending with a period, while the policy for tooltips is
> not to use periods at all (as they're always infinitive). Maybe worth fixing
> it here?

File a new bug and we can update them, as it is not really related to this bug.

Also, is this documented on a page I can refer to somewhere?  I was not aware of this policy.

Comment 14

[Ton [:Tonnes]]

(In reply to J. Ryan Stinnett [:jryans] from comment #13)
> 
> File a new bug and we can update them, as it is not really related to this
> bug.
> 
> Also, is this documented on a page I can refer to somewhere?  I was not
> aware of this policy.

Clear, see bug 951132.

Comment 15

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed on latest Aurora 28.0a2 (20140116004003) under Win 7 32-bit, Ubuntu 32-bit and Mac OSX 10.9.

On Firefox 27 beta 7 (20140116125114) the Manifest Editor is not enabled (this feature is targeted for Firefox 28, see bug 925921). 
If the pref devtools.appmanager.manifestEditor.enabled is set to true, the Manifest Editor is visible in App Manager, but the Save button is not present and this issue is still reproducible.

Comment 16

[Ioana (away)]

Removing the keyword given comment 15.

Comment 17

[u279076]

(In reply to petruta.rasa from comment #15)
> Verified as fixed on latest Aurora 28.0a2 (20140116004003) under Win 7
> 32-bit, Ubuntu 32-bit and Mac OSX 10.9.
> 
> On Firefox 27 beta 7 (20140116125114) the Manifest Editor is not enabled
> (this feature is targeted for Firefox 28, see bug 925921). 
> If the pref devtools.appmanager.manifestEditor.enabled is set to true, the
> Manifest Editor is visible in App Manager, but the Save button is not
> present and this issue is still reproducible.

Ryan, can you please advise on this for Firefox 27? The current state of the builds would seem to contradict the value of the status-firefox27 flag.

Comment 18

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

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #17)
> (In reply to petruta.rasa from comment #15)
> > Verified as fixed on latest Aurora 28.0a2 (20140116004003) under Win 7
> > 32-bit, Ubuntu 32-bit and Mac OSX 10.9.
> > 
> > On Firefox 27 beta 7 (20140116125114) the Manifest Editor is not enabled
> > (this feature is targeted for Firefox 28, see bug 925921). 
> > If the pref devtools.appmanager.manifestEditor.enabled is set to true, the
> > Manifest Editor is visible in App Manager, but the Save button is not
> > present and this issue is still reproducible.
> 
> Ryan, can you please advise on this for Firefox 27? The current state of the
> builds would seem to contradict the value of the status-firefox27 flag.

The fix for Firefox 27 (as mentioned in comment 8) is more targeted than the one in Firefox 28, to avoid string changes.  Since the feature is hidden by default, the only change made was to stop the "Update" button from saving if the Manifest Editor is hidden.

So, in Firefox 27, only the following case is fixed:

1. Open App Manager (with Manifest Editor hidden)
2. Edit and save the manifest file in your text editor
3. Press "Update" in the App Manager
4. Your file in the text editor should not change

Prior to the fix for 27, the App Manager would replace your updated changes with its own copy, even though the manifest editor was not even visible.  Now, it only saves if the editor is visible.

Anyway, I've verified just now that this fix is still working as intended.

Comment 19

[u279076]

(In reply to J. Ryan Stinnett [:jryans] from comment #18)
> Anyway, I've verified just now that this fix is still working as intended.

Thanks for the detailed explanation and helping to verify this is fixed.