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

Cleaner fix for #2761 (Project tree selection incorrect after opening file by means other than sidebar) #3398

Merged
merged 1 commit into from
Apr 10, 2013
Merged

Cleaner fix for #2761 (Project tree selection incorrect after opening file by means other than sidebar) #3398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions src/project/ProjectManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,17 +195,32 @@ define(function (require, exports, module) {
function _documentSelectionFocusChange() {
var curDoc = DocumentManager.getCurrentDocument();
if (curDoc && _hasFileSelectionFocus()) {
$("#project-files-container li").is(function (index) {
var entry = $(this).data("entry");
if (entry && entry.fullPath === curDoc.file.fullPath && !_projectTree.jstree("is_selected", $(this))) {
//we don't want to trigger another selection change event, so manually deselect
//and select without sending out notifications
_projectTree.jstree("deselect_all");
_projectTree.jstree("select_node", $(this), false);
var nodeFound = $("#project-files-container li").is(function (index) {
var $treeNode = $(this),
entry = $treeNode.data("entry");
if (entry && entry.fullPath === curDoc.file.fullPath) {
if (!_projectTree.jstree("is_selected", $treeNode)) {
if ($treeNode.parents(".jstree-closed").length) {
//don't auto-expand tree to show file - but remember it if parent is manually expanded later
_projectTree.jstree("deselect_all");
_lastSelected = $treeNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the comment in this block, I tried this:

  1. Selected a Find in Files result for file in a collapsed folder
  2. File is correctly shown in editor and there's no visible selection in file tree
  3. Manually expanded folder that contains the file in editor

Result:
Folder expands. File is not visibly selected.

Expected:
File in editor to be selected in file tree once it becomes exposed

Should that be expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, not ideal... but would you mind spinning that off as a separate bug? I fixed one similar bug as part of this pull request (see cases in unit tests), but I think these are basically separate issues from #2761.

#2761 was about a much more confusing issue -- a file is highlighted in the tree but it's not the file shown in the editor. In this case we just leave the folder highlighted, which is far less likely to be confusing. (And we already have a number of cases where a folder is even expected to be highlighted in the tree instead of the file that's open, so it's probably something users will have seen before).

} else {
//we don't want to trigger another selection change event, so manually deselect
//and select without sending out notifications
_projectTree.jstree("deselect_all");
_projectTree.jstree("select_node", $treeNode, false); // sets _lastSelected
}
}
return true;
}
return false;
});

// file is outside project subtree, or in a folder that's never been expanded yet
if (!nodeFound) {
_projectTree.jstree("deselect_all");
_lastSelected = null;
}
} else if (_projectTree !== null) {
_projectTree.jstree("deselect_all");
_lastSelected = null;
Expand Down Expand Up @@ -915,11 +930,8 @@ define(function (require, exports, module) {
function showInTree(entry) {
return _findTreeNode(entry)
.done(function ($node) {
_projectTree.jstree("deselect_node", _lastSelected);
_lastSelected = null;
_projectTree.jstree("select_node", $node);
_lastSelected = $node;
_redraw(true, true);
// jsTree will automatically expand parent nodes to ensure visible
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment: "jsTree will automatically expand parent nodes to ensure visible". Isn't that what your fix is trying to avoid ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's restoring the previous code. Maybe that comment is incorrect?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's correct -- that is what this code does, but this code is no longer invoked when you click a Find in Files result (see deletion in FindInFiles.js). This is now only invoked by the 'Show in File Tree' command.

_projectTree.jstree("select_node", $node, false);
});
}

Expand Down
1 change: 0 additions & 1 deletion src/search/FindInFiles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,6 @@ define(function (require, exports, module) {
.done(function (doc) {
// Opened document is now the current main editor
EditorManager.getCurrentFullEditor().setSelection(match.start, match.end, true);
ProjectManager.showInTree(doc.file);
});
});
resultsDisplayed++;
Expand Down
137 changes: 135 additions & 2 deletions test/spec/ProjectManager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,32 @@


/*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */
/*global define: false, require: false, describe: false, it: false, expect: false, beforeEach: false, afterEach: false, waitsFor: false, runs: false */
/*global $, define, require, describe, it, expect, beforeEach, afterEach, waitsFor, runs, waitsForDone */
define(function (require, exports, module) {
'use strict';

// Load dependent modules
var ProjectManager, // Load from brackets.test
CommandManager, // Load from brackets.test
Commands = require("command/Commands"),
SpecRunnerUtils = require("spec/SpecRunnerUtils");

describe("ProjectManager", function () {

this.category = "integration";

var testPath = SpecRunnerUtils.getTestPath("/spec/ProjectManager-test-files"),
testWindow,
brackets;

beforeEach(function () {
SpecRunnerUtils.createTestWindowAndRun(this, function (testWindow) {
SpecRunnerUtils.createTestWindowAndRun(this, function (w) {
testWindow = w;

// Load module instances from brackets.test
brackets = testWindow.brackets;
ProjectManager = testWindow.brackets.test.ProjectManager;
CommandManager = testWindow.brackets.test.CommandManager;
});
});

Expand Down Expand Up @@ -171,6 +177,133 @@ define(function (require, exports, module) {
});
});

describe("Selection indicator", function () {

function expectSelected(fullPath) {
var $projectTreeItems = testWindow.$("#project-files-container > ul").children();
var $selectedItem = $projectTreeItems.find("a.jstree-clicked");
if (!fullPath) {
expect($selectedItem.length).toBe(0);
} else {
expect($selectedItem.length).toBe(1);
expect($selectedItem.parent().data("entry").fullPath).toBe(fullPath);
}
}

it("should deselect after opening file not rendered in tree", function () {
SpecRunnerUtils.loadProjectInTestWindow(testPath);
var promise,
exposedFile = testPath + "/file.js",
unexposedFile = testPath + "/directory/file.js";

runs(function () {
promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: exposedFile });
waitsForDone(promise);
});
runs(function () {
expectSelected(exposedFile);

promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: unexposedFile });
waitsForDone(promise);
});
runs(function () {
expectSelected(null);
});
});


function findExtantNode(fullPath) {
var $treeItems = testWindow.$("#project-files-container li"),
$result;
$treeItems.is(function () {
var $treeNode = testWindow.$(this),
entry = $treeNode.data("entry");
if (entry && entry.fullPath === fullPath) {
$result = $treeNode;
return true;
}
return false;
});
return $result;
}
function toggleFolder(fullPath, open) {
var $treeNode = findExtantNode(fullPath);

var expectedClass = open ? "jstree-open" : "jstree-closed";
expect($treeNode.hasClass(expectedClass)).toBe(false);

$treeNode.children("a").click();

// if a folder has never been expanded before, this will be async
waitsFor(function () {
return $treeNode.hasClass(expectedClass);
}, (open ? "Open" : "Close") + " tree node", 1000);
}

it("should reselect previously selected file when made visible again", function () {
SpecRunnerUtils.loadProjectInTestWindow(testPath);
var promise,
initialFile = testPath + "/file.js",
folder = testPath + "/directory/",
fileInFolder = testPath + "/directory/file.js";

runs(function () {
promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: initialFile });
waitsForDone(promise);
});
runs(function () {
expectSelected(initialFile);
toggleFolder(folder, true); // open folder
});
runs(function () { // open file in folder
promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: fileInFolder });
waitsForDone(promise);
});
runs(function () {
expectSelected(fileInFolder);
toggleFolder(folder, false); // close folder
});
runs(function () {
expectSelected(folder);
toggleFolder(folder, true); // open folder again
});
runs(function () {
expectSelected(fileInFolder);
});
});

it("should deselect after opening file hidden in tree, but select when made visible again", function () {
SpecRunnerUtils.loadProjectInTestWindow(testPath);
var promise,
initialFile = testPath + "/file.js",
folder = testPath + "/directory/",
fileInFolder = testPath + "/directory/file.js";

runs(function () {
promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: initialFile });
waitsForDone(promise);
});
runs(function () {
expectSelected(initialFile);
toggleFolder(folder, true); // open folder
});
runs(function () {
toggleFolder(folder, false); // close folder
});
runs(function () { // open file in folder
promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: fileInFolder });
waitsForDone(promise);
});
runs(function () {
expectSelected(null);
toggleFolder(folder, true); // open folder again
});
runs(function () {
expectSelected(fileInFolder);
});
});
});

describe("File Display", function () {
it("should not show useless directory entries", function () {
var shouldShow = ProjectManager.shouldShow;
Expand Down