Closed Bug 922193 Opened 11 years ago Closed 11 years ago

Integrate the VariablesView as a manifest editor

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files, 3 obsolete files)

      No description provided.
This is a rough (but functional) version of the manifest editor in the App Manager.  I'll attach a screenshot of the current appearance.

I have not attempted to style the editor at all.  That will be handled in bug 922144.

I left in the "save on update" functionality for now.  If you think it should wait until the buttons are re-done, or possibly a new button should be added, let me know.
Attachment #814725 - Flags: review?(paul)
Attached image Basic Appearance
Here's a screenshot of the current (unstyled) appearance.
* Improved test stability (was leaking data)
* Added pref to enable editor, on by default for Nightly only

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9cb55febe192
Attachment #814725 - Attachment is obsolete: true
Attachment #814725 - Flags: review?(paul)
Attachment #815131 - Flags: review?(paul)
* More test fixing

The new tests were passing locally for me, but then failing intermittently on try.  Should be good now!  Sorry for r? spam.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=054c4eb26dd2
Attachment #815131 - Attachment is obsolete: true
Attachment #815131 - Flags: review?(paul)
Attachment #815764 - Flags: review?(paul)
Comment on attachment 815764 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager v3

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

This looks good. The main issues I see are:

- having to click on update to re-validate the manifest, update the project store (which updates the UI) and save the manifest
- using `eval` to update the manifest

Don't bother addressing all of these. We can iterate as long as this stays preffed off.

--


Is there a way to save the manifest without using the refresh button?
Also, can we get the validation errors without having to click update?

Changing the name property doesn't update the project name in the UI until the manifest is saved. But it works for the description. Maybe we should use manifest.name in the UI.

Do we have to use eval() ? It looks fragile (easy to break). Can't we rebuilt the manifest from the variable view instead of updating it with eval()?

Can we hide _proto_?

::: browser/branding/nightly/pref/firefox-branding.js
@@ +29,5 @@
>  pref("browser.search.param.yahoo-fr-ja", "");
>  pref("browser.search.param.yahoo-f-CN", "");
> +
> +// Enable the manifest editor on Nightly
> +pref("devtools.appmanager.manifestEditor.enabled", true);

We usually don't do that. Just keep it preffed-off.

::: browser/devtools/app-manager/content/manifest-editor.js
@@ +96,5 @@
> +
> +      let encoder = new TextEncoder();
> +      let data = encoder.encode(JSON.stringify(this.manifest, null, 2));
> +
> +      return OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" });

Is it required to have a tmp file?

::: browser/devtools/app-manager/content/projects.js
@@ +55,1 @@
>      });

We usually use "ready" instead of "initialize"

::: browser/devtools/app-manager/content/projects.xhtml
@@ +33,1 @@
>            <div onclick="UI.addHosted()" id="new-hosted-project-click" title="&projects.addHostedTooltip;"></div>

I guess this is for tests. Any alternative way to do this?

@@ +59,5 @@
>    <div>
>      <div class="project-details" template='{"type":"attribute","path":"validationStatus","name":"status"}'>
>        <div class="project-header">
>          <img class="project-icon" template='{"type":"attribute","path":"icon","name":"src"}'/>
> +        <div class="project-metadata">

I'd prefer if we don't rename classes (extension and themes might not like that).

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +462,5 @@
> +  width: 100%;
> +  height: 100%;
> +  border: 0;
> +  border-top: 5px solid #C9C9C9;
> +}

Can't you use flex-grow here?

@@ +465,5 @@
> +  border-top: 5px solid #C9C9C9;
> +}
> +
> +/* These blocks can removed when the manifest editor is always on */
> +

You a word.

Also, add a bug number here.
Attachment #815764 - Flags: review?(paul)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=7c2dee0320d8

(In reply to Paul Rouget [:paul] from comment #6)
> Comment on attachment 815764 [details] [diff] [review]
> Add VariablesView as manifest editor in App Manager v3
> 
> Review of attachment 815764 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review! :D

> Is there a way to save the manifest without using the refresh button?
> Also, can we get the validation errors without having to click update?

Yes, those things can be done.  Mostly I was just unsure if it made sense to save the file without some kind of explicit user action to that effect.  I've filed bug 925957 about this, so please add some feedback there!  :)

> Changing the name property doesn't update the project name in the UI until
> the manifest is saved. But it works for the description. Maybe we should use
> manifest.name in the UI.

Updated UI to use manifest.name, so it now updates immediately.

> Do we have to use eval() ? It looks fragile (easy to break). Can't we
> rebuilt the manifest from the variable view instead of updating it with
> eval()?

I agree is it unfortunate...  VariablesView is geared towards clients that use eval or something like it.  See webconsole (https://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#3495) and other clients for examples.

But, we should surely be able to expose a nicer API for local (non-remote) objects so that eval is not needed.  Filed bug 925923.

> Can we hide _proto_?

Should be pretty straightforward to add an option to VariablesView to omit this.  Filed 925928.

> ::: browser/branding/nightly/pref/firefox-branding.js
> @@ +29,5 @@
> >  pref("browser.search.param.yahoo-fr-ja", "");
> >  pref("browser.search.param.yahoo-f-CN", "");
> > +
> > +// Enable the manifest editor on Nightly
> > +pref("devtools.appmanager.manifestEditor.enabled", true);
> 
> We usually don't do that. Just keep it preffed-off.

Removed the default on for Nightly.

> ::: browser/devtools/app-manager/content/manifest-editor.js
> @@ +96,5 @@
> > +
> > +      let encoder = new TextEncoder();
> > +      let data = encoder.encode(JSON.stringify(this.manifest, null, 2));
> > +
> > +      return OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" });
> 
> Is it required to have a tmp file?

Yes, for some reason the API requires it.  Surely it could be defaulted...

> ::: browser/devtools/app-manager/content/projects.js
> @@ +55,1 @@
> >      });
> 
> We usually use "ready" instead of "initialize"

Changed.

> ::: browser/devtools/app-manager/content/projects.xhtml
> @@ +33,1 @@
> >            <div onclick="UI.addHosted()" id="new-hosted-project-click" title="&projects.addHostedTooltip;"></div>
> 
> I guess this is for tests. Any alternative way to do this?

Yes, it is for tests.  I think this is probably the best approach, and it doesn't seem that bad to allow chrome://.  Mostly you just want to ensure that there is some URL in that field before trying to add, so this modification of the pattern seems fine.

I experimented with having the test add the project directly to store, but then you have to duplicate much of the logic from project.js, like ensuring you validate after adding to store, etc.  This approach removes that duplication, and also tests the add hosted project UI code path.

> @@ +59,5 @@
> >    <div>
> >      <div class="project-details" template='{"type":"attribute","path":"validationStatus","name":"status"}'>
> >        <div class="project-header">
> >          <img class="project-icon" template='{"type":"attribute","path":"icon","name":"src"}'/>
> > +        <div class="project-metadata">
> 
> I'd prefer if we don't rename classes (extension and themes might not like
> that).

That makes sense as a general rule.  In this case though, the "project-details" class was originally used for two elements that are unrelated, and it seemed like it was just a mistake.  As you can see, it is still applied to the element that wraps the one I changed, a few lines up from there.

The styles that were applied previously were only actually needed on the outer one with that class, so that's why I gave the inner one a new class, so it could be styled separately.

> ::: browser/themes/shared/devtools/app-manager/projects.css
> @@ +462,5 @@
> > +  width: 100%;
> > +  height: 100%;
> > +  border: 0;
> > +  border-top: 5px solid #C9C9C9;
> > +}
> 
> Can't you use flex-grow here?

Yep, changed.

> @@ +465,5 @@
> > +  border-top: 5px solid #C9C9C9;
> > +}
> > +
> > +/* These blocks can removed when the manifest editor is always on */
> > +
> 
> You a word.
> 
> Also, add a bug number here.

Reworded and filed bug 925921.
Attachment #815764 - Attachment is obsolete: true
Attachment #816128 - Flags: review?(paul)
Comment on attachment 816128 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager v4

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

::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd
@@ +77,1 @@
>  

I'm not sure we want this label yet. We'll add that once we get the full design.
Attachment #816128 - Flags: review?(paul) → review+
Splinter…

> <!ENTITY projects.manifestEditor "Manifest Editor">

I'm not sure we want this label yet. We'll add that once we get the full design.
(In reply to Paul Rouget [:paul] from comment #9)
> Splinter…
> 
> > <!ENTITY projects.manifestEditor "Manifest Editor">
> 
> I'm not sure we want this label yet. We'll add that once we get the full
> design.

I think it's fine to leave the label in for now.  If we need to change or remove it, we just have to update the key, so it's not that bad.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f12bb231ae37
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f12bb231ae37
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: