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

Display current project title in main window #6878

Closed
wants to merge 11 commits into from
Closed

Display current project title in main window #6878

wants to merge 11 commits into from

Conversation

le717
Copy link
Contributor

@le717 le717 commented Feb 14, 2014

I was feeling adventurous, so I grabbed a copy of the Brackets source and worked on #2281. Jasmine tests are currently running but appear to be all good. For some reason, comma separating the code always broke the window title completely, so I had to
concentrate it to _currentTitlePath.

@@ -132,7 +132,7 @@ define(function (require, exports, module) {

// build shell/browser window title, e.g. "• file.html — Brackets"
if (currentlyViewedPath) {
windowTitle = StringUtils.format(WINDOW_TITLE_STRING, _currentTitlePath, windowTitle);
windowTitle = StringUtils.format(WINDOW_TITLE_STRING, _currentTitlePath + " (" + _.escape(ProjectManager.getProjectRoot().name + ")"), windowTitle);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't escape here -- window.title doesn't need it, so you'll wind up seeing the raw entity codes.

@peterflynn
Copy link
Member

Looks good otherwise though!

@le717
Copy link
Contributor Author

le717 commented Feb 14, 2014

@peterflynn Fixed. What about the concentration? Should it be that way considering the other items are comma-separated?

@le717
Copy link
Contributor Author

le717 commented Feb 14, 2014

BTW, the tests finally completed. It says there were two failures in DocumentCommandHandlers: "should report clean immediately after opening a file" and "should report dirty when modified". Did my changes cause these to occur?

@peterflynn
Copy link
Member

  • Yes, there are 4 unit tests there that expect a specific string for the document.title value. They'll probably all need to get adjusted.
  • We should include the project name even when no files are open. This would be the value that windowTitle is initialized to at the top of the function. (This affects the other 2 tests that aren't failing yet).
  • You're right that the string concatenation is not ideal. Can you fold it into WINDOW_TITLE_STRING so it's just another argument to the format() call? (Combined with the above bullet, this probably means adding a second string so we can use format() for the no-files-open case as well).

@le717
Copy link
Contributor Author

le717 commented Feb 14, 2014

@peterflynn Complete bullet 3 (I think...). I hack adjusted the two tests listed above for the title, and they are still failing because ProjectManager.getProjectRoot().name is returning null rather than the project name. This is also blocking bullet 2. Since it returns null, Brackets stops loading and it unusable. It would appear ProjectManager.getProjectRoot() is not "running" fast enough for both the tests and initialization to successfully pass/load.

@le717
Copy link
Contributor Author

le717 commented Feb 15, 2014

src/project/ProjectManager.js, lines 309-310:

Returns the root folder of the currently loaded project, or null if no project is open (during startup, or running outside of app shell).

That explains why it is null is returned when trying to display the title even when a file is not open. I have no idea what to do about that, though.

@marcelgerber
Copy link
Contributor

I think it should return the projectRoot even if no file is currently opened - you are still in a project.

@le717
Copy link
Contributor Author

le717 commented Feb 16, 2014

As do I for the same reason. But in order to do that, the code will have to be adjusted to not return null but rather the project directory (and I have no clue how to do that. I'm still working on fixing the currently broken unit tests...). Would be possible to initialize _projectRoot sooner or have Brackets wait for the non-null value to be returned before continuing on? But then the latter would mean a possible slow down in app starting.

@le717
Copy link
Contributor Author

le717 commented Feb 18, 2014

@peterflynn @SAplayer OK, I've fixed the first two broken unit tests the best I can (more on that in a second) and added a new string to display the title even when a file is not open.

However, because the projectRoot is not being properly returned, the code renders Brackets in an unusable state and prevents the unit tests from passing until a patch to fix the issue is provided. @peterflynn If you would like, you may go ahead and review the changes. I've left an example title in a comment. I haven't patched the remaining 2 tests, but I will once you are satisfied. :)

@le717
Copy link
Contributor Author

le717 commented Feb 25, 2014

@peterflynn or whoever sees this: I found a way to get the project root, but I only have one question: what is the ideal way to get the variable from the function's scope to the global namespace? Something to the extent of this perhaps?

var myVari = null;

function myFunc() {
    myVari = something.getItem();
}

Calling the function when needed would not be ideal because it would be called twice, once on load and again when needed. Adding it to the global namespace would be the ideal way so the function is only called once.

@le717
Copy link
Contributor Author

le717 commented Feb 25, 2014

Although the commit message might suggest something else, the project root is now displayed in the title no matter if a file is open or not. There are, however, still some issues.

  1. The initial value needed to be set to something other than null to avoid displaying that or an empty pair of parentheses for a split second while waiting for "projectOpen" (from ProjectManager) to fire.
  2. Opening a new project or switching to one without working files continues to display the previous project root or the initial project root value. Opening a file for editing updates it to the proper name, as does reloading/switching to another project.
  3. I have no idea how this can be extended to the unit tests.

Basically, this is an imperfect workaround for the null value ProjectManager.getProjectRoot() returns on startup and when there are no working files open. @peterflynn Would you mind taking a look at the current changes or having someone else do it so I know how to proceed? I've done all I can here, and fixing ProjectManager.getProjectRoot() is out of scope for me.

EDIT: The Travis CI error was caused by a line being off by one character. I'll fix it next push (whenever that is).

@@ -1588,6 +1600,7 @@ define(function (require, exports, module) {
CommandManager.registerInternal(Commands.APP_RELOAD_WITHOUT_EXTS, handleReloadWithoutExts);

// Listen for changes that require updating the editor titlebar
$(ProjectManager).on("projectOpen", _getProjectName);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be working as well:

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

/** @type {string} String template for window title. Use emdash on mac only. */
var WINDOW_TITLE_STRING = (brackets.platform !== "mac") ? "{0} - {1}" : "{0} \u2014 {1}";
/** @type {string} The current project name; displayed in window title. Set to app_title to avoid split-second display of null */
var _projectName = brackets.config.app_title;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's cleaner to leave the initial value null, and just have updateTitle() no-op if it's not set yet (since that means we're still in the middle of startup and no project is open yet).

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 have set the initial value to null, but I am a bit confused by "just have updateTitle() no-op if it's not set yet". Something to the effect of

if (_projectName === null) {
    _projectName = brackets.config.app_title;
}

Or did you have something else in mind?

@@ -930,7 +931,8 @@ define(function (require, exports, module) {
expect(DocumentManager.getCurrentDocument().isDirty).toBe(false);

// verify no dot in titlebar
var expectedTitle = (brackets.platform === "mac" ? ("test.js — " + brackets.config.app_title) : ("test.js - " + brackets.config.app_title));
var expectedTitleClean = (brackets.platform !== "mac" ? ("test.js — ({0}) - {1}") : ("test.js - ({0}) - {1}"));
var expectedTitle = StringUtils.format(expectedTitleClean, ProjectManager.getProjectRoot().name, brackets.config.app_title);
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to just hardcode the expected project name so the test will fail even if bugs in ProjectManager.getProjectRoot() break the title bar display.

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 did as you said and hard coded the expected project name.

@peterflynn
Copy link
Member

@le717 Done with another review pass. Sorry for the delay.

Also, looks like this needs to resolve some merge conflicts with master btw.

@le717
Copy link
Contributor Author

le717 commented Mar 20, 2014

Quick note: the edits to the tests are no good. They were from earlier revisions. I'll obviously listen to your review, but I will probably end up restoring the vanilla tests and restarting. ;)

The Windows GitHub client was having issues pulling down changes on my fork's master, so I haven't attempted updating this branch. I'll try to fix it from the command-line.

@ghost
Copy link

ghost commented Mar 28, 2014

@le717 Thanks so much for working on this! I hope this feature can get in before the next milestone? :)

Today (again) I'm trying to completely switch to Brackets, but I'm finding the lack of this feature a major inconvenience, as I need to work with several projects (switching between them often), so I have several project windows open, and it's really hard to navigate through Brackets' windows without the project name in their titles...

Again, thanks for fixing this...

@peterflynn
Copy link
Member

@DEVTIME Just fyi, launching multiple separate Brackets windows isn't a supported workflow yet. The windows don't communicate with each other, so you could wind up with conflicting unsaved changes, for example. So be very careful to not have any overlap in which files are open in the different windows.

@ghost
Copy link

ghost commented Mar 29, 2014

@peterflynn Thank you, Peter. I will keep that in mind.

@le717
Copy link
Contributor Author

le717 commented Mar 29, 2014

@peterflynn Changes per last review pushed. I am a bit confused if the on lines 327-937 is correct yet. Sorry about taking so long and the mess I made in the last commit. :\

@le717
Copy link
Contributor Author

le717 commented Apr 21, 2014

@peterflynn You can go ahead and review the latest changes. My fork is helplessly broken beyond repair for some reason, so I cannot rebase this with the master. Once we can get the code finalized, I will manually add the changes to the latest master and submit the code in a new PR. Hopefully we can get this in soon (I've been lagging on working on it. This PR has been up since Sprint 36!). :)

@le717
Copy link
Contributor Author

le717 commented May 8, 2014

@peterflynn I'm going to fix my fork and supersede this PR with a working (read: not broken on my end) PR for you. I'll try to have it up by this afternoon. ;)

@le717
Copy link
Contributor Author

le717 commented May 8, 2014

Superseded by #7789. @peterflynn It's ready for review as soon as you can.

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

Successfully merging this pull request may close these issues.

3 participants