Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Statusbar for Brackets #1717

Merged
merged 8 commits into from Oct 3, 2012

Conversation

Projects
None yet
5 participants
Member

jbalsas commented Sep 25, 2012

Adds a bottom status bar in the content area to show general editor settings and other status indicators. This is inspired by this user story https://trello.com/card/2-status-bar-bottom/4f90a6d98f77505d7940ce88/337

NOTES

I'm creating this pull request because I'm not sure about the right process and maybe I'm going a little over my head here... I think it still needs some work and discussion if you'd want to add it, so maybe it would be best if it could be forked and polished to reach a better state before trying to merge it.

INFORMATION

As suggested in the card, the status bar sits at the bottom of the editor and shows the line and column number, number of lines, mode and current tab setting for the current focused editor.

API

exports.showBusyIndicator = showBusyIndicator;
exports.hideBusyIndicator = hideBusyIndicator;

These APIs are a possible solution (at least partial) for the story in this card https://trello.com/card/ui-for-long-running-operatons/4f90a6d98f77505d7940ce88/551. The status bar includes a busy indicator that any module can show or hide through these methods. Additionally, showBusyIndicator takes one optional parameter to set a busy cursor over Brackets until hideBusyIndicator is called.

In this pull request, the Find in Files module has been updated to show the busy indicator and cursor while waiting for the search operation to complete.

exports.addIndicator = addIndicator;
exports.updateIndicator = updateIndicator;

These APIs allow any module to add an indicator on the status bar and update it at any time.

addIndicator takes an id (module.id could be nice) and other optional parameters such as indicator, visible, style, tooltip and command.

updateIndicator takes the same parameters as the other except indicator. It allows to update the values with new ones to change the status indicator appearance and behavior.

  • If a DOM element indicator is passed, it will be added to the status bar. Otherwise, an empty span will be used.
  • The visible parameter sets the initial indicator visibility
  • The style parameter indicates a class to style the indicator
  • The tooltip parameter is set as the indicator title so that it appears as a tooltip when the mouse is over it
  • The command parameter indicates a function to be called on click over the indicator. (It could be used to show/hide the JSLint errors panel decoupling this from the enable/disable action as outlined in https://groups.google.com/forum/?fromgroups=#!topic/brackets-dev/PqEbuhw7k6c

In this pull request, and just as a test for the API, the JSLintUtils module has been updated to add and update the indicator at the statusbar instead of at the toolbar. Also, the UpdateNotification module has been updated to show the gift icon as a status indicator.

KNOWN ISSUES

  • The style for the status bar and the busy indicator are in serious need of a makeover :)
  • The information fields were not fully working with inline editors, so I didn't take them into account for now. Is it possible that inline editors are not triggering the cursorActivity event?
  • Indicator commands are not implemented waiting further discussion.
  • The addIndicator and updateIndicator APIs could be improved for better flexibility.

Sorry for the long and booring post :)

PS: Although not yet finished, the status bar also comes in a Brackets Extension flavour https://github.com/jbalsas/brackets-statusbar-extension ;)

jbalsas added some commits Sep 24, 2012

@jbalsas jbalsas Show/Hide BusyIndicator API in StatusBar
Add/Update Indicator API in StatusBar
JSLintUtils indicator
UpdateNotificatino indicator
Find in files shows busyIndicator
3667786
@jbalsas jbalsas Merge branch 'master' of https://github.com/adobe/brackets into statu…
…sbar
ef2eed8
@jbalsas jbalsas Cleanup changes 37032e8
@jbalsas jbalsas More cleanup 3be18f4
Member

njx commented Sep 25, 2012

@jbalsas -- this is totally sweet. We literally just got out of our planning meetings for the next sprint, and one of the tasks we added was for a status bar :) We mainly wanted to add it to support adding a simple switcher for spaces vs. tabs, so what you've got could be a great start.

We'll try to do a closer review over the next few days and suggest improvements if any. Thanks!

Member

njx commented Sep 25, 2012

BTW, you can see a mockup that @GarthDB made for the status bar in the Extension UI Guidelines doc on the wiki: https://github.com/adobe/brackets/wiki/Extension-UI-Guidelines (search for the "Status Bar" section). Aside from the internal layout/positioning of the items in the status bar, the main differences are in the background color, font size and vertical padding. Probably worth tweaking those simple visual issues first--the internal layout stuff is probably a bigger question that's worth some discussion.

Member

GarthDB commented Sep 26, 2012

