Add the category of the application to the desktop entry file

RESOLVED FIXED in Firefox 17

Status

Firefox Graveyard
Web Apps
P3
normal
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: marco, Assigned: marco)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
Firefox 17
All
Linux
dev-doc-needed
Dependency tree / graph

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Depends on: 760339

Updated

5 years ago
No longer depends on: 760339
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

5 years ago
(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.
Priority: -- → P3

Comment 3

5 years ago
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
(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.
(Assignee)

Comment 5

5 years ago
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.
(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.
(Assignee)

Updated

5 years ago
Blocks: 765399

Updated

5 years ago
QA Contact: jsmith
(Assignee)

Comment 7

5 years ago
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.
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #641805 - Flags: feedback?(ianb)
(Assignee)

Comment 8

5 years ago
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
Attachment #641805 - Attachment is obsolete: true
Attachment #641805 - Flags: feedback?(ianb)
Attachment #641812 - Flags: feedback?(felipc)
(Assignee)

Comment 9

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

Sorry, this isn't obsolete :)
Attachment #641805 - Attachment is obsolete: false
Attachment #641805 - Flags: feedback?(ianb)

Comment 10

5 years ago
(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 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": "...",
};
Attachment #641812 - Flags: feedback?(felipc) → review+
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"]});
Attachment #641805 - Flags: review?(fabrice)
Attachment #641805 - Flags: feedback+
(Assignee)

Comment 13

5 years ago
Created attachment 642148 [details] [diff] [review]
Add support for the categories parameter

I've just fixed a nit and tested the patch.
Attachment #641805 - Attachment is obsolete: true
Attachment #641805 - Flags: review?(fabrice)
Attachment #641805 - Flags: feedback?(ianb)
Attachment #642148 - Flags: review?(fabrice)
Attachment #642148 - Flags: feedback?(ianb)
(Assignee)

Comment 14

5 years ago
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.
Attachment #641812 - Attachment is obsolete: true
Attachment #642150 - Flags: review?(felipc)
Attachment #642150 - Flags: review?(felipc) → review+

Updated

5 years ago
status-firefox16: --- → wontfix
Flagging dev-doc-needed. We should update the API spec to indicate how we handle categories.
Keywords: dev-doc-needed
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
Attachment #642148 - Flags: review?(fabrice) → review-
(Assignee)

Comment 17

5 years ago
(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.
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? :)
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).
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
Created attachment 649485 [details] [diff] [review]
Add support for the categories parameter
Attachment #642148 - Attachment is obsolete: true
Attachment #642148 - Flags: feedback?(ianb)
Attachment #649485 - Flags: review?(fabrice)
(Assignee)

Comment 22

5 years ago
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.
Attachment #642150 - Attachment is obsolete: true
Attachment #649489 - Flags: review?(felipc)
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)"
Attachment #649489 - Flags: review?(felipc) → review+
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
Attachment #649485 - Flags: review?(fabrice) → review-
Attachment #649485 - Flags: review- → review+
(Assignee)

Comment 25

5 years ago
Created attachment 649505 [details] [diff] [review]
Add support for the categories parameter

Typo fix, carrying forward r+.
Attachment #649485 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
Created attachment 649507 [details] [diff] [review]
Use the categories parameter on Linux

Updated to the current tree, carrying forward r+.
Attachment #649489 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6452db09473
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b948b3565a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c6452db09473
https://hg.mozilla.org/mozilla-central/rev/b8b948b3565a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

Updated

5 years ago
Whiteboard: [qa+]

Updated

5 years ago
Whiteboard: [qa+]

Updated

5 years ago
Blocks: 783741
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.