The toggle side-panel button image does not change

[fflorent]

The button is always displayed as if the side-panel was expanded.

Florent

[fflorent]

@janodvarko What do you think of the above patch? Is there a cleaner way than this hack for the inspector panel?

Florent

[janodvarko]

Yep, good call, we need to fix this (not sure about the alpha blocker though).

@janodvarko What do you think of the above patch? Is there a cleaner way than this hack for the inspector panel?

Yeah, it looks hacky. I have already landed some platform API that should help us to implement this properly. See issue #88 and this platform bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1072208

There are new events fired by ToolSidebar object:

• 'show': fired when the side-bar is displayed (fired on the sidebar object)
• 'hide': fired when the side-bar is hidden (fired on the sidebar object)
• 'sidebar-created': fired when the side-bar is created (fired on the parent panel object)
• 'sidebar-destroyed': fired when the side-bar is destroyed (fired on the parent panel object)

Some goals/comments:

1. The entire logic related to the toggle button (its update) should be inside the toggleSideBarButton
2. The button should handle new events (above) to be notified about side panel open/close and update itself automatically.
3. In order to handle 'show' (or 'hide') event on the side bar, we need to register 'sidebar-created' ('sidebar-destroyed') fired by the parent panel to get sidebar instance.
4. If further platform support is needed we should file a bug, provide a test case and implement it.
5. Built-in panels don't use the ToolSidebar widget consistently. For now only Inspector, WebConsole (and Scratchpad) are using it. Our goal should be to have the ToolSidbar in every panel to avoid set of hacks for other panels (can take a time and that's why I am not sure if we want to have this as a blocker)
6. We should start with the Console and Inspector panels since they already use ToolSidebar and presumably we don't need any hacks.

The patch:

1. The button is updated upon new "side-panel-loaded" event. I think it's wrong since that event is fired when side bar panel is loaded while the button should be updated when the side bar itself is opened.
2. You register an event handler for that event using 'once' and then using 'off' to remove. Note that 'once' causes the listener to be executed just once then the listener is auto-removed.

Honza

[fflorent]

Some goals/comments:

Right, thanks for the hints!

Our goal should be to have the ToolSidbar in every panel to avoid set of hacks for other panels (can take a time and that's why I am not sure if we want to have this as a blocker)

Sorry, I don't understand this part.

1. The button is updated upon new "side-panel-loaded" event. I think it's wrong since that event is fired when side bar panel is loaded while the button should be updated when the side bar itself is opened.

Yeah... Do you have an event in mind that should be used instead? (perhaps "show"?)

1. You register an event handler for that event using 'once' and then using 'off' to remove. Note that 'once' causes the listener to be executed just once then the listener is auto-removed.

Yes, but the event can be not handled when destroying the toolbox...

Florent

[janodvarko]

Sorry, I don't understand this part.

Not every built in panel is using the ToolSidebar widget. Some panels are re-implementing the side panels UI again and again, which makes life for extensions hard since we need to use different ways to overlay various implementations of the side bar. Of course things would get a lot easier if ToolSidebar is used by every panel and all APIs are nice and consistent. Extensions can use the same way for every panel.

It'll take a time to get these nice APIs. I also told Sebastian he can file some Bugzilla bugs about it (you might be interested in fixing them).

We should start with panels that use the ToolSidebar since it's the way to go and we can learn how to do things properly.

Yeah... Do you have an event in mind that should be used instead? (perhaps "show"?)

The button update should not be based on events fired by individual panels. It should use events fired by the sidebar itself, i.e. the show and hide events described above.

Yes, but the event can be not handled when destroying the toolbox...

Not sure what you mean by "can be not handled", but as mentioned above the event is coming from a side panel (and btw. can fire multiple times if there are multiple side panels) and not the side bar itself, so this part should be removed anyway.

Honza

[fflorent]

May I continue working on it? (maybe more important than the unit-test of issue #143?)

Florent

[janodvarko]

Finishing the test should have higher priority atm. (but then you can definitely continue on this one!)

Honza

[fflorent]

Right. Continuing right now (I am in vacation this day ;) ).

Florent

[janodvarko]

(I am in vacation this day ;) ).

Ah, you are one lucky Frenchman :-)

Honza

[fflorent]

It's not a rumor that it happens the French work :).

Florent