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

fix some working set view issues #9134

Closed
wants to merge 11 commits into from
Closed

fix some working set view issues #9134

Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions src/project/FileViewController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,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
Contributor Author

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
14 changes: 13 additions & 1 deletion 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
Contributor Author

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 @@ -305,8 +305,10 @@ define(function (require, exports, module) {

if (paneId === this.paneId) {
this.$el.addClass("active");
this.$openFilesContainer.addClass("active");
} else {
this.$el.removeClass("active");
this.$openFilesContainer.removeClass("active");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding and removing the active class on the .open-files-container div is what styles the items with blue text.
I left the active class on the outer on the working set list div which shouldn't matter if it isn't being used.

}

this._updateVisibility();
Expand Down Expand Up @@ -650,6 +652,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
Contributor Author

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 +667,8 @@ define(function (require, exports, module) {
WorkingSetView.prototype._handleFileListAdded = function (e, files, paneId) {
if (paneId === this.paneId) {
this._rebuildViewList(true);
} else {
this._checkForDuplicatesInWorkingTree();
}
};

Expand Down Expand Up @@ -690,6 +696,8 @@ define(function (require, exports, module) {
}

this._redraw();
} else {
this._checkForDuplicatesInWorkingTree();
Copy link
Contributor

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
Contributor

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
Contributor Author

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
Contributor

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 +719,8 @@ define(function (require, exports, module) {
});

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

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

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
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 @@ -97,7 +97,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 @@ -954,6 +954,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 @@ -975,9 +985,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
Contributor Author

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 @@ -1516,7 +1528,7 @@ define(function (require, exports, module) {
return result;
}

/**
/**
Copy link
Contributor

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
Contributor Author

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
Contributor

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 @@ -162,6 +162,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 @@ -238,12 +239,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 @@ -620,8 +626,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 @@ -742,10 +753,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
Contributor Author

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 @@ -774,7 +796,7 @@ define(function (require, exports, module) {
delete this._views[oldname];
}

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

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