Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

console error indicator for status bar #7639

Merged
merged 15 commits into from
Apr 30, 2014
Merged

console error indicator for status bar #7639

merged 15 commits into from
Apr 30, 2014

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Apr 25, 2014

Old Subject: error icon for main-toolbar

This is something I've made out of need when working with Brackets especially on non-brackets projects (when I don't have my Brackets developer console open) to see if something wrong happened inside Brackets immediately.

From my experience many users don't use console to look for errors before reporting an issue so having a red icon appearing on the main toolbar might help also to non-developers.

Leaving to your consideration if this's a good idea for a default extension, or if I should move it to the extension registry. I'm fine with both.

Looks like this:
image

ExtensionUtils.loadStyleSheet(module, "style.css");

function showDeveloperTools() {
CommandManager.execute("debug.showDeveloperTools");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A default extension shouldn't rely on another default extension.
I don't know how to work around this, but maybe at least a try {} catch {}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried and no exception or console error is raised when you execute a command string that isn't registered. So no need to try-catch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break if the other extension is not installed or disabled (eventually we'd like to add ability to disable extensions including default extensions). You should be able to call brackets.app.showDeveloperTools() directly.

if (_windowOnError) {
return _windowOnError(errorMsg, url, lineNumber);
}
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does return false signify anything for this event? We want to be careful not to prevent the dev tools 'break on uncaught errors' feature from working correctly...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return false means that we didn't handle this error and it should run the default handler

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers.onerror

@redmunds
Copy link
Contributor

@zaggino This is pretty cool. We talked about it in the team meeting today, and would need to see the following changes for this to go into master:

  • Move it to the status bar which is a more appropriate place for this
  • While this is great for devs, we're concerned that this could be disconcerting to users that aren't Dev Tools savvy, so it should have a Debug menu command to toggle it on/off and the default is off.
  • This seems to fit with DebugCommands, so merge it there

Of course, you're welcome to keep it as it is and release it as an extension, but this would be great to have in core Brackets.

At one time, Brackets had an option to "open dev tools on exception". It would be nice to have something like that again. This would also need to have toggle switch. If you want to try to get this working, be sure to test case when dev tools window is already open, the existing window should be re-used (and optimally brought to front) instead of opening new window (which was a problem with original feature).

@zaggino
Copy link
Contributor Author

zaggino commented Apr 26, 2014

@redmunds take a look now please if this's what you've meant
image

@redmunds
Copy link
Contributor

@zaggino Nice! We'll have to get @larz0 to give it a UX review.

Seems like it should get inserted at left end (of the status bar indicators on the right side) so all other indicators are not shifted when it's toggled on/off.

Not sure if you have access to a Windows system, but hit the Insert key and watch how the INS/OVR indicator does a single pulse when it changes. That might help you notice when error number is incremented (but it might have to be debounced in case where several errors come in very quickly).

I like how clicking on the status bar indicator opens Dev Tools. The mouse pointer should change to a hand in the hover state.

It might be nice to have a way to clear the count. I could see wanting to do that if you've looked at the console log and understand all the errors, then you could clear the count so it's easier to tell when new errors happen.

@marcelgerber
Copy link
Contributor

You could also event console.clear() to reset the count.

@larz0
Copy link
Member

larz0 commented Apr 28, 2014

@zaginno I agree with @redmunds that it should be on the left end. I can't get the error count to show in the status bar for some reason despite having errors in dev tools.

From the screenshot you provided looks like we could use a space after ":".

@marcelgerber
Copy link
Contributor

@larz0 He already added the space in 24be428

@larz0
Copy link
Member

larz0 commented Apr 28, 2014

@SAplayer ahh awesome!

@redmunds
Copy link
Contributor

@larz0 In the Debug menu, toggle it on with "Show Errors in Status Bar".

I wrote a simple extension that adds a "Randy Test" item to Debug menu and causes an exception every time you click it:

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50, regexp: true */
/*global define, brackets, $, window */

define(function (require, exports, module) {
    "use strict";

    var CommandManager      = brackets.getModule("command/CommandManager"),
        Menus               = brackets.getModule("command/Menus");

    function randyTest() {
        // Force an exception
        Menus.nonExistentFunction();
    }

    CommandManager.register("Randy Test", "randy-test", randyTest);

    Menus.getMenu("debug-menu").addMenuItem("randy-test");
});

@zaggino
Copy link
Contributor Author

zaggino commented Apr 28, 2014

@larz0 have you turned it on in Debug menu? It won't show without it. I'll move it to the left.

Btw is it a good thing that status bar dissapears when viewing images or no file is open?

@redmunds
Copy link
Contributor

@zaggino The current design is to only show status bar for files being edited (not viewed). Not sure if we've considered any other designs.

@larz0
Copy link
Member

larz0 commented Apr 28, 2014

@redmunds thanks that worked.

@zaggino we can show the status bar in image view if there's a need for it. Right now the info we get above the image is enough.

@zaggino
Copy link
Contributor Author

zaggino commented Apr 28, 2014

comments implemented, you can clear now with console.clear as suggested by @SAplayer

}

function attachFunctions() {
if (_attached) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format for this should be:

        if (_attached) {
            return;
        }

Also line 137.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by b081a1a

@redmunds
Copy link
Contributor

@larz0 This is ready for another review.

@larz0
Copy link
Member

larz0 commented Apr 29, 2014

@zaggino @redmunds looks good! Just realized that we should probably add a tooltip that says something along the lines of "Show Errors in Dev Tools…".

@zaggino
Copy link
Contributor Author

zaggino commented Apr 29, 2014

It won't navigate you exactly to the errors so just "Show Developer Tools" would probably be more accurate description of the click action?

@larz0
Copy link
Member

larz0 commented Apr 29, 2014

@zaggino sounds good. Remember the "…" because it goes to a different window.

@zaggino
Copy link
Contributor Author

zaggino commented Apr 29, 2014

Yes, added.
image

@larz0
Copy link
Member

larz0 commented Apr 29, 2014

@zaggino looks good!

@redmunds review complete.

@njx
Copy link
Contributor

njx commented Apr 29, 2014

This is really cool. I had a UI idea: what if instead of blinking the number, we did a background fade like the INS/OVR status bar field does? i.e., set the background to red and fade it out. I could imagine doing the same thing for the JSLint icon too - when the panel is closed I often don't notice when the icon turns yellow.

@larz0 what would you think of that? It would make those status bar indicators all very consistent with each other, which I find very appealing.

@peterflynn pointed out that the main issue with this is that if the text is red as well as the initial background color, then you wouldn't be able to read it at first - so we'd need to pick a background color that wasn't exactly the same as the text (or the same yellow as the warning icon in the JSLint case).

@njx
Copy link
Contributor

njx commented Apr 29, 2014

Also @zaggino - we talked about making it so the error indicator shows errors even from before you toggle it on - would that be possible?

@redmunds redmunds changed the title error icon for main-toolbar console error indicator for status bar Apr 29, 2014
@larz0
Copy link
Member

larz0 commented Apr 29, 2014

@njx sounds good. We can use #ffb0cd, the same color we used for input error glow.

@zaggino
Copy link
Contributor Author

zaggino commented Apr 29, 2014

fade in/out done, @larz0 please take a look

@njx If you check it on in the Debug menu, it'll show errors immediately from the moment the default extension is loaded, even after Brackets restart - is that what you've meant?

It won't catch errors that happen before the debug extension is loaded, for that to happen we'd need to put the code to a different place, ideally before anything from brackets gets loaded.

@larz0
Copy link
Member

larz0 commented Apr 30, 2014

@zaggino it's not fading out for me. The flash class persists.

@zaggino
Copy link
Contributor Author

zaggino commented Apr 30, 2014

That's weird, I've tested it again and it works for me :-/
Can anybody else try/suggest what might be wrong?

@redmunds
Copy link
Contributor

@larz0 @zaggino The flash works correctly for me (Win7).

@larz0
Copy link
Member

larz0 commented Apr 30, 2014

@zaggino @redmunds my bad, it works. It was due to one of the extensions I have; haven't identified which extension though.

}

function refreshIcon() {
// never show 0 errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to do anything here if icon is toggled off, so maybe a quick check:

        if (!_attached) {
            if ($span) {
                $span.parent().hide();
            }
            return;
        }

@redmunds
Copy link
Contributor

This looks good. Just a couple last comments.

@zaggino
Copy link
Contributor Author

zaggino commented Apr 30, 2014

Fixed @redmunds

@redmunds
Copy link
Contributor

Thanks! Merging.

redmunds added a commit that referenced this pull request Apr 30, 2014
console error indicator for status bar
@redmunds redmunds merged commit 996899f into master Apr 30, 2014
@redmunds redmunds deleted the zaggino/error-icon branch April 30, 2014 23:24
@sixertoy
Copy link

screenshot
Hello :)

I'm looking for Menus.getMenu
But i found this issue, so... i've finished a logger panel based on brackets-console (see readme details) wich doesn't seems to be maintained anymore like another extension wich on is completly broken and unsable, so what i grab opportunity to share my own dev

https://github.com/malas34/brackets-console-plus

not yet in brackets repo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants