Closed Bug 662012 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 4 obsolete files)

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

Any other suggestions?
Attached patch Colour (obsolete) — Splinter Review
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #537695 - Flags: review?(dtownsend)
Attachment #537696 - Flags: review?(dtownsend)
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.
Attachment #537695 - Flags: review?(dtownsend) → review+
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
Attachment #537696 - Flags: review?(dtownsend) → review+
What's the story with mobile here?
Attached patch Colour (obsolete) — Splinter Review
I thought that was frowned upon, but I guess setTimeout is just as bad.
Attachment #537695 - Attachment is obsolete: true
Attached patch Colour v3Splinter Review
Attachment #538651 - Attachment is obsolete: true
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.
Attachment #539069 - Flags: review?(dtownsend)
Attached patch File and directory v2 (obsolete) — Splinter Review
I've changed the behaviour to show the full path on desktop and just the leaf name on mobile.
Attachment #539071 - Flags: review?(dtownsend)
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.
Attachment #539071 - Flags: review?(dtownsend) → review-
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
Attachment #539069 - Flags: review?(dtownsend) → review+
Attachment #539071 - Attachment is obsolete: true
Attachment #539379 - Flags: review?(dtownsend)
Attachment #539379 - Flags: review?(dtownsend) → review+
Attachment #539070 - Attachment is obsolete: true
Two patches to land (both v3). I'll update the docs at some point.
Merged:
http://hg.mozilla.org/mozilla-central/rev/3e14a58e0dee
http://hg.mozilla.org/mozilla-central/rev/c2a696f69c27
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
Geoff, can you please give us the link to the page you have updated for dev-doc?
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110703 Firefox/7.0a1
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Summary: Add more settings type to addon manager inline settings → Add file, directory, and color type to add-on manager inline preferences
See Also: → 1284166
You need to log in before you can comment on or make changes to this bug.