Last Comment Bug 760748 - Add the category of the application to the desktop entry file
: Add the category of the application to the desktop entry file
Status: RESOLVED FIXED
: dev-doc-needed
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: unspecified
: All Linux
: P3 normal
: Firefox 17
Assigned To: Marco Castelluccio [:marco]
: Jason Smith [:jsmith]
Mentors:
Depends on: 744193 759906
Blocks: Apps-Dev-Doc-Needed 765399
  Show dependency treegraph
 
Reported: 2012-06-01 18:29 PDT by Marco Castelluccio [:marco]
Modified: 2016-02-04 15:00 PST (History)
14 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add support for the categories parameter (12.61 KB, patch)
2012-07-13 03:51 PDT, Marco Castelluccio [:marco]
felipc: feedback+
Details | Diff | Splinter Review
Use the categories parameter on Linux (3.06 KB, patch)
2012-07-13 04:22 PDT, Marco Castelluccio [:marco]
felipc: review+
Details | Diff | Splinter Review
Add support for the categories parameter (12.61 KB, patch)
2012-07-13 17:17 PDT, Marco Castelluccio [:marco]
fabrice: review-
Details | Diff | Splinter Review
Use the categories parameter on Linux (3.08 KB, patch)
2012-07-13 17:23 PDT, Marco Castelluccio [:marco]
felipc: review+
Details | Diff | Splinter Review
Add support for the categories parameter (6.58 KB, patch)
2012-08-06 17:14 PDT, Marco Castelluccio [:marco]
fabrice: review+
Details | Diff | Splinter Review
Use the categories parameter on Linux (3.71 KB, patch)
2012-08-06 17:22 PDT, Marco Castelluccio [:marco]
felipc: review+
Details | Diff | Splinter Review
Add support for the categories parameter (6.58 KB, patch)
2012-08-06 18:16 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Use the categories parameter on Linux (3.61 KB, patch)
2012-08-06 18:23 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review

Description Marco Castelluccio [:marco] 2012-06-01 18:29:46 PDT
We'll use the category from install_data (bug 759906).
Comment 1 Jason Smith [:jsmith] 2012-06-03 17:49:10 PDT
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] 2012-06-03 20:26:09 PDT
(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 2012-06-10 15:48:56 PDT
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) 2012-06-11 03:28:49 PDT
(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] 2012-06-11 03:37:49 PDT
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) 2012-06-11 06:19:46 PDT
(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] 2012-07-13 03:51:38 PDT
Created attachment 641805 [details] [diff] [review]
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] 2012-07-13 04:22:32 PDT
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
Comment 9 Marco Castelluccio [:marco] 2012-07-13 04:23:29 PDT
Comment on attachment 641805 [details] [diff] [review]
Add support for the categories parameter

Sorry, this isn't obsolete :)
Comment 10 Yann Brelière 2012-07-13 04:40:52 PDT
(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 me!) 2012-07-13 15:01:23 PDT
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 me!) 2012-07-13 15:06:03 PDT
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] 2012-07-13 17:17:23 PDT
Created attachment 642148 [details] [diff] [review]
Add support for the categories parameter

I've just fixed a nit and tested the patch.
Comment 14 Marco Castelluccio [:marco] 2012-07-13 17:23:29 PDT
Created attachment 642150 [details] [diff] [review]
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] 2012-07-23 13:59:48 PDT
Flagging dev-doc-needed. We should update the API spec to indicate how we handle categories.
Comment 16 [:fabrice] Fabrice Desré 2012-07-24 01:37:10 PDT
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] 2012-07-24 09:56:07 PDT
(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] 2012-07-24 10:48:14 PDT
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] 2012-07-24 10:51:08 PDT
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] 2012-07-24 15:10:02 PDT
(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] 2012-08-06 17:14:04 PDT
Created attachment 649485 [details] [diff] [review]
Add support for the categories parameter
Comment 22 Marco Castelluccio [:marco] 2012-08-06 17:22:37 PDT
Created attachment 649489 [details] [diff] [review]
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 me!) 2012-08-06 17:32:43 PDT
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é 2012-08-06 17:43:53 PDT
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] 2012-08-06 18:16:12 PDT
Created attachment 649505 [details] [diff] [review]
Add support for the categories parameter

Typo fix, carrying forward r+.
Comment 26 Marco Castelluccio [:marco] 2012-08-06 18:23:27 PDT
Created attachment 649507 [details] [diff] [review]
Use the categories parameter on Linux

Updated to the current tree, carrying forward r+.

Note You need to log in before you can comment on or make changes to this bug.