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

Working Set Performance Improvements #9493

Closed
wants to merge 13 commits into from
Closed

Working Set Performance Improvements #9493

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ac10e05
add some improvements
JeffryBooher Oct 8, 2014
35e7d58
fix focus performance
JeffryBooher Oct 8, 2014
a76e45e
remove _showAsSelected(wasn't adding anything)
JeffryBooher Oct 8, 2014
c898836
improve pane.focus, editormanager._showEditor, mvm._open/mvm._edit
JeffryBooher Oct 9, 2014
66c1b74
fix failing mvm test and bogus focus issue
JeffryBooher Oct 9, 2014
fda2251
rtc
JeffryBooher Oct 9, 2014
3fe01a3
@@ nit
JeffryBooher Oct 9, 2014
8f5560b
rtc
JeffryBooher Oct 9, 2014
42f2498
rework ghost drag position
JeffryBooher Oct 9, 2014
bf2c5f6
fix jitter on drag start
JeffryBooher Oct 9, 2014
ddd4418
from whence it came
JeffryBooher Oct 9, 2014
7092110
comment
JeffryBooher Oct 9, 2014
003badf
Merge branch 'master' into jeff/wsv-perf-improvements
JeffryBooher Oct 9, 2014
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
15 changes: 8 additions & 7 deletions src/editor/EditorManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,13 +470,15 @@ define(function (require, exports, module) {
* Semi-private: should only be called within this module or by Document.
* @param {!Document} document Document whose main/full Editor to create
* @param {!Pane} pane Pane in which the editor will be hosted
@ @return {!Editor}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: There is a @ instead of *.

*/
function _createFullEditorForDocument(document, pane) {
// Create editor; make it initially invisible
var editor = _createEditorForDocument(document, true, pane.$content);
editor.setVisible(false);
pane.addView(editor);
$(exports).triggerHandler("_fullEditorCreatedForDocument", [document, editor, pane.id]);
return editor;
}


Expand Down Expand Up @@ -535,32 +537,31 @@ define(function (require, exports, module) {
editor = document._masterEditor;

if (!editor) {
createdNewEditor = true;

// Performance (see #4757) Chrome wastes time messing with selection
// that will just be changed at end, so clear it for now
if (window.getSelection && window.getSelection().empty) { // Chrome
window.getSelection().empty();
}

// Editor doesn't exist: populate a new Editor with the text
_createFullEditorForDocument(document, pane);
} else if (editor.$el.parent() !== pane.$el) {
editor = _createFullEditorForDocument(document, pane);
createdNewEditor = true;
} else if (editor.$el.parent()[0] !== pane.$content[0]) {
// editor does exist but is not a child of the pane so add it to the
// pane (which will switch the view's container as well)
pane.addView(editor);
}

// show the view
pane.showView(document._masterEditor);
pane.showView(editor);

if (MainViewManager.getActivePaneId() === pane.id) {
// give it focus
document._masterEditor.focus();
editor.focus();
}

if (createdNewEditor) {
_restoreEditorViewState(document._masterEditor);
_restoreEditorViewState(editor);
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/project/FileViewController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,10 @@ define(function (require, exports, module) {
return;
}

_fileSelectionFocus = fileSelectionFocus;
$(exports).triggerHandler("fileViewFocusChange");
if (_fileSelectionFocus !== fileSelectionFocus) {
_fileSelectionFocus = fileSelectionFocus;
$(exports).triggerHandler("fileViewFocusChange");
}
}

/**
Expand Down
99 changes: 60 additions & 39 deletions src/project/WorkingSetView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ define(function (require, exports, module) {
"use strict";

// Load dependent modules
var DocumentManager = require("document/DocumentManager"),
var AppInit = require("utils/AppInit"),
DocumentManager = require("document/DocumentManager"),
MainViewManager = require("view/MainViewManager"),
CommandManager = require("command/CommandManager"),
Commands = require("command/Commands"),
Expand Down Expand Up @@ -69,6 +70,12 @@ define(function (require, exports, module) {
var _classProviders = [];


/**
* #working-set-list-container
* @type {jQuery}
*/
var $workingFilesContainer;

/**
* Constants for event.which values
* @enum {number}
Expand Down Expand Up @@ -129,7 +136,6 @@ define(function (require, exports, module) {
*/
function _updateListItemSelection(listItem, selectedFile) {
var shouldBeSelected = (selectedFile && $(listItem).data(_FILE_KEY).fullPath === selectedFile.fullPath);

ViewUtils.toggleClass($(listItem), "selected", shouldBeSelected);
}

Expand Down Expand Up @@ -263,19 +269,43 @@ define(function (require, exports, module) {
dragged = false,
startPageY = e.pageY,
lastPageY = startPageY,
itemHeight = $el.height(),
lastHit = { where: NOMANSLAND },
tryClosing = $(e.target).hasClass("can-close"),
offset = $el.offset(),
$copy = $el.clone(),
$ghost = $("<div class='open-files-container wsv-drag-ghost' style='overflow: hidden; display: inline-block;'>").append($("<ul>").append($copy).css("padding", "0")),
sourceView = _viewFromEl($el),
currentFile = MainViewManager.getCurrentlyViewedFile(),
activePaneId = MainViewManager.getActivePaneId(),
activeView = _views[activePaneId],
draggingCurrentFile = ($el.hasClass("selected") && sourceView.paneId === activePaneId),
startingIndex = MainViewManager.findInWorkingSet(sourceView.paneId, sourceFile.fullPath),
sourceView = _viewFromEl($el),
currentView = sourceView,
lastHit = { where: NOMANSLAND };
startingIndex = $el.index(),
itemHeight,
offset,
$copy,
$ghost,
draggingCurrentFile;

function initDragging() {
itemHeight = $el.height();
offset = $el.offset();
$copy = $el.clone();
$ghost = $("<div class='open-files-container wsv-drag-ghost' style='overflow: hidden; display: inline-block;'>").append($("<ul>").append($copy).css("padding", "0"));
draggingCurrentFile = ($el.hasClass("selected") && sourceView.paneId === activePaneId);

// setup our ghost element as position absolute
// so we can put it wherever we want to while dragging
if (draggingCurrentFile && _hasSelectionFocus()) {
$ghost.addClass("dragging-current-file");
}

$ghost.css({
top: offset.top,
left: offset.left,
width: $el.width() + 8
});

// this will give the element the appearence that it's ghosted if the user
// drags the element out of the view and goes off into no mans land
$ghost.appendTo($("body"));
}

// Switches the view context to match the hit context
function updateContext(hit) {
Expand All @@ -294,7 +324,6 @@ define(function (require, exports, module) {
hasScroller = false,
onTopScroller = false,
onBottomScroller = false,
$workingFilesContainer = $("#working-set-list-container"),
$container,
$hit,
$item,
Expand Down Expand Up @@ -564,7 +593,7 @@ define(function (require, exports, module) {
// The drag function
function drag(e) {
if (!dragged) {

initDragging();
// sort redraw and scroll shadows
// cause problems during drag so disable them
_suppressSortRedrawForAllViews(true);
Expand All @@ -590,6 +619,8 @@ define(function (require, exports, module) {
dragged = true;
}

$ghost.css("top", $ghost.offset().top + (e.pageY - lastPageY));

// reset the scrolling direction to no-scroll
scrollDir = 0;

Expand Down Expand Up @@ -654,9 +685,6 @@ define(function (require, exports, module) {
}
}

// move the drag affordance
$ghost.css("top", $ghost.offset().top + (e.pageY - lastPageY));

// if we have't started dragging yet then we wait until
// the mouse has moved 3 pixels before we start dragging
// to avoid the item moving when clicked or double clicked
Expand All @@ -681,19 +709,21 @@ define(function (require, exports, module) {

// Close down the drag operation
function preDropCleanup() {
$("#working-set-list-container").removeClass("dragging");
$("#working-set-list-container .drag-show-as-selected").removeClass("drag-show-as-selected");
endScroll($el);
// re-activate the views (adds the "active" class to the view that was previously active)
_deactivateAllViews(false);
// turn scroll wheel back on
window.onmousewheel = window.document.onmousewheel = null;
$(window).off(".wsvdragging");
$ghost.remove();
$el.css("opacity", "");
if (dragged) {
$("#working-set-list-container").removeClass("dragging");
Copy link
Contributor

Choose a reason for hiding this comment

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

$("#working-set-list-container") is already stored in $workingFilesContainer, so no need for another lookup.

$("#working-set-list-container .drag-show-as-selected").removeClass("drag-show-as-selected");
Copy link
Contributor

Choose a reason for hiding this comment

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

The lookup for $("#working-set-list-container .drag-show-as-selected") is faster if you use either $workingFilesContainer.find(".drag-show-as-selected") or $(".drag-show-as-selected", $workingFilesContainer).

endScroll($el);
// re-activate the views (adds the "active" class to the view that was previously active)
_deactivateAllViews(false);
// turn scroll wheel back on
$ghost.remove();
$el.css("opacity", "");

if (dragged && $el.next().length === 0) {
scrollCurrentViewToBottom();
if ($el.next().length === 0) {
scrollCurrentViewToBottom();
}
}
}

Expand Down Expand Up @@ -800,21 +830,7 @@ define(function (require, exports, module) {
return;
}

// setup our ghost element as position absolute
// so we can put it wherever we want to while dragging
if (draggingCurrentFile && _hasSelectionFocus()) {
$ghost.addClass("dragging-current-file");
}

$ghost.css({
top: offset.top,
left: offset.left,
width: $el.width() + 8
});

// this will give the element the appearence that it's ghosted if the user
// drags the element out of the view and goes off into no mans land
$ghost.appendTo($("body"));

e.stopPropagation();
});
Expand Down Expand Up @@ -1435,6 +1451,11 @@ define(function (require, exports, module) {
refresh(true);
}

AppInit.htmlReady(function () {
$workingFilesContainer = $("#working-set-list-container");
});


// Public API
exports.createWorkingSetViewForPane = createWorkingSetViewForPane;
exports.refresh = refresh;
Expand Down
51 changes: 32 additions & 19 deletions src/view/MainViewManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,9 +370,8 @@ define(function (require, exports, module) {
oldPaneId]);

_makePaneMostRecent(_activePaneId);
focusActivePane();
}

focusActivePane();
}

/**
Expand Down Expand Up @@ -1132,20 +1131,20 @@ define(function (require, exports, module) {
* @private
*/
function _edit(paneId, doc, optionsIn) {
var currentPaneId = _getPaneIdForPath(doc.file.fullPath),
options = optionsIn || {};


var options = optionsIn || {},
currentPaneId;

if (options.noPaneRedundcancyCheck) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: noPaneRedundcancyCheck.

Also need to add this option to jsdoc comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it's not clear how this prevents pane redundancy checking, so can you add a comment that gives a brief explanation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to also fix noPaneRedundcancyCheck typo in header/jsdos comment

currentPaneId = _getPaneIdForPath(doc.file.fullPath);
}

if (currentPaneId) {
// If the doc is open in another pane
// then switch to that pane and call open document
// which will really just show the view as it has always done
// we could just do pane.showView(doc._masterEditor) in that
// case but Editor Manager may do some state syncing
paneId = currentPaneId;
if (!options.noPaneActivate) {
setActivePaneId(paneId);
}
}

var pane = _getPane(paneId);
Expand All @@ -1159,6 +1158,10 @@ define(function (require, exports, module) {
// open document will show the editor if there is one already
EditorManager.openDocument(doc, pane);
_makeFileMostRecent(paneId, doc.file);

if (!options.noPaneActivate) {
setActivePaneId(paneId);
}
}

/**
Expand All @@ -1171,7 +1174,8 @@ define(function (require, exports, module) {
* rejects with a File error or string
*/
function _open(paneId, file, optionsIn) {
var result = new $.Deferred(),
var master = new $.Deferred(),
result = new $.Deferred(),
options = optionsIn || {};

if (!file || !_getPane(paneId)) {
Expand Down Expand Up @@ -1203,9 +1207,6 @@ define(function (require, exports, module) {
// we could just do pane.showView(doc._masterEditor) in that
// case but Editor Manager may do some state syncing
paneId = currentPaneId;
if (!options.noPaneActivate) {
setActivePaneId(paneId);
}
}

// See if there is already a view for the file
Expand All @@ -1227,25 +1228,37 @@ define(function (require, exports, module) {
if (!ProjectManager.isWithinProject(file.fullPath)) {
addToWorkingSet(paneId, file);
}
result.resolve(file);
master.resolve(file);
})
.fail(function (fileError) {
result.reject(fileError);
master.reject(fileError);
});
} else {
result.reject(fileError || FileSystemError.NOT_FOUND);
master.reject(fileError || FileSystemError.NOT_FOUND);
}
});
} else {
DocumentManager.getDocumentForPath(file.fullPath)
.done(function (doc) {
_edit(paneId, doc, options);
result.resolve(doc.file);
_edit(paneId, doc, {noPaneActivate: true,
noPaneRedundcancyCheck: true});
master.resolve(doc.file);
})
.fail(function (fileError) {
result.reject(fileError);
master.reject(fileError);
});
}

// when we finish opening the document
// then set the active pane if we opened it
master.done(function (file) {
if (!options.noPaneActivate) {
setActivePaneId(paneId);
}
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 think another Deferred needs to be introduced for this. Can't you just move this block:

            if (!options.noPaneActivate) {
                setActivePaneId(paneId);
            }

to just before result.resolve() above (which is now master.resolve()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to do this last after the document has been opened and the pane has changed its current file. This prevents unwanted flicker in the title bar. If we do this before the document has been opened and the current file changed in the pane then the activating the pane will cause the title bar to change again after opening the document

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 see how my suggestion changes the order of code execution. It should do the same thing, but with 1 less Deferred object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right but there are 2 code paths and 2 result.resolves. The master Deferred was just to reduce code duplication.
I refactored this to use fewer promises by using a function to activate the pane post open.

result.resolve(file);
}).fail(function (error) {
result.reject(error);
});

return result;
}
Expand Down
Loading