999417 - Land the new App Manager UI

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

The current App Manager UI is too confusing, and the fact it's built inside a tab is a cause of many bugs.

Comment 1

[Paul Rouget [:paul]]

Attachment: github PR

Alright, I think it's time to get some initial landing. We need this code to land asap to unblock the work on itchpad.

To enable the code, you need to enable it at compile time: --enable-fxide. I don't want us to ship this code until we get something reasonably solid. It's self contained (we don't pollute /browser/locales and /browser/themes).

(a build peer review will be required)

The strategy to release the new app manager is:
- First, land this code
- Then, enable at build time but disabled in about:config
- Then enable it (it will replace the app manager) (released)
- Then remove the old app manager code

It's a lot of code, but I tried to keep it compatible with the initial version of the app manager (both tool can run at the same time). It could be better with some better project management code, and a more generic notion of "runtimes", but it's a lot of work.

There's not test yet.

Most of the code is split between fxide.js (UI code), and app-manager.js (logic code).

I have a huge todo list on the side. I will start filing bugs once this lands.

Ryan, don't be too severe with this review. We want this to land soon, and the code is not even compiled (won't get shipped to the user). Please split your comments in 3 categories:
- can't land until comment addressed
- can't be enabled at build time until comment addressed
- can't replace initial app manager until comment addressed

We will file follow-up bugs.

Comment 2

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

Comment on attachment 8418074 [details] [review]
github PR

Coming together nicely! :D There's not much that needs to be resolved before landing.

See the PR for various comments, as well as a review summary at the end.  I'll repeat that here for completeness.

I'll attempt to use this version for now on, so I can file bugs and such.

**Review Summary**

* Before landing
    * Explain meaning of FXIDE
        * Will this `fxide` name and directories live on after release, or merge with existing dirs, etc.?
    * Explain why I can't start projects
        * Only seems to work with the runtime apps list?
    * Get build peer review

* Before enabling at build time
    * Need to see how Itchpad interaction works once it lands with this
    * Pick one type of `Promise` / `promise`

* Before replacing current App Manager
    * Real URL hosting for templates URL
    * Need to keep some way to connect to manual host/port
    * Ensure existing projects from current version are ready to use immediately on first load
    * Break up `app-manager.js` into different modules
    * Move logic out of view JS files and down into `app-manager.js` / other logic modules
    * Break out inline styles and HTML into separate files
    * Pick a new name other than `lense`
    * Increase Logs font size when expanded
    * Why use sprites in the browser?
    * Maybe add some tooltips? :)
    * Various nits (check the full diff)

Comment 3

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

What bug do I block for things to be done before replacing the current one?

Comment 4

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #2)
> **Review Summary**
> 
> * Before landing
>     * Explain meaning of FXIDE
>         * Will this `fxide` name and directories live on after release, or
> merge with existing dirs, etc.?

fxide is a generic name. The same way "browser" is the name of "firefox".

>     * Explain why I can't start projects
>         * Only seems to work with the runtime apps list?

It should work with the simulator. It works here.

>     * Get build peer review

Yes


>     * Pick one type of `Promise` / `promise`


I'll do that in this bug.

Comment 5

[Paul Rouget [:paul]]

Attachment: part1 - Validator should exclude non-existing directories

Comment 6

[Paul Rouget [:paul]]

Attachment: v1

Switching to bugzilla (let's forget about the github PR)

Comment 7

[Paul Rouget [:paul]]

Comment on attachment 8418703 [details] [diff] [review]
v1

(build peer review)

Michael,

we introduce a new project in the devtools. Its codename is fxide. This is an initial landing, and we don't want to build it by default (yet). We also try to keep the code as self contained as possible as we might want to be able to ship it outside of Firefox one day.

Can you look at:

/configure.in
/browser/app/profile/firefox.js
/browser/devtools/fxide/Makefile.in
and all the build-related files in /browser/devtools/fxide/

Thank you.

Comment 8

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #3)
> What bug do I block for things to be done before replacing the current one?

See https://bugzilla.mozilla.org/showdependencytree.cgi?id=999417&hide_resolved=1

Comment 9

[Paul Rouget [:paul]]

Attachment: v1.1

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8418744 [details] [diff] [review]
v1.1

Everything looks good from a build perspective.

Comment 11

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

Comment on attachment 8418744 [details] [diff] [review]
v1.1

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

::: browser/devtools/fxide/content/fxide.js
@@ +6,5 @@
> +const Cu = Components.utils;
> +const Ci = Components.interfaces;
> +
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

It doesn't seem like you use this currently...?  Seems other people return all the promise you are using in this file.

::: browser/devtools/fxide/modules/app-manager.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {Cu} = require("chrome");
> +
> +Cu.import("resource://gre/modules/Promise.jsm");

Other DevTools code loads this as:

let { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});