I'll try and take a look at the request and your current UI sometime tonight.

Sent from my iPhone

On Sep 25, 2012, at 4:56 PM, Narciso Jaramillo notifications@github.com wrote:

BTW, you can see a mockup that @GarthDB made for the status bar in the Extension UI Guidelines doc on the wiki: https://github.com/adobe/brackets/wiki/Extension-UI-Guidelines (search for the "Status Bar" section). Aside from the internal layout/positioning of the items in the status bar, the main differences are in the background color, font size and vertical padding. Probably worth tweaking those simple visual issues first--the internal layout stuff is probably a bigger question that's worth some discussion.


Reply to this email directly or view it on GitHub.

Member

njx commented Sep 26, 2012

The busy indicator is cool too. @GarthDB, do we have artwork for a spinner handy? (SVG preferably so we can scale it.) We could add a simple animation to rotate it, and start/stop it when we show/hide it.

Member

GarthDB commented Sep 26, 2012

There is a design done for reflow. I'll look into it.

Sent from my iPhone

On Sep 25, 2012, at 6:19 PM, Narciso Jaramillo notifications@github.com wrote:

The busy indicator is cool too. @GarthDB, do we have artwork for a spinner handy? (SVG preferably so we can scale it.) We could add a simple animation to rotate it, and start/stop it when we show/hide it.


Reply to this email directly or view it on GitHub.

@jasonsanjose jasonsanjose and 1 other commented on an outdated diff Sep 29, 2012

src/project/StatusBar.js
+
+ function _updateModeInfo() {
+ $modeInfo.text(fullEditor.getModeForSelection());
+ }
+
+ function _updateFileInfo() {
+ $fileInfo.text(fullEditor.lineCount() + " Lines");
+ }
+
+ function _updateTabCharInfo() {
+ $tabInfo.text("Tab Size: " + fullEditor._codeMirror.getOption("tabSize"));
+ }
+
+ function _updateCursorInfo() {
+ var cursor = fullEditor.getCursorPos(),
+ cursorInfo = "Line " + (cursor.line + 1) + ", " + "Column " + (cursor.ch + 1);
@jasonsanjose

jasonsanjose Sep 29, 2012

Member

Externalize string, Line {0}, Column {1}

@jasonsanjose

jasonsanjose Sep 29, 2012

Member

It seems like the cursor movement/statusbar updating feels a little laggy on my 2012 macbook air. Especially when scrolling down by holding the down arrow. I tried using webkitRequestAnimationFrame but that only helped slightly. Are you seeing any issues?

@jbalsas

jbalsas Sep 30, 2012

Member

It looks good to me... I'm on an old Macbook Pro (2007 I think...), with Mac OS X 10.7.4.

I've noticed it lags when I have some other panels such 'Find in Files' with many results opened, but then so does everything else...

@ghost ghost assigned jasonsanjose Sep 29, 2012

Member

jasonsanjose commented Sep 29, 2012

Reviewing

@jasonsanjose jasonsanjose commented on an outdated diff Sep 30, 2012

src/project/StatusBar.js
+ cursorInfo = "Line " + (cursor.line + 1) + ", " + "Column " + (cursor.ch + 1);
+
+ $cursorInfo.text(cursorInfo);
+ }
+
+ /**
+ * @private
+ * Updates the focused full editor and cleans listeners
+ */
+ function _onFocusedEditorChange(evt) {
+
+ if (fullEditor) {
+ $(fullEditor).off("cursorActivity", _updateCursorInfo);
+ }
+
+ fullEditor = EditorManager.getCurrentFullEditor();
@jasonsanjose

jasonsanjose Sep 30, 2012

Member

This probably has something to do with #1257, but we should put a comment here about supporting inline editors.

@jasonsanjose

jasonsanjose Oct 1, 2012

Member

I just saw the note in the description about supporting inline editors. It's still worth putting a comment here in the code as a reminder.

@jasonsanjose jasonsanjose commented on an outdated diff Sep 30, 2012

src/project/StatusBar.js
+ $cursorInfo,
+ $fileInfo,
+ $tabInfo,
+ $indicators,
+ $busyIndicator;
+
+ function _updateModeInfo() {
+ $modeInfo.text(fullEditor.getModeForSelection());
+ }
+
+ function _updateFileInfo() {
+ $fileInfo.text(fullEditor.lineCount() + " Lines");
+ }
+
+ function _updateTabCharInfo() {
+ $tabInfo.text("Tab Size: " + fullEditor._codeMirror.getOption("tabSize"));
@jasonsanjose

jasonsanjose Sep 30, 2012

Member

Externalize string, Tab Size: {0}

@jasonsanjose jasonsanjose commented on an outdated diff Sep 30, 2012

src/project/StatusBar.js
+ // below since they refer to DOM elements
+ var $editorContainer,
+ $statusBar,
+ $modeInfo,
+ $cursorInfo,
+ $fileInfo,
+ $tabInfo,
+ $indicators,
+ $busyIndicator;
+
+ function _updateModeInfo() {
+ $modeInfo.text(fullEditor.getModeForSelection());
+ }
+
+ function _updateFileInfo() {
+ $fileInfo.text(fullEditor.lineCount() + " Lines");
@jasonsanjose

jasonsanjose Sep 30, 2012

Member

Externalize string, {0} Lines

@jasonsanjose jasonsanjose commented on an outdated diff Sep 30, 2012

src/project/StatusBar.js
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, $, brackets, window, document*/
+
+/**
+ * A status bar with support for file information and busy and status indicators.
+ */
+define(function (require, exports, module) {
+ 'use strict';
+
+ var AppInit = require("utils/AppInit"),
+ Editor = require("editor/Editor"),
+ EditorManager = require("editor/EditorManager"),
+ ExtensionUtils = require("utils/ExtensionUtils");
+
+ // Current focused full editor
+ var fullEditor;
@jasonsanjose

jasonsanjose Sep 30, 2012

Member

What about inline editors? See comments below.

@jasonsanjose jasonsanjose commented on an outdated diff Sep 30, 2012

src/project/StatusBar.js
+ */
+ function hideBusyIndicator() {
+ // Check if we are using the busyCursor class to avoid
+ // unnecesary calls to $('*').removeClass()
+ if (busyCursor) {
+ busyCursor = false;
+ $("*").removeClass("busyCursor");
+ }
+
+ $busyIndicator.hide();
+ }
+
+ /**
+ * Registers a new status indicator
+ * @param {string} id Registration id of the indicator to be updated.
+ * @param {DOMNode) indicator Optional DOMNode for the indicator
@jasonsanjose

jasonsanjose Sep 30, 2012

Member

Typo close paren {DOMNode)

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/project/StatusBar.js
@@ -0,0 +1,199 @@
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

I think this module's location makes more sense under src/widgets instead of src/project.

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/language/JSLintUtils.js
@@ -136,9 +138,11 @@ define(function (require, exports, module) {
.append($errorTable);
$lintResults.show();
$goldStar.hide();
+ StatusBar.updateIndicator(module.id, true, "jslint-errors", JSLINT.errors.length + " JSLint errors");
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

New externalized strings 1 JSLint error and {0} JSLint errors

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/language/JSLintUtils.js
} else {
$lintResults.hide();
$goldStar.show();
+ StatusBar.updateIndicator(module.id, true, "jslint-valid", "No JSLint errors - good job!");
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

reuse externalized string Strings.JSLINT_NO_ERRORS

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/project/StatusBar.js
+ $("*").removeClass("busyCursor");
+ }
+
+ $busyIndicator.hide();
+ }
+
+ /**
+ * Registers a new status indicator
+ * @param {string} id Registration id of the indicator to be updated.
+ * @param {DOMNode) indicator Optional DOMNode for the indicator
+ * @param {boolean} visible Shows or hides the indicator over the statusbar.
+ * @param {string} style Sets the attribute "class" of the indicator.
+ * @param {string} tooltip Sets the attribute "title" of the indicator.
+ * @param {string} command Optional command name to execute on the indicator click.
+ */
+ function addIndicator(id, indicator, visible, style, tooltip, command) {
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

command parameter is unused. Add TODO here.

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/utils/UpdateNotification.js
@@ -286,6 +287,8 @@ define(function (require, exports, module) {
if (allUpdates) {
// Always show the "update available" icon if any updates are available
var $updateNotification = $("#update-notification");
+
+ StatusBar.addIndicator("update-notification", $updateNotification, true);
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

Why update-notification here instead of module.id?

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/styles/brackets.less
+ overflow: hidden;
+ background-color: lighten(@bc-grey, @bc-color-step-size*4);
+
+ .info {
+ float: left;
+ margin-left: 8px;
+ margin-top: 3px;
+
+ span {
+ color: @bc-grey;
+ padding-right: 15px;
+ vertical-align: middle;
+ }
+ }
+
+ #busy-indicator {
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

ID selectors should not be nested

@jasonsanjose jasonsanjose commented on the diff Oct 1, 2012

src/htmlContent/main-view.html
@@ -108,6 +108,18 @@
</div>
<div class="table-container"></div>
</div>
+
+ <div id="status-bar" class="statusbar">
+ <ul id="status-info" class="info" >
+ <span id="status-cursor"></span>
+ <span id="status-file"></span>
+ <span id="status-mode"></span>
+ <span id="status-tab"></span>
+ </ul>
+ <div id="busy-indicator">&#9719;</div>
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

@GarthDB will review the UI here. We won't need to fix anything immediately for this pull request.

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/language/JSLintUtils.js
@@ -148,6 +152,7 @@ define(function (require, exports, module) {
// both the results and the gold star
$lintResults.hide();
$goldStar.hide();
+ StatusBar.updateIndicator(module.id, true, "jslint-disabled", "JSLint disabled or not working for the current file");
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

new externalized string JSLint disabled or not working for the current file

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/project/StatusBar.js
+ } else {
+ $indicator.removeClass();
+ $indicator.addClass("indicator");
+ }
+
+ if (tooltip) {
+ $indicator.attr("title", tooltip);
+ }
+ }
+ }
+
+ /**
+ * @private
+ * Initialize the status bar and the focused editor status
+ */
+ function _initStatusBar() {
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

It seems like you can remove _initStatusBar and replace the call on line 192 with _onFocusedEditorChange().

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/project/StatusBar.js
+ if (!visible) {
+ $indicator.hide();
+ }
+
+ $indicators.prepend($indicator);
+ }
+
+ /**
+ * Updates a status indicator
+ * @param {string} id Registration id of the indicator to be updated.
+ * @param {boolean} visible Shows or hides the indicator over the statusbar.
+ * @param {string} style Sets the attribute "class" of the indicator.
+ * @param {string} tooltip Sets the attribute "title" of the indicator.
+ * @param {string} command Optional command name to execute on the indicator click.
+ */
+ function updateIndicator(id, visible, style, tooltip, command) {
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

Suggested refactoring. @jbalsas Don't worry about making these changes. We'll refactor this ourselves later.

We should factor out a StatusIndicator class here and replace addIndicator with createIndicator. StatusIndicator will define methods to update visibility, style, etc.

@jasonsanjose jasonsanjose commented on an outdated diff Oct 1, 2012

src/project/StatusBar.js
+ }
+
+ fullEditor = EditorManager.getCurrentFullEditor();
+
+ $(fullEditor).on('cursorActivity', _updateCursorInfo);
+ _updateCursorInfo();
+ _updateModeInfo();
+ _updateFileInfo();
+ _updateTabCharInfo();
+ }
+
+ /**
+ * Shows the 'busy' indicator
+ * @param {boolean} updateCursor Sets the cursor to "wait"
+ */
+ function showBusyIndicator(updateCursor) {
@jasonsanjose

jasonsanjose Oct 1, 2012

Member

Suggested refactoring. @jbalsas Again, just a suggestion for our team, nothing for you to fix for this pull request.

@GarthDB we might want to differentiate a background progress indicator that runs just in the status bar and a modal one that uses busyCursor. This current implementation with only a busyCursor does not prevent interactions with the page. I can still open menus, edit files, etc.

Member

jasonsanjose commented Oct 1, 2012

Initial review complete. Hopefully this is mostly just cleanup and not a ton of refactoring. Our goal is to merge this in as soon as possible to unblock the tabs/spaces story https://trello.com/c/exOQhrGw. If you're too busy to take on these code review changes, we can also help.

Thanks for the great contribution. The timing couldn't be better.

Member

jbalsas commented Oct 2, 2012

I'm attending tomorrows hackaton in London. Hopefully I'll be able to go through all the review changes there ;)

Member

jasonsanjose commented Oct 2, 2012

@jbalsas I'm not going to be in London, but please introduce yourself to the team that's there. Maybe @peterflynn, @njx or @gruehle will pick up the review or I can finish it tomorrow as well.

Member

gruehle commented Oct 3, 2012

Looks great! Thanks for the contribution. Merging.

@gruehle gruehle added a commit that referenced this pull request Oct 3, 2012

@gruehle gruehle Merge pull request #1717 from jbalsas/statusbar
Statusbar for Brackets
4c7b775

@gruehle gruehle merged commit 4c7b775 into adobe:master Oct 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment