Display build errors

RESOLVED FIXED in Firefox 38

Status

DevTools
WebIDE
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: jryans, Assigned: ochameau)

Tracking

Trunk
Firefox 38
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

If a project has a build step that fails, we don't make that very clear at all.
(Assignee)

Updated

4 years ago
Blocks: 1000993
(Assignee)

Updated

4 years ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 1

4 years ago
Created attachment 8555896 [details] [diff] [review]
patch - v1

This add a panel that is transcient, if build succeed, permanent if failed panel.
It just contains all command output.
There is no way to display this panel back after a successful build...
I think we do want to hide it automatically, but we most likely want to somehow
offer a way to look at logs when the build succeed.
Attachment #8555896 - Flags: feedback?(jryans)
(Assignee)

Comment 2

4 years ago
Created attachment 8556411 [details] [diff] [review]
patch - v2

Rebased
Attachment #8555896 - Attachment is obsolete: true
Attachment #8555896 - Flags: feedback?(jryans)
Attachment #8556411 - Flags: feedback?(jryans)
(Reporter)

Comment 3

4 years ago
Comment on attachment 8556411 [details] [diff] [review]
patch - v2

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

The new files aren't in your patch.
Attachment #8556411 - Flags: feedback?(jryans)
(Assignee)

Comment 4

4 years ago
Created attachment 8556467 [details] [diff] [review]
patch - v3

With new files!!
(Assignee)

Updated

4 years ago
Attachment #8556411 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8556467 - Flags: feedback?(jryans)
(Reporter)

Comment 5

4 years ago
Comment on attachment 8556467 [details] [diff] [review]
patch - v3

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

In general, I think this works nicely, even though it's quite basic so far.

I agree we should offer some way to access the logs even when successful.  For example, Gaia's build returns success even when there is actually an error, so it's a bit confusing for them to close when they do.

Perhaps a button in the project details pane to "Show Last Pre-package Log"?

Paul, do you have a feedback or suggestions on the UI of this?  I am hoping your thoughts could help in place of our non-existent UI designer. :)

::: browser/devtools/webide/content/webide.js
@@ +1033,5 @@
>      }));
>    },
> +
> +  prePackageLog: function (msg) {
> +    UI.selectDeckPanel("logs");

This means we |selectDeckPanel| for every log message.

Please optimize |selectDeckPanel| to not do any work if the given panel is already active.

::: browser/devtools/webide/modules/app-manager.js
@@ +514,5 @@
>      return Task.spawn(function* () {
>        let self = AppManager;
>  
> +      let packageDir = yield ProjectBuilding.build(project,
> +                                                   self.update.bind(self, "pre-package"));

Pass an object instead, like:

let packageDir = yield ProjectBuilding.build({ project, logger: self.update.bind(self, "pre-package") });

This way it's more obvious that this function is a logger and not some success callback or something.

::: browser/devtools/webide/modules/build.js
@@ +68,5 @@
> +
> +    return packageDir;
> +  }),
> +
> +  _build: Task.async(function* (project, manifest, logger) {

Not amazed by |build| and |_build|, but not sure of a good suggestion either.

::: browser/devtools/webide/themes/jar.mn
@@ +15,5 @@
>    skin/runtimedetails.css      (runtimedetails.css)
>    skin/permissionstable.css    (permissionstable.css)
>    skin/monitor.css             (monitor.css)
>    skin/config-view.css         (config-view.css)
> +  skin/devicepreferences.css   (devicepreferences.css)

This file was removed, you should remove this line.

::: browser/devtools/webide/themes/logs.css
@@ +1,3 @@
> +html, body {
> +  background: black;
> +  color: white;

I guess you're going for a terminal-like appearance?

I think it would be better to use the DevTools theme colors and match the editor's basic background and text color.

@@ +1,4 @@
> +html, body {
> +  background: black;
> +  color: white;
> +}

Nit: Keep an empty line after }

@@ +9,5 @@
> +  padding: 0;
> +  font-family: arial;
> +  font-size: 1em;
> +}
> +li {

Use a monospaced font for the log messages.  (Should happen by default if you use the theme per above.)

::: browser/locales/en-US/chrome/browser/devtools/webide.dtd
@@ +176,5 @@
>  <!ENTITY monitor_title "Monitor">
>  <!ENTITY monitor_help "Help">
> +
> +<!-- Logs panel -->
> +<!ENTITY logs_title "Pre-packaging command logs">

English title case uses capitals for each letter of most words:

Pre-packaging Command Logs
Attachment #8556467 - Flags: feedback?(paul)
Attachment #8556467 - Flags: feedback?(jryans)
Attachment #8556467 - Flags: feedback+

Updated

4 years ago
Attachment #8556467 - Flags: feedback?(paul) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8561325 [details] [diff] [review]
patch v4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86f291b7410d

(In reply to J. Ryan Stinnett [:jryans] from comment #5)
> Comment on attachment 8556467 [details] [diff] [review]
> I agree we should offer some way to access the logs even when successful. 
> For example, Gaia's build returns success even when there is actually an
> error, so it's a bit confusing for them to close when they do.

Gaia build system is confusing, it should return error code in case of error...
Note that it *tries* to return error code in case of error, but often fails to do that :/
I think webide behavior is ok.

> Perhaps a button in the project details pane to "Show Last Pre-package Log"?

I thought about Jen proposal to make it appear on bottom, like the jsconsole
that appear in any panel of the toolboxes. But it would work when you already have
a toolbox opened (which is a typical usecase), because there isn't much space left
when webIDE window is at its default size.


> ::: browser/devtools/webide/content/webide.js
> @@ +1033,5 @@
> >      }));
> >    },
> > +
> > +  prePackageLog: function (msg) {
> > +    UI.selectDeckPanel("logs");
> 
> This means we |selectDeckPanel| for every log message.
> 
> Please optimize |selectDeckPanel| to not do any work if the given panel is
> already active.

I optimized selectDeckPanel and also call it only on first message.

> ::: browser/devtools/webide/modules/build.js
> @@ +68,5 @@
> > +
> > +    return packageDir;
> > +  }),
> > +
> > +  _build: Task.async(function* (project, manifest, logger) {
> 
> Not amazed by |build| and |_build|, but not sure of a good suggestion either.

The only suggestion that comes in mind is build and _doBuild, but I'm not sure it is any better?

> ::: browser/devtools/webide/themes/logs.css
> @@ +1,3 @@
> > +html, body {
> > +  background: black;
> > +  color: white;
> 
> I guess you're going for a terminal-like appearance?
> 
> I think it would be better to use the DevTools theme colors and match the
> editor's basic background and text color.

I tried that by using:
  background: --theme-body-background;
  color: --theme-body-color;

> @@ +9,5 @@
> > +  padding: 0;
> > +  font-family: arial;
> > +  font-size: 1em;
> > +}
> > +li {
> 
> Use a monospaced font for the log messages.  (Should happen by default if
> you use the theme per above.)

I don't see how it could "happen by default"? 
Should I import deck.css into this document?
Attachment #8556467 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 8561379 [details] [diff] [review]
patch v5

This patch adds a button in project panel that is displayed only
if the app has a prepackage entry in package.json.
Attachment #8561325 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8562024 [details] [diff] [review]
patch v6

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

Fixed some issue with CSS..
Attachment #8561379 - Attachment is obsolete: true
(Reporter)

Comment 9

4 years ago
Is this ready for more review?  No flag, but you asked some questions...
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 10

4 years ago
Created attachment 8562611 [details] [diff] [review]
patch v7

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

Fixed tests with new build() arguments.

I was waiting for green try before r?, let's see if that's the good one!
Attachment #8562024 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Comment on attachment 8562983 [details] [diff] [review]
patch v8

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

Try has still some orange, but hopefully that's not related to these changes!!
Attachment #8562983 - Flags: review?(jryans)
Comment on attachment 8562983 [details] [diff] [review]
patch v8

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

Great, I like it! :D

A few tips to blend in with DevTools theme.  Feel free to tweak if you think it makes things worse.

::: browser/devtools/webide/content/webide.js
@@ +771,5 @@
>    },
>  
>    selectDeckPanel: function(id) {
> +    let deck = document.querySelector("#deck");
> +    if (deck.selectedPanel && deck.selectedPanel.id === "#deck-panel-" + id) {

Seems like you forgot to remove the # here... :)

::: browser/devtools/webide/modules/build.js
@@ +40,2 @@
>      let manifest = yield ProjectBuilding.fetchPackageManifest(project);
>      if (!manifest || !manifest.webide || !manifest.webide.prepackage) {

Replace this with |!hasPrepackage|

::: browser/devtools/webide/test/test_build.html
@@ +51,5 @@
>              is(project.name, "--", "Display name uses manifest name");
>  
> +            let packageDir = yield ProjectBuilding.build({
> +              project,
> +              logger: console.log.bind(console)

Should we assert that something does get logged?

::: browser/devtools/webide/themes/logs.css
@@ +1,3 @@
> +html, body {
> +  background: var(--theme-body-background);
> +  color: var(--theme-body-color);

For the theme vars to actually have a value defined you need to include

<script type="application/javascript;version=1.8" src="chrome://browser/content/devtools/theme-switching.js"></script>

in the document.

@@ +8,5 @@
> +}
> +
> +ul {
> +  padding: 0;
> +  font-family: monospace;

If you add the DevTools common CSS:

<link rel="stylesheet" href="chrome://browser/skin/devtools/common.css" type="text/css"/>

you can get the right platform-specific monospace by adding the "devtools-monospace" class to the ul.
Attachment #8562983 - Flags: review?(jryans) → review+
(Assignee)

Comment 14

4 years ago
Created attachment 8564955 [details] [diff] [review]
patch v9

https://treeherder.mozilla.org/#/jobs?repo=try&revision=807ff041cc92

I've added naive assertion against logger. But test app don't log anything on stdout.
Attachment #8562983 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8565048 - Flags: review+
(Assignee)

Comment 16

4 years ago
Created attachment 8565147 [details] [diff] [review]
patch v11

https://treeherder.mozilla.org/#/jobs?repo=try&revision=172070d18614
With real assertions against log messages...
Attachment #8565048 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8565147 - Flags: review+
(Assignee)

Updated

4 years ago
Flags: needinfo?(poirot.alex)
Keywords: checkin-needed
(Reporter)

Updated

4 years ago
Blocks: 1124326
https://hg.mozilla.org/integration/fx-team/rev/d547c062e79a
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d547c062e79a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
(Reporter)

Updated

3 years ago
Depends on: 1136450

Comment 19

3 years ago
(In reply to J. Ryan Stinnett [:jryans] (on PTO Feb. 28 - Mar. 7) from comment #5)
> > 
> > +<!-- Logs panel -->
> > +<!ENTITY logs_title "Pre-packaging command logs">
> 
> English title case uses capitals for each letter of most words:
> 
> Pre-packaging Command Logs

Nitty perhaps, but ‘most words’ should probably be readd as ‘proper nouns’ here - See https://developer.mozilla.org/en-US/docs/MDN/Contribute/Content/Style_guide#Page_titles (presumably valid globally) and http://en.wikipedia.org/wiki/Proper_noun. Similar to ‘persistent logs’ in toolbox.tdt, I wonder if ‘command logs’ is one.

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.