Closed Bug 937607 Opened 6 years ago Closed 6 years ago

Manifest Editor erases local modifications

Categories

(DevTools :: WebIDE, defect)

x86
All
defect
Not set

Tracking

(firefox27 verified, firefox28 verified)

VERIFIED FIXED
Firefox 28
Tracking Status
firefox27 --- verified
firefox28 --- verified

People

(Reporter: paul, Assigned: jryans)

References

Details

Attachments

(2 files, 1 obsolete file)

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?
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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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
Attachment #831032 - Flags: review?(paul)
* 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
Attachment #831032 - Attachment is obsolete: true
Attachment #831032 - Flags: review?(paul)
Attachment #831711 - Flags: review?(paul)
Attachment #831711 - Flags: review?(paul) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8653cd5f7b38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Duplicate of this bug: 942180
Attached patch Patch for AuroraSplinter Review
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.
Attachment #8337490 - Flags: review?(paul)
Attachment #8337490 - Flags: review?(paul) → review+
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
Attachment #8337490 - Flags: approval-mozilla-aurora?
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 .
Attachment #8337490 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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?
(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.
(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.
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.
Status: RESOLVED → VERIFIED
Removing the keyword given comment 15.
Keywords: verifyme
(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.
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.