StatusBar.addIndicator() doesn't actually add the indicator

[njx]

Originally, when StatusBar.addIndicator() was implemented, it would actually add the indicator into the status bar. However, in d7bce2c, that line was removed, which is a breaking API change. It's not clear why it was removed.

(Edited 12/14/2013 by @lkcampbell to add repro steps)

OS Version: Mac OSX Mavericks

Brackets Version: Sprint 34

Repro Steps:
Open Brackets source code, then open Dev Tools to Console and paste the following code snippet into the Console:

var StatusBar = require("widgets/StatusBar"),
    $foo = $("<div>FOO</div>");
StatusBar.addIndicator("foo", $foo, true, "", "foo tooltip");

Look at the right side of the editor status bar.

In the Dev Tools, Elements tab, look at the contents of status-indicators div tag.

Observed Result:
The FOO div does not show up in the status bar and is not in the status-indicators div tag

Expected Result:
The FOO div shows up in the status bar and is in the status-indicators div tag

[njx]

@larz0 - do you know why you removed this?

[larz0]

@njx I think it was because it was inserting something else that's not supposed to be there. I can't remember what though. Could it be related to JSLint indicator?

[larz0]

@njx I put it back in on this branch https://github.com/adobe/brackets/tree/larz/issue-5682 and there are no visible changes.

[lkcampbell]

Assigning this issue to myself to fix, I need resolved so I can address issue #5078.

[lkcampbell]

@njx, question for you. What repro steps are we using to test it and make sure I fixed it? This is the second issue I have run into where I need to create test steps for a Brackets module and I have no idea how to do it.

Do you guys have some way to conduct a headless test on some specific piece of the Brackets API?

[lkcampbell]

Edited the description to include repro steps.

[lkcampbell]

@njx and @larz0 , this problem is caused by what @njx mentioned above: the $indicators.prepend($indicator); line was removed from StatusBar.addIndicator().

But I also discovered the reason why @larz0 pulled this code in the first place.

This is what the status bar indicators look like normally:

This is what the status bar indicators look like after inserting the prepend line again:

The problem is addIndicator() doesn't have a way to indicate relative position. The default behavior is to push indicators onto the right side. It is a little counterintuitive because children of the status-indicators div are drawn right to left. That is to say, the first child is drawn on the rightmost side and the last child in drawn on the leftmost side.

We must have some special code that sets the JSLint indicator position manually right now. Reactivating the default behavior of addIndicator() overrides it.

My question is, how do you want me to fix this problem? What is the expected behavior? If the JSLint indicator is to remain in the middle, I may have to add relative positioning to the addIndicator() method interface. Any other ideas?

[lkcampbell]

@njx or @redmunds, Randy was asking about the status of the "Reload Without User Extensions" feature today. This issue is one of the blocking issues for the feature. I am looking for feedback from you regarding the best way to address this problem. Please see my questions above.

[njx]

Sorry, forgot to follow up. A few thoughts:

• I think it makes sense to add indicators from right-to-left, since these indicators are right-justified. So addIndicator() should add the new indicator to the left side of the group.
• I'm okay adding a relative position to addIndicator() (e.g. by ID), though I think we should really only use this for our core features. Third-party extensions shouldn't care too much about where they get added.

[lkcampbell]

@njx and @redmunds, fix committed.

[redmunds]

FBNC back to @njx

[njx]

Haven't tried it myself but the code looks sensible. Closing.

[peterflynn]

@lkcampbell @redmunds Do you think this is clean enough to be an official API for use by extensions now? If so, would one of you mind adding it to the release notes as a new API?

[redmunds]

@peterflynn Done.