Display current project name in main window

[le717]

Supersedes #6878.

This implements #2281, displaying the project name in the Brackets window title.

[marcelgerber]

@le717 This code is better:

    $(ProjectManager).on("projectOpen", function (e, projectRoot) {
        _projectName = projectRoot.name;
        updateTitle();
    });

[le717]

@SAplayer That worked! After looking more closely at it, I found out _projectName was not getting updated even though the project name was being retrieved. I didn't think about calling updateTitle() after getting it (although that makes sense). Thanks!

The only thing left now would be this split-second display of null on load.

However, because it appears for such a short time, it might be acceptable.

@peterflynn Any chance this could get in Release 40?

[marcelgerber]

I guess initializing _projectName = ""; would be a little bit more subtle, but the perfect solution is, of course, making the title context-specific.

You could probably try this:

_projectName = _projectName.replace(/^\s*(-|\u2014)/, "").trim();

[le717]

@SAplayer I tried setting it to an empty string, but then I got that blank dash in front of Brackets (which you regex would fix). I'm just trying to think where the regex would go in the code if the _projectName was initialized as an empty string. I'm thinking right before the project name is set?

[marcelgerber]

Probably like this:

    $(ProjectManager).on("projectOpen", function (e, projectRoot) {
        _projectName = projectRoot.name;
        _projectName = _projectName.replace(/^\s*[\-\u2014]\s*/, "");
        updateTitle();
    });

[marcelgerber]

Forget what I said.

if (_projectName === "") {
  windowTitle = windowTitle.replace(/^\s*[\-\u2014]\s*/, "");
}
window.document.title = windowTitle;

Tested.

[le717]

@SAplayer Oh, come on! I had that exact same code (save the regex) in place before, but I removed it long before I committed (wasn't sure if it was what @peterflynn was talking about). I didn't think that code snippet from last night was what was needed, but I know this new one would work even is you had not tested it. I'll make the change ASAP (on mobile right now).

[marcelgerber]

Set it to "" in order to make the code below work.

[le717]

*facepalm* Fixed.

[le717]

This PR should (hopefully) finally be complete, with all tests passing. @peterflynn Ready for review again.

Thanks for all the help, @SAplayer. I probably wouldn't have been able to finish this until later if it were not for you. :)

[redmunds]

Changed Milestone to Release 41.

[le717]

@peterflynn Do you still want to review this? I just rebased it and merged with master, so it's all ready to pull. If I need, I will try to fix the whitespace changes. That was from a force-of-habit keyboard shortcut action when I started this.

[le717]

@peterflynn Is this feature still wanted in Brackets? I can post a quick screenshot of the final version if needed.

[ingorichter]

@le717 Would you mind updating the screenshot? That would be great.
@peterflynn I guess we still want to have this feature added, correct?

[le717]

@ingorichter Quick check, what screenshot do you refer to? Yes, I'll get this PR ready to be merged again, no problem there.

[le717]

@ingorichter

With working file

Without working file

Is that what you meant?

EDIT: Updated, ready to be merged, Travis build is clean.

[ingorichter]

I think the new format should be available for the Mac too. Currently it's missing and the tests are failing.

[marcelgerber]

I'd do OS differentiation like this:

var osDash = brackets.platform === "mac" ? "\u2014" : "-";
var WINDOW_TITLE_STRING_NO_DOC = "{0} " + osDash + " {1}";
var WINDOW_TITLE_STRING_DOC = "{0} ({1}) " + osDash + " {2}";

[le717]

Oh shoot, I didn't realize this was not occurring on Mac.
Before I update, my question would be why different dashes are used on Win and Mac. OS app convention/requirement? Why not use the same dash on Win, Mac, and Linux?

[ingorichter]

I don't know. But what I heart is, that \2014 is the default delimiter on OSX and - for all other platforms.

[le717]

I don't see why we can't use \2014 on all platforms then. It's just a tad longer than -. *sigh* Will fix.

[peterflynn]

@le717 We decided originally we want to match each platform's standard separator, so can you please keep the split? \u2014 looks weird on Windows (doesn't match any other app), and - looks weird on Mac (same reason). We want to feel as much like native code editors as possible.

[le717]

Fully understand that native app feeling. Just trying to understand the reasoning behind the difference. I was going to keep the split dashes anyway.

[le717]

Fixed.

[peterflynn]

It'd be cleaner to move this initializer down into an else with the other case below.

[le717]

Done.

[le717]

@ingorichter @peterflynn @marcelgerber Changes pushed. Project title should now work on Mac (with appropriate dashes), refactoring completed, and null issues I keep hitting should be fixed now.

[peterflynn]

We can never get here -- if ProjectManager.getProjectRoot() is null, we'll crash up on line 149 before getting this far. We should either remove this else or move the projectName var down inside the if (projectRoot) block here.

[le717]

I'm in favor of moving it into if (projectRoot).

[ingorichter]

I never ended up in the else branch. Perhaps you can initialize windowTitle with brackets.config.app_title before the if (projectRoot) statement and get rid of the else branch entirely (only to initialize it with something valid).

[le717]

@ingorichter Sounds good. It looks like it was that way in case projectRoot was undefined or null, but initializing it as brackets.config.app_title is a better method.

[le717]

@peterflynn Changes pushed.

[ingorichter]

nit: you can reuse projectRoot here instead of calling ProjectManager.getProjectRoot again.

[le717]

Fixed.

[marcelgerber]

JSHint:

Linting test/spec/DocumentCommandHandlers-test.js...ERROR
[L44:C20] W098: 'StringUtils' is defined but never used.
        StringUtils              = require("utils/StringUtils");

[le717]

@marcelgerber Fixed just as you posted that.

[le717]

Changes pushed, ready for another look.

[peterflynn]

@le717 Just needs a merge with master so you're in sync with the new way of registering event listeners, and then I think this is good to go!

And actually if you could squash/rebase after doing your merge with master, that would be ideal -- there are a lot of commits here and many of them are redundant merge commits.

[le717]

@peterflynn Certainly for both merge and squash. Will get that ready ASAP. :)

[le717]

@peterflynn Merged and rebased. Travis is running right now. Unless it reports something, this is ready to be merged. :D

[peterflynn]

Great, all set! Thanks for the typo/spelling fixes too btw :-)

[le717]

[redmunds]

Argh! All of the unnecessary whitespace changes in this file (and others) are making my life difficult. I'm trying to keep my promises-upgrade branch up-to-date and every one of these whitespace changes that coincide with any change in my branch causes a phantom merge conflict that needs to be manually resolved. Please stop doing this! Brackets has no convention for whitespace in empty lines -- it's left to the authors discretion to have none or match indent level of previous/next line of code, so there's nothing to "fix".

Sorry @le717 I don't mean to single you out -- I'm just using your code as an example. This comment is meant for everyone: contributors and committers.

[le717]

I felt bad about these whitespaces things when this was finally merged. This was my first PR, and the whitespace was removed automatically. I kept considering adding it back, but it was mentioned or asked for. If someone had asked for it to be restored, I would have gladly done so. I've been trying to watch whitespace removal since this.

It's fine, I know how it is to be frustrated by changes like this. ;)

[redmunds]

The team has gone back and forth about whether to accept these changes, but I think this is a good reason to no longer accept them.

[marcelgerber]

@redmunds Pro Tip: Use git merge -X ignore-all-space
Source: http://stackoverflow.com/a/9784089/3455922 (haven't tested it, though)

[redmunds]

@marcelgerber Thanks for the tip! I'll try to remember that one.