so it is used as the lowercase version.  Are we not doing that anymore in new files?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> Comment on attachment 8418744 [details] [diff] [review]
> v1.1
> 
> Review of attachment 8418744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/fxide/content/fxide.js
> @@ +6,5 @@
> > +const Cu = Components.utils;
> > +const Ci = Components.interfaces;
> > +
> > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > +Cu.import("resource://gre/modules/Promise.jsm");
> 
> It doesn't seem like you use this currently...?  Seems other people return
> all the promise you are using in this file.

Promise is used in the current integration patch in Bug 987089, but it will not be needed anymore after I upload an updated patch that changes the itchpad API slightly (which I will do shortly)

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to J. Ryan Stinnett [:jryans] from comment #11)
> > Comment on attachment 8418744 [details] [diff] [review]
> > v1.1
> > 
> > Review of attachment 8418744 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/fxide/content/fxide.js
> > @@ +6,5 @@
> > > +const Cu = Components.utils;
> > > +const Ci = Components.interfaces;
> > > +
> > > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > > +Cu.import("resource://gre/modules/Promise.jsm");
> > 
> > It doesn't seem like you use this currently...?  Seems other people return
> > all the promise you are using in this file.
> 
> Promise is used in the current integration patch in Bug 987089, but it will
> not be needed anymore after I upload an updated patch that changes the
> itchpad API slightly (which I will do shortly)

As of https://bugzilla.mozilla.org/show_bug.cgi?id=987089#c38, Promise is no longer needed for the itchpad integration if you have the latest two patches from Bug 987089 applied.

Comment 14

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

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] from comment #2)
> > **Review Summary**
> > 
> > * Before landing
> >     * Explain meaning of FXIDE
> >         * Will this `fxide` name and directories live on after release, or
> > merge with existing dirs, etc.?
> 
> fxide is a generic name. The same way "browser" is the name of "firefox".

Okay, I'd still like to understand the thinking here in more detail.

Do you imagine we'll call the end result "Firefox IDE"?  Will we point at the end result and say "this is our IDE for Firefox OS / apps"?  I haven't really heard these words said much yet so far.  The only thing I've heard is App Manager v2 / v3.  Obviously, I understand that it now contains an editor and resembles other tools that people call IDEs.

I think part of it is the "fx" portion of "fxide":  Your analogy about it being a generic name seems flawed since it includes the branded browser's name of Firefox / "fx".  All the files are already under the "browser" directory, so "fx" is redundant.

I think it's important to get even code names "right" / close to the name of the end product early on because that's what everyone will say, at least internally, and it gets really confusing when they don't match the external / marketing name.  Also, people tend to avoid changing internal names to match external names after the fact, since tons of files are changed and moved to do it, it bitrots all patches, etc.

This seems simpler to hash out over IRC or Vidyo, rather than having me post many hypothetical questions to understand your thinking. :) I know you want to land now / yesterday, so let's work through this one last thing, and you should be ready to land.

Comment 15

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

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] from comment #2)
> >     * Explain why I can't start projects
> >         * Only seems to work with the runtime apps list?
> 
> It should work with the simulator. It works here.

Haha, it actually works here too...  Most of my projects just have errors so it refuses to install them. ;)

Comment 16

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #14)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #4)
> > (In reply to J. Ryan Stinnett [:jryans] from comment #2)
> > > **Review Summary**
> > > 
> > > * Before landing
> > >     * Explain meaning of FXIDE
> > >         * Will this `fxide` name and directories live on after release, or
> > > merge with existing dirs, etc.?
> > 
> > fxide is a generic name. The same way "browser" is the name of "firefox".
> 
> Okay, I'd still like to understand the thinking here in more detail.
> 
> Do you imagine we'll call the end result "Firefox IDE"?  Will we point at
> the end result and say "this is our IDE for Firefox OS / apps"?  I haven't
> really heard these words said much yet so far.  The only thing I've heard is
> App Manager v2 / v3.  Obviously, I understand that it now contains an editor
> and resembles other tools that people call IDEs.

It's totally an IDE :)

> I think part of it is the "fx" portion of "fxide":  Your analogy about it
> being a generic name seems flawed since it includes the branded browser's
> name of Firefox / "fx".  All the files are already under the "browser"
> directory, so "fx" is redundant.

Right. But I think it sounds better than just "ide".

> I think it's important to get even code names "right" / close to the name of
> the end product early on because that's what everyone will say, at least
> internally, and it gets really confusing when they don't match the external
> / marketing name.  Also, people tend to avoid changing internal names to
> match external names after the fact, since tons of files are changed and
> moved to do it, it bitrots all patches, etc.

I'm not strongly attached to fxide. I don't even like it. It was first fxcode,
but it's too close to xcode. I really don't want to keep the name app-manager
because:
- it won't only be about app in the future
- it's not only a "manager", it's an editor too
- it needs to live next to the previous app-manager (we can't have 2 directories with the same name)

> This seems simpler to hash out over IRC or Vidyo, rather than having me post
> many hypothetical questions to understand your thinking. :) I know you want
> to land now / yesterday, so let's work through this one last thing, and you
> should be ready to land.

Alright!

Comment 17

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #2)
>     * Maybe add some tooltips? :)

There are tooltips. But they don't work when buttons are disabled. Added to my todolist.

Comment 18

[Paul Rouget [:paul]]

Just for the record (and this is only valid at the time I'm typing this), here is my todolist:

Before enable:
* "no simulator" experience
* "no device" experience
* "no adb addon" install experience
* if no USB devices, or fail to connect, link to troubleshouting section
* first run experience
* if no app, suggest creating new app
* -chrome chrome://fxide/content doesn’t work well
* unlistenToApps
* Send commands to AppManager (from vim, I want to be able to build): bug 1007218
* missing icon in "remove project"
* support cordova
* Make sure we cover all the features of the V1
* pre-hook  
* update MDN
* blue icons for debug and device
* Need to keep some way to connect to manual host/port
* Ensure existing projects from current version are ready to use immediately on first load
* Break up `app-manager.js` into different modules
* Move logic out of view JS files and down into `app-manager.js` / other logic modules
* Increase Logs font size when expanded
* Why use sprites in the browser? (PAUL: ?)
* Maybe add some tooltips? (PAUL: we have tooltips)

Before build:
* rtl
* Can't copy logs, or even select just a line
* newapp.js logic in app-manager.js
* disable black theme in toolbox and itchpad 
* toolbox can stay open if device disconnected
* windows & linux support  
* remove animation from panels  
* If icon updated on hard drive, only the cached version appears (in 3 locations)
* inline HTML and CSS for permission table and device info: UGLY!!!
* Real URL hosting for templates URL
* icons are cut in linux
  
Itchpad:
* add gear to itchpad header
* add validation dots to itchpad header
* Update logo & name
* add “edit manifest” button
* menu “Edit” and “File"  
* The manifest file should be very visible and have a special syntax highlighting

Misc:
* resource:/// … /fxide/…  
* Ctrl-w Ctrl-q is confusing
* add real URL for app templates
* Sheet Apps are considered “closed” after one of their window is closed. bug 999414
* command line options
* Show all the simulators (from JSON online), and add an “install” button
* uninstall  
* running adb logcat disconnect the device  
* cache template JSON (and download asap)

Comment 19

[Paul Rouget [:paul]]

In term of OS integration, the main issue is the panel overflow on Linux and Windows. I'll look at that later.

Comment 20

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #2)
>     * Why use sprites in the browser?

I don't understand that.

Comment 21

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

We agreed over Vidyo to rename to just "ide" since that's a generic word for what we have here.

Comment 22

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

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #20)
> (In reply to J. Ryan Stinnett [:jryans] from comment #2)
> >     * Why use sprites in the browser?
> 
> I don't understand that.

Paul explained we already use them in many places in the browser.  Sort of blows my mind that this is necessary... :( But anyway, seems fine then.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

(In reply to J. Ryan Stinnett [:jryans] from comment #21)
> We agreed over Vidyo to rename to just "ide" since that's a generic word for
> what we have here.

My other suggestion would be "webide", which gives it a bit more specificity (and heft) while remaining generic enough.  I would also consider "webappide", although y'all noted that it won't be app-specific in the future, so that seems too constraining.

Comment 24

[Paul Rouget [:paul]]

Right, "ide" feels light. And I like webide. So I'll go for webide.

Comment 25

[Paul Rouget [:paul]]

Attachment: v1.2: Land the new App Manager UI. r=jryans r=mshal

Fixed the promises. Rename fxide to webide.

Comment 26

[Paul Rouget [:paul]]

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

Comment 28

[Paul Rouget [:paul]]

Comment on attachment 8420549 [details] [diff] [review]
v1.2: Land the new App Manager UI. r=jryans r=mshal

Cancelling the review. I want to clean up the /themes/ code.

Comment 29

[Paul Rouget [:paul]]

Attachment: v1.2: Land the new App Manager UI. r=jryans r=mshal

Michael, sorry to ask you to re-review this. 2 things changed: we changed the name (fxide -> webide), we changed the way /themes/ is build (no platform specific directories, just preprocessor).

Comment 30

[Paul Rouget [:paul]]

Comment on attachment 8420937 [details] [diff] [review]
v1.2: Land the new App Manager UI. r=jryans r=mshal

Notable changes (beside renaming): no platform-specific css files anymore (see previous comment), and fixed a bunch of issues for Linux and Windows.

Comment 31

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

Comment on attachment 8420937 [details] [diff] [review]
v1.2: Land the new App Manager UI. r=jryans r=mshal

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

Looks good! :D "webide" seems fine to me.

::: browser/devtools/webide/locales/en-US/webide.properties
@@ +8,5 @@
> +runtimeButton_label=Select Runtime
> +projectButton_label=Open App
> +
> +importPackagedApp_title=Select directory
> +importHostedApp_title=Open Hosted Ap

Ap -> App

Comment 32

[Paul Rouget [:paul]]

Attachment: fixed typo (patch to land)

Comment 33

[Michael Shal [:mshal]]

Comment on attachment 8420937 [details] [diff] [review]
v1.2: Land the new App Manager UI. r=jryans r=mshal

Build side still looks good!

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85c60372f2bf