760748 - Add the category of the application to the desktop entry file

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect, P3)

Description

[Marco Castelluccio [:marco]]

We'll use the category from install_data (bug 759906).

Comment 1

[Jason Smith [:jsmith]]

install_data? Don't see that mentioned here:

https://developer.mozilla.org/en/Apps/Apps_JavaScript_API/navigator.mozApps.install

Are the docs out of date here?

Comment 2

[Mark Giffin [:markg]]

(In reply to Jason Smith [:jsmith] from comment #1)
> install_data? Don't see that mentioned here:
> 
> https://developer.mozilla.org/en/Apps/Apps_JavaScript_API/navigator.mozApps.
> install
> 
> Are the docs out of date here?

This doc is correct right now for Nightly. But per anant, michaelhanson and fabrice on #openwebapps 6/1/12, install_data will be added back in. Not sure if there's a bug on it yet.

Comment 3

[skierpage]

Marco Castelluccio, nice work! I assume you mean the *Categories=* line, per 
http://standards.freedesktop.org/menu-spec/menu-spec-1.0.html#category-registry

without a Categories= line, web apps show up in the "Lost & Found" submenu of the "Kickoff" start menu on the KDE Plasma Desktop (but it's great they show up at all); I mentioned this in https://wiki.mozilla.org/Marketplace/Mozillian_Preview

Comment 4

[John Drinkwater (:beta)]

(In reply to skierpage from comment #3)
> without a Categories= line, web apps show up in the "Lost & Found" submenu
> of the "Kickoff" start menu on the KDE Plasma Desktop (but it's great they
> show up at all); 

And on GNOME 2, they appear in the Other menu below Office.

It might be nice to include a default category and prepend install_data before ‘Network’ which would put it into the Internet menu.

Comment 5

[Marco Castelluccio [:marco]]

The web apps should behave exactly like native applications. If a developer writes, for example, an eBook reader, he doesn't want his application to appear in the Internet menu.

Comment 6

[John Drinkwater (:beta)]

(In reply to Marco Castelluccio from comment #5)
> The web apps should behave exactly like native applications. If a developer
> writes, for example, an eBook reader, he doesn't want his application to
> appear in the Internet menu.

If the eBook reader is strictly offline, then I agree.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Add support for the categories parameter

I'm requesting only feedback as I can't build and test the patch because of a problem with Angle.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Use the categories parameter on Linux

I did my best to translate the Marketplace categories to freedesktop categories: http://standards.freedesktop.org/menu-spec/latest/apa.html

Comment 9

[Marco Castelluccio [:marco]]

Comment on attachment 641805 [details] [diff] [review]
Add support for the categories parameter

Sorry, this isn't obsolete :)

Comment 10

[Yann Brelière]

(In reply to Marco Castelluccio from comment #8)
> Created attachment 641812 [details] [diff] [review]
> Use the categories parameter on Linux
> 
> I did my best to translate the Marketplace categories to freedesktop
> categories: http://standards.freedesktop.org/menu-spec/latest/apa.html

May I suggest adding 'Network;' to "social"?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 641812 [details] [diff] [review]
Use the categories parameter on Linux

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

Can you add two comments in this functions for easier reference: one above the function (javadoc style) that points to the FreeDesktop's list of categories, and another inside the function just saying that a trailing ; is needed. (to avoid someone accidentally removing it in the future)

::: browser/modules/WebappsInstaller.jsm
@@ +776,5 @@
>    },
>  
> +  _translateCategories: function() {
> +    let translations = {
> +                         "books-reference": "Education;Literature",

you can indent this block just one indent further from the declaration, like:

let translations = {
  "books-ref": "...",
  "business": "...",
};

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 641805 [details] [diff] [review]
Add support for the categories parameter

This looks good to me. I'm leaving the feedback req for Ian for him to take a look as well.

Fabrice would need to review it. Fabrice, if you are too busy to review this let me know and I'll see if Jst can look at it, or I can do a thorough review if you take a simple look over the approach.

fwiw: this patch is adding support for another field in the install parameters called categories, so that mozApps.install can be called as:

mozApps.install("foo.manifest", {receipt: xxx, categories: ["Office", "Chat"]});

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Add support for the categories parameter

I've just fixed a nit and tested the patch.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Use the categories parameter on Linux

I've addressed your review comments.

(In reply to Yann Brelière from comment #10)
> May I suggest adding 'Network;' to "social"?

Well, the social category is probably the worst translation I did :)
However, I don't know if a social application should always be a network application.

Getting feedback from some Linux package maintainers and from some Marketplace folks could be extremely useful.

Comment 15

[Jason Smith [:jsmith]]

Flagging dev-doc-needed. We should update the API spec to indicate how we handle categories.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 642148 [details] [diff] [review]
Add support for the categories parameter

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

r- not for code issues, but because I need to be convinced that we need to expose that to web content on the Application object.

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +14,5 @@
>  {
>    readonly attribute jsval manifest;
>    readonly attribute DOMString manifestURL;
>    readonly attribute jsval receipts; /* an array of strings */
> +  readonly attribute jsval categories; /* an array of strings */

Do we really want to expose that to web content?

@@ +92,5 @@
>     *
>     * @param manifestUrl : the URL of the webapps manifest.
>     * @param parameters  : A structure with optional information.
>     *                      { receipts: ... } will be used to specify the payment receipts for this installation.
> +   *                      { categories: ... } will be used to specify the categories of the webapp.

nit:

{
 receipts: ....
 categories: ...
}

@@ +119,5 @@
>     *
>     * @param packageUrl : the URL of the webapps manifest.
>     * @param parameters : A structure with optional information.
>     *                     { receipts: ... } will be used to specify the payment receipts for this installation.
> +   *                     { categories: ... } will be used to specify the categories of the webapp.

ditto

Comment 17

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Comment on attachment 642148 [details] [diff] [review]
> Add support for the categories parameter
> 
> Review of attachment 642148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- not for code issues, but because I need to be convinced that we need to
> expose that to web content on the Application object.

For example a web dashboard could show the installed applications grouped by category. Or search the installed applications that are in a particular category.

Comment 18

[Anant Narayanan [:anant]]

I do agree with Fabrice that we should refrain from adding explicit getters for fields that have arbitrary meanings. The intention for install_data is precisely to store information like this, but we shouldn't be creating an index of allowed properties.

The application/dashboard can already retrieve the install_data via getSelf() - atleast when bug 760339 is fixed - so an additional property on the application object is not required.

Marco, do you feel like tackling bug 760339? :)

Comment 19

[Anant Narayanan [:anant]]

I will hasten to add that the "receipts" property in install_data /should/ be treated specially, so a fix for bug 760339 will retain the AppObject.receipts property, but we should not add special properties for anything else (you can always get to it via AppObject.installData.categories, for example).

Comment 20

[Marco Castelluccio [:marco]]

(In reply to Anant Narayanan [:anant] from comment #18)
> The application/dashboard can already retrieve the install_data via
> getSelf() - atleast when bug 760339 is fixed - so an additional property on
> the application object is not required.

Great, I didn't know that bug :)

> Marco, do you feel like tackling bug 760339? :)

I'll work on it, but I won't be at home for nearly a week.

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Add support for the categories parameter

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Use the categories parameter on Linux

I've just added a check to add the "Categories" field to the desktop entry file only when at least a category is passed.
Even if unrelated, I've removed an unused variable.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 649489 [details] [diff] [review]
Use the categories parameter on Linux

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

::: toolkit/webapps/WebappsInstaller.jsm
@@ +829,5 @@
> +
> +    let categories = this._translateCategories();
> +    if (categories.length > 0)
> +      writer.setString("Desktop Entry", "Categories", categories);
> +

just use "if (categories)"

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 649485 [details] [diff] [review]
Add support for the categories parameter

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

::: dom/apps/src/Webapps.jsm
@@ +522,5 @@
>                          .createInstance(Ci.nsIZipReader);
>          try {
>            zipReader.open(zipFile);
>            if (!zipReader.hasEntry("manifest.webapp")) {
>              throw "No manifest.webapp found.";

You also need to update _cloneAppObject

Comment 25

[Marco Castelluccio [:marco]]

Attachment: Add support for the categories parameter

Typo fix, carrying forward r+.

Comment 26

[Marco Castelluccio [:marco]]

Attachment: Use the categories parameter on Linux

Updated to the current tree, carrying forward r+.

Comment 27

[Anant Narayanan [:anant]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6452db09473
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b948b3565a

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c6452db09473
https://hg.mozilla.org/mozilla-central/rev/b8b948b3565a