662012 - Add file, directory, and color type to add-on manager inline preferences

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Geoff Lankow (:darktrojan)]

The inline settings could support a few more useful types of setting. On my todo list so far:
- file/directory
- colour

Any other suggestions?

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: Colour

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: File and directory

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 537695 [details] [diff] [review]
Colour

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

Try to avoid the setTimeout if possible, otherwise this looks fine.

::: toolkit/mozapps/extensions/content/setting.xml
@@ +368,5 @@
> +        <![CDATA[
> +          // We must wait for the colorpicker's binding to be applied before setting the value
> +          let self = this;
> +          let val = Services.prefs.getCharPref(self.pref);
> +          setTimeout(function() { self.value = val }, 0);

oof. You can sometimes force the binding to apply by getting offsetTop from the element. Would rather do that than this.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 537696 [details] [diff] [review]
File and directory

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

::: toolkit/mozapps/extensions/content/setting.xml
@@ +421,5 @@
> +              Cu.reportError(e);
> +            }
> +          }
> +          if (filePicker.show() != Ci.nsIFilePicker.returnCancel) {
> +            this.value = filePicker.file.path;

I wonder if it would be better to only put the filename in the display rather than the full path, particularly on mobile

Comment 5

[Dave Townsend [:mossop]]

What's the story with mobile here?

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: Colour

I thought that was frowned upon, but I guess setTimeout is just as bad.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: Colour v3

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: Colour v1 -> v3 interdiff

Looks like .clientTop isn't enough to call the colorpicker's constructor. Calling initialize twice (once by us and once by the constructor when it eventually happens) won't cause any problems, so we'll do that.

I've added the binding for mobile. There will need to be a better colour picker written for mobile, but that's another bug.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: File and directory v2

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: File and directory v1 -> v2 interdiff

I've changed the behaviour to show the full path on desktop and just the leaf name on mobile.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 539071 [details] [diff] [review]
File and directory v1 -> v2 interdiff

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

::: b/toolkit/mozapps/extensions/content/setting.xml
@@ +461,5 @@
> +          if (val) {
> +            try {
> +              let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
> +              file.initWithPath(val);
> +              label = document.documentElement.id == "addons-page" ? file.path : file.leafName;

I dislike relying on things outside of the XBL to change behaviour. Setting an attribute on the element to control whether to show filename or path would be better, either that or include both and use CSS to hide one depending on the app.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 539069 [details] [diff] [review]
Colour v1 -> v3 interdiff

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

::: b/toolkit/mozapps/extensions/content/setting.xml
@@ +368,5 @@
>          <![CDATA[
>            // We must wait for the colorpicker's binding to be applied before setting the value
> +          if (!this.input.color)
> +            this.input.initialize();
> +          this.value = Services.prefs.getCharPref(this.pref);

Not ideal either way for sure but at least this is more predictable

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: File and directory v2 -> interdiff

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: File and directory v3

Comment 15

[Geoff Lankow (:darktrojan)]

Two patches to land (both v3). I'll update the docs at some point.

Comment 16

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c2a696f69c27
http://hg.mozilla.org/integration/mozilla-inbound/rev/3e14a58e0dee

Comment 17

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/3e14a58e0dee
http://hg.mozilla.org/mozilla-central/rev/c2a696f69c27

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Geoff, can you please give us the link to the page you have updated for dev-doc?

Comment 19

[Geoff Lankow (:darktrojan)]

https://developer.mozilla.org/en/Extensions/Inline_Options

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110703 Firefox/7.0a1