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

fix some working set view issues #9134

Closed
JeffryBooher wants to merge 11 commits into from
Closed

fix some working set view issues #9134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4e2b40c
fix a couple of working set view issues
JeffryBooher Sep 17, 2014
6b511cd
fix 2 more issues
JeffryBooher Sep 17, 2014
2211499
fix rte
JeffryBooher Sep 17, 2014
9ee3a2a
merge with master
JeffryBooher Sep 17, 2014
9433ad8
fix selector issue with selected item
JeffryBooher Sep 17, 2014
eba535c
restore the order
JeffryBooher Sep 17, 2014
fb2b075
Merge branch 'master' into jeff/wsv-fixes
JeffryBooher Sep 17, 2014
4df265e
rtc: document _suppresRedraw flag and qualify affected working more r…
JeffryBooher Sep 17, 2014
eefe6b3
restore the blues
JeffryBooher Sep 18, 2014
2ecef07
fix tree selection issue
JeffryBooher Sep 18, 2014
48f6486
Merge branch 'master' into jeff/wsv-fixes
JeffryBooher Sep 18, 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
8 changes: 6 additions & 2 deletions src/project/FileViewController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ define(function (require, exports, module) {
* @return {$.Promise}
*/
function openAndSelectDocument(fullPath, fileSelectionFocus, paneId) {
var result;
var result,
curDocChangedDueToMe = _curDocChangedDueToMe;

if (fileSelectionFocus !== PROJECT_MANAGER && fileSelectionFocus !== WORKING_SET_VIEW) {
console.error("Bad parameter passed to FileViewController.openAndSelectDocument");
Expand Down Expand Up @@ -172,7 +173,7 @@ define(function (require, exports, module) {

// clear after notification is done
result.always(function () {
_curDocChangedDueToMe = false;
_curDocChangedDueToMe = curDocChangedDueToMe;
});

return result;
Expand All @@ -199,6 +200,9 @@ define(function (require, exports, module) {
// image file, we get a null doc here but we still want to keep _fileSelectionFocus
// as PROJECT_MANAGER. Regardless of doc is null or not, call _activatePane
// to trigger documentSelectionFocusChange event.
_fileSelectionFocus = WORKING_SET_VIEW;
_activatePane(paneId);
Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

calling _activatePane here after setting _fileSelectionFocus to WORKING_SET_VIEW will trigger a documentSelectionFocusChange event which will tell the working set view to show the file in the selected state which fixes #9032.

Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

this is what the old master did, btw.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 17, 2014

Choose a reason for hiding this comment

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

Fix for #9032 looks good except for 1 quirk with single select in project tree.

  1. Split View, Open 4 files -- 2 in each working set

  2. In project tree, single-click file that is open in current working set, but not displayed in editor.

    Results:
    File is selected in project tree. File is displayed in editor. This is same as 0.43

  3. In project tree, single-click file in the other working set that is not displayed in editor.

    Results:
    File is selected in project tree. File is displayed in editor. This is same as 0.43

  4. In project tree, single-click file in the other working set that is displayed in editor.

    Results:
    File is selected in working set.

    Expected:
    File is selected in project tree.

Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 18, 2014

Choose a reason for hiding this comment

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

this should be fixed now.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 18, 2014

Choose a reason for hiding this comment

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

I'm seeing that case 4 is fixed, but case 3 is now broken.

Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 18, 2014

Choose a reason for hiding this comment

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

hmmmm. this worked fine for me last night on the mac and it's working fine for me today on Windows.

#4 - single click file in other working set that IS displayed in editor
-> File is selected in project tree.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 18, 2014

Choose a reason for hiding this comment

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

@JeffryBooher Yes, 4 is now working for me, but 3 is no longer working.

Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 18, 2014

Choose a reason for hiding this comment

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

sorry, I was drawn to the "bold". The problem with #3 is the issue I fixed last night which is still working today for me. LMK after syncing to the latest branch jeff/wsv-fixes and we can look at it over connect.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 18, 2014

Choose a reason for hiding this comment

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

It's fixed. I guess I didn't pull latest change.


result.resolve(file);
}).fail(function (err) {
result.reject(err);
Expand Down
83 changes: 60 additions & 23 deletions src/project/WorkingSetView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ define(function (require, exports, module) {
WorkingSetView.prototype._checkForDuplicatesInWorkingTree = function () {
var self = this,
map = {},
fileList = MainViewManager.getWorkingSet(this.paneId);
fileList = MainViewManager.getWorkingSet(MainViewManager.ALL_PANES);
Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

don't limit the search to just the current pane's working set. search them all...


// We need to always clear current directories as files could be removed from working tree.
this.$openFilesContainer.find("ul > li > a > span.directory").remove();
Expand Down Expand Up @@ -301,14 +301,7 @@ define(function (require, exports, module) {
* @private
*/
WorkingSetView.prototype._redraw = function () {
var paneId = MainViewManager.getActivePaneId();

if (paneId === this.paneId) {
this.$el.addClass("active");
} else {
this.$el.removeClass("active");
}

this._updateViewState();
this._updateVisibility();
this._adjustForScrollbars();
this._fireSelectionChanged();
Expand Down Expand Up @@ -622,13 +615,32 @@ define(function (require, exports, module) {
}
};

/**
* Updates the pane view's selection state
* @private
*/
WorkingSetView.prototype._updateViewState = function () {
var paneId = MainViewManager.getActivePaneId();
if ((FileViewController.getFileSelectionFocus() === FileViewController.WORKING_SET_VIEW) &&
Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

if the focus is in the project tree then remove the active class so the blue highlight goes away

(paneId === this.paneId)) {
this.$el.addClass("active");
this.$openFilesContainer.addClass("active");
} else {
this.$el.removeClass("active");
this.$openFilesContainer.removeClass("active");
}
};

/**
* Updates the pane view's selection marker and scrolls the item into view
* @private
*/
WorkingSetView.prototype._updateListSelection = function () {
var file = MainViewManager.getCurrentlyViewedFile(this.paneId);


this._updateViewState();


// Iterate through working set list and update the selection on each
this.$openFilesContainer.find("ul").children().each(function () {
_updateListItemSelection(this, file);
Expand All @@ -650,6 +662,8 @@ define(function (require, exports, module) {
WorkingSetView.prototype._handleFileAdded = function (e, fileAdded, index, paneId) {
if (paneId === this.paneId) {
this._rebuildViewList(true);
} else {
this._checkForDuplicatesInWorkingTree();
Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

these calls to _checkForDuplicatesInWorkingTree when there event wasn't directed to this pane (paneId !== this.paneId) will fix the current working set when a duplicate is added to another pane and remove the path from the list element when the duplicate entry is removed.

}
};

Expand All @@ -663,6 +677,8 @@ define(function (require, exports, module) {
WorkingSetView.prototype._handleFileListAdded = function (e, files, paneId) {
if (paneId === this.paneId) {
this._rebuildViewList(true);
} else {
this._checkForDuplicatesInWorkingTree();
}
};

Expand All @@ -675,21 +691,38 @@ define(function (require, exports, module) {
* @param {!string} paneId - the id of the pane the item that was to
*/
WorkingSetView.prototype._handleFileRemoved = function (e, file, suppressRedraw, paneId) {
if (paneId === this.paneId && !suppressRedraw) {
var $listItem = this._findListItemFromFile(file);
if ($listItem) {
// Make the next file in the list show the close icon,
// without having to move the mouse, if there is a next file.
var $nextListItem = $listItem.next();
if ($nextListItem && $nextListItem.length > 0) {
var canClose = ($listItem.find(".can-close").length === 1);
var isDirty = _isOpenAndDirty($nextListItem.data(_FILE_KEY));
this._updateFileStatusIcon($nextListItem, isDirty, canClose);
/*
* The suppressRedraw flag is used in cases when we are replacing the working
* set entry with another one. There are only 2 use cases for this:
*
* 1) When an untitled document is being saved.
* 2) When a file is saved with a new name.
*/
if (paneId === this.paneId) {
if (!suppressRedraw) {
var $listItem = this._findListItemFromFile(file);
if ($listItem) {
// Make the next file in the list show the close icon,
// without having to move the mouse, if there is a next file.
var $nextListItem = $listItem.next();
if ($nextListItem && $nextListItem.length > 0) {
var canClose = ($listItem.find(".can-close").length === 1);
var isDirty = _isOpenAndDirty($nextListItem.data(_FILE_KEY));
this._updateFileStatusIcon($nextListItem, isDirty, canClose);
}
$listItem.remove();
}
$listItem.remove();

this._redraw();
}

this._redraw();
} else {
/*
* When this event is handled by a pane that is not being updated then
* the suppressRedraw flag does not need to be respected.
* _checkForDuplicatesInWorkingTree() does not remove any entries so it's
* safe to call at any time.
*/
this._checkForDuplicatesInWorkingTree();
Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 17, 2014

Choose a reason for hiding this comment

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

The suppressRedraw param here is a bit weird. Before this change, if suppressRedraw is true, then method does nothing. Seems like caller should detect that and not even call it?

I'm wondering if _checkForDuplicatesInWorkingTree() needs to be called when suppressRedraw is true?

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 17, 2014

Choose a reason for hiding this comment

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

Looks like we want to ignore suppressRedraw and just check paneId:

        if (paneId === this.paneId) {
            if (!suppressRedraw) {
                // snip...
            }
        } else {
            this._checkForDuplicatesInWorkingTree();
        }

Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

Keep in mind that there are 2 working set views...

The suppress redraw flag is only affecting the one that's being updated which won't check for duplicates when the suppressRedraw flag is true.

This is used in 2 cases:

  1. When an untitled document is being saved.
  2. When a file is saved with a new name.

The pane that isn't affected by the change, however, is free to draw when it needs to with check for duplicates.

The other thing to keep in mind is that when using the suppressRedraw flag, it's just to prevent removing list items when they are being removed from the working set and a new one is being added in a new place. Check for duplicates doesn't remove anything so we would still be able to check for duplicates when suppressRedraw is passed on the affected view.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 17, 2014

Choose a reason for hiding this comment

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

That's a great explanation that belongs in the code somewhere!

}
};

Expand All @@ -711,6 +744,8 @@ define(function (require, exports, module) {
});

this._redraw();
} else {
this._checkForDuplicatesInWorkingTree();
}
};

Expand Down Expand Up @@ -749,6 +784,8 @@ define(function (require, exports, module) {
WorkingSetView.prototype._handleWorkingSetUpdate = function (e, paneId) {
if (this.paneId === paneId) {
Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

also check for rename and delete.

this._rebuildViewList(true);
} else {
this._checkForDuplicatesInWorkingTree();
}
};

Expand Down
22 changes: 6 additions & 16 deletions src/styles/brackets.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,11 +769,12 @@ a, img {
min-height: 18px;
vertical-align: baseline;

&.selected {
a,
.extension {
color: #99dbff;
}
&.selected a {
color: @open-working-file-name-highlight;
}

Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

@larz0 this deviated from what it was previously so I changed it back to use @open-working-file-name-highlight and @open-working-file-ext-highlight and remove this in lieu of the latter rule.

Copy link
Copy Markdown
Member

@larz0 larz0 Sep 17, 2014

Choose a reason for hiding this comment

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

@JeffryBooher this is fine as long as it's still behaves and looks the same.

&.selected .extension {
color: @open-working-file-ext-highlight;
}
}

Expand Down Expand Up @@ -805,18 +806,7 @@ a, img {
}
}

.open-files-container.active {

li {
&.selected a {
color: @open-working-file-name-highlight;
}

&.selected .extension {
color: @open-working-file-ext-highlight;
}
}
}

.sidebar-selection {
background: @bc-sidebar-selection;
Expand Down
16 changes: 14 additions & 2 deletions src/view/MainViewManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ define(function (require, exports, module) {
ViewUtils = require("utils/ViewUtils"),
Resizer = require("utils/Resizer"),
Pane = require("view/Pane").Pane;

/**
* Preference setting name for the MainView Saved State
* @const
Expand Down Expand Up @@ -949,6 +949,16 @@ define(function (require, exports, module) {
});
}

/**
* Updates the header text for all panes
*/
function _updatePaneHeaders() {
_forEachPaneOrPanes(ALL_PANES, function (pane) {
pane.updateHeaderText();
});

}

/**
* Creates a pane for paneId if one doesn't already exist
* @param {!string} paneId - id of the pane to create
Expand All @@ -969,9 +979,11 @@ define(function (require, exports, module) {
});

$(pane).on("viewListChange.mainview", function () {
_updatePaneHeaders();
$(exports).triggerHandler("workingSetUpdate", [pane.id]);
});
$(pane).on("currentViewChange.mainview", function (e, newView, oldView) {
_updatePaneHeaders();
Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

Updating the titles here will ensure that "Temporary" views and duplicates thereof are given a qualified name too.

if (_activePaneId === pane.id) {
$(exports).triggerHandler("currentFileChange",
[newView && newView.getFile(),
Expand Down Expand Up @@ -1502,7 +1514,7 @@ define(function (require, exports, module) {
return result;
}

/**
/**
Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 18, 2014

Choose a reason for hiding this comment

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

@JeffryBooher Why did you add a space at end of comment open delimiter here? I cleaned up a bunch of those in another PR.

Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 18, 2014

Choose a reason for hiding this comment

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

i'm pretty sure that brackets did this on its own :) Looks like it did something similar on line 97.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 18, 2014

Choose a reason for hiding this comment

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

I've never seen this in any other code review. Maybe an extension? Maybe a recently introduced bug?

* Setup a ready event to initialize ourself
*/
AppInit.htmlReady(function () {
Expand Down
34 changes: 28 additions & 6 deletions src/view/Pane.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ define(function (require, exports, module) {
Commands = require("command/Commands"),
Strings = require("strings"),
ViewUtils = require("utils/ViewUtils"),
ProjectManager = require("project/ProjectManager"),
paneTemplate = require("text!htmlContent/pane.html");


Expand Down Expand Up @@ -237,12 +238,17 @@ define(function (require, exports, module) {
}
});

this._updateHeaderText();
this.updateHeaderText();

// Listen to document events so we can update ourself
$(DocumentManager).on(this._makeEventName("fileNameChange"), _.bind(this._handleFileNameChange, this));
$(DocumentManager).on(this._makeEventName("pathDeleted"), _.bind(this._handleFileDeleted, this));
$(MainViewManager).on(this._makeEventName("activePaneChange"), _.bind(this._handleActivePaneChange, this));
$(MainViewManager).on(this._makeEventName("workingSetAdd"), _.bind(this.updateHeaderText, this));
$(MainViewManager).on(this._makeEventName("workingSetRemove"), _.bind(this.updateHeaderText, this));
$(MainViewManager).on(this._makeEventName("workingSetAddList"), _.bind(this.updateHeaderText, this));
$(MainViewManager).on(this._makeEventName("workingSetRemoveList"), _.bind(this.updateHeaderText, this));

}

/**
Expand Down Expand Up @@ -619,8 +625,13 @@ define(function (require, exports, module) {
return uniqueFileList;
};

/**
* Dispatches a currentViewChange event
* @param {?View} newView - the view become the current view
* @param {?View} oldView - the view being replaced
*/
Pane.prototype._notifyCurrentViewChange = function (newView, oldView) {
this._updateHeaderText();
this.updateHeaderText();

$(this).triggerHandler("currentViewChange", [newView, oldView]);
};
Expand Down Expand Up @@ -741,10 +752,21 @@ define(function (require, exports, module) {
* Updates text in pane header
* @private
*/
Pane.prototype._updateHeaderText = function () {
var file = this.getCurrentlyViewedFile();
Pane.prototype.updateHeaderText = function () {
var file = this.getCurrentlyViewedFile(),
files,
displayName;

if (file) {
this.$header.text(file.name);
files = MainViewManager.getAllOpenFiles().filter(function (item) {
Copy link
Copy Markdown
Contributor Author

@JeffryBooher JeffryBooher Sep 17, 2014

Choose a reason for hiding this comment

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

we only care about the currently displayed file so just filter the list to those matching the name of the current file

return (item.name === file.name);
});
if (files.length < 2) {
this.$header.text(file.name);
} else {
displayName = ProjectManager.makeProjectRelativeIfPossible(file.fullPath);
this.$header.text(displayName);
}
} else {
this.$header.html(Strings.EMPTY_VIEW_HEADER);
}
Expand Down Expand Up @@ -773,7 +795,7 @@ define(function (require, exports, module) {
delete this._views[oldname];
}

this._updateHeaderText();
this.updateHeaderText();

// dispatch the change event
if (dispatchEvent) {
Expand Down