637291 - Add titlebar and name attributes to Panels

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, enhancement, P2)

Description

[Rob Campbell [:rc] (:robcee)]

The Panel module could provide a titlebar (with close button) and name properties for addons that want to create a more window-like panel.

Comment 1

[Rob Campbell [:rc] (:robcee)]

added a pull request for this functionality:

https://github.com/mozilla/addon-sdk/pull/113

Please let me know if this is the best way to attach a pull request to a bugzilla bug.

Comment 2

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> added a pull request for this functionality:
> 
> https://github.com/mozilla/addon-sdk/pull/113

Awesome, thanks!

> Please let me know if this is the best way to attach a pull request to a
> bugzilla bug.

There's not an ideal way. Atul came up with this (scroll to the bottom): https://github.com/toolness/pybugzilla.

It's better, but not super easy on the workflow to attach.

Comment 3

[Dietrich Ayala (:dietrich)]

The titlebar and name bits look fine so far. In the spirit of "DOM, you're dead to me", the ID shouldn't be necessary. What's your use-case for that?

Can you update the docs and add some tests to test-panel.js for these changes?

Comment 4

[Rob Campbell [:rc] (:robcee)]

ah yeah. I heard of atul's magical bugzilla-github bridge. Thanks for the link.

(In reply to comment #3)
> The titlebar and name bits look fine so far. In the spirit of "DOM, you're dead
> to me", the ID shouldn't be necessary. What's your use-case for that?

It's to provide compatibility with Firebug's panel API which we're trying to support.

I wonder if it wouldn't make more sense to have a generic properties mechanism we could pass in via the options object?

e.g., { properties: { id: "value", etc: "more",... } }

and we could just hang onto those at initialization time.

> Can you update the docs and add some tests to test-panel.js for these changes?

Will do! Thanks for taking a look at this.

Comment 5

[Rob Campbell [:rc] (:robcee)]

updated my pull request with the above suggestions (doc updated, added tests). Also removed the ID attribute as it's unneeded. Some minor whitespace fixes in nearby code too since I was in there.

Comment 6

[Dietrich Ayala (:dietrich)]

Great, thanks! Can you do the pybugzilla attachment, or attach the patch itself here? Then request review from me (or any Jetpack reviewer) and Myk for API review.

Comment 7

[Rob Campbell [:rc] (:robcee)]

Attachment: Pointer to pull request

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 518045 [details]
Pointer to pull request

asking for additional review for the API pieces. Had to expose panel.titlebar and panel.name for the unittests.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 518045 [details]
Pointer to pull request

This seems like a fine API, just two thoughts:

1. The Context Menu and Widgets APIs both expose "label" properties (for context menu items and widgets, respectively), while there aren't currently any high-level APIs that expose "name" properties, so for consistency it would make more sense to call the "name" property "label" (unless you expect to reuse its value in a way to which the word "label" doesn't apply).

2. The patch allows one to create a panel with a titlebar but no label and vice-versa, but it isn't clear what the use cases would be for such  configurations, so I wonder if the API could be as simple as a single "label" property that, if specified, will by itself cause the titlebar to appear and the panel to become draggable and more persistent (instead of requiring one to specify the additional "titlebar" property in addition to "label" to obtain those characteristics).

Comment 10

[Rob Campbell [:rc] (:robcee)]

Those are good suggestions. I mentioned initially that the id and name properties were to provide compatibility with Firebug conventions. That might not be entirely necessary as they can likely do their own associations if necessary and I'd prefer not to over-complicate the Add-ons SDK needlessly.

I'll update the patch per your feedback and repost.

Thanks!

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Despite what I said earlier, after triaging bug 595040, I realize that there is a use case for making a shown panel persist in the visible state until the addon explicitly hides it without also adding additional UI like a titlebar.

So it does sense to have a separate boolean property for specifying that.  The triage discussion, and some post-triage noodling, suggests that the name of the property should be "isPersistent".

Comment 12

[Rob Campbell [:rc] (:robcee)]

do you want me to incorporate that change into this patch or do you want to do it separately. Sorry I haven't gotten back to this yet. I didn't get a chance to fix up my patch this week, but am still planning on getting to it soon.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Either way works.  If you can incorporate the change, do so!  Otherwise, no worries, we'll do them separately.

Comment 14

[Rob Campbell [:rc] (:robcee)]

It should be a trivial addition. I'll try to get you a revised patch RSN!

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Marking anything that potentially looks like a feature request as "enhancement", sorry for the bugspam. :)



(On a side note, any word on that revised patch, Rob?)

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Re-prioritizing all 1.1-targeted feature requests to 1.2.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

I tried to implement this and it looks like panel title bars are not used and majorly broken..

See https://github.com/erikvold/addon-sdk/tree/637291

Rob can you point me to a working example?

Comment 20

[Rob Campbell [:rc] (:robcee)]

I had one back in the mists of time but it's been awhile since I've looked at xul:panels.

from MDN, https://developer.mozilla.org/en-US/docs/XUL/panel

#Attributes:



titlebar New in Firefox 4
    Type: string
    Set the panel's titlebar attribute to the value normal to display a panel with a titlebar. The noautohide attribute must be set to true.

I believe that's what I was doing. Let me know if that helps and if not I can try to dig up some code.

Comment 21

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #20)
> I had one back in the mists of time but it's been awhile since I've looked
> at xul:panels.
> 
> from MDN, https://developer.mozilla.org/en-US/docs/XUL/panel
> 
> #Attributes:
> 
> 
> 
> titlebar New in Firefox 4
>     Type: string
>     Set the panel's titlebar attribute to the value normal to display a
> panel with a titlebar. The noautohide attribute must be set to true.
> 
> I believe that's what I was doing. Let me know if that helps and if not I
> can try to dig up some code.

Ya I looked at https://github.com/mozilla/addon-sdk/pull/113 and removed the bit rot, but it wasn't working (iirc nothing in the appearance of the panel changed).  Then I made this attempt https://github.com/erikvold/addon-sdk/compare/master...637291 based on some mxr test code iirc but this doesn't display right.  Maybe it just needs some css work idk.

I'm dropping this for now tho.

Comment 22

[Erik Vold [:erikvold] (please needinfo? me)]

A 3rd party module can handle this use case so I don't think we should spend time on it.