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

Close Others extension improvements. #5497

Closed
wants to merge 8 commits into from
Closed

Close Others extension improvements. #5497

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eaa593b
'clearCurrentDoc' flag added to '_doCloseDocumentList'
sathyamoorthi Oct 12, 2013
7ff5efa
unit test file update
sathyamoorthi Oct 12, 2013
913cabc
Add / remove context menus at run time.
sathyamoorthi Oct 17, 2013
05e41ef
jsHint fixes.
sathyamoorthi Oct 17, 2013
d0fa2db
replace `jQuery` with `$` for coding standard.
sathyamoorthi Oct 18, 2013
83d603c
Code changes proposed by Jeff and Ian
sathyamoorthi Oct 22, 2013
e1efd6d
Tab alignments corrected.
sathyamoorthi Oct 23, 2013
fbdd877
Test file updated to match standards.
sathyamoorthi Oct 25, 2013
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
6 changes: 3 additions & 3 deletions src/document/DocumentCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,7 @@ define(function (require, exports, module) {
// guarantees that handlers run in the order they are added.
result.done(function () {
if (!promptOnly) {
DocumentManager.removeListFromWorkingSet(list, (clearCurrentDoc || true));
DocumentManager.removeListFromWorkingSet(list, clearCurrentDoc);
}
});

Expand All @@ -1018,11 +1018,11 @@ define(function (require, exports, module) {
* @return {$.Promise} a promise that is resolved when all files are closed
*/
function handleFileCloseAll(commandData) {
return _doCloseDocumentList(DocumentManager.getWorkingSet(), (commandData && commandData.promptOnly));
return _doCloseDocumentList(DocumentManager.getWorkingSet(), (commandData && commandData.promptOnly), true);
}

function handleFileCloseList(commandData) {
return _doCloseDocumentList((commandData && commandData.documentList), false);
return _doCloseDocumentList((commandData && commandData.documentList), false, false);
}

/**
Expand Down
98 changes: 71 additions & 27 deletions src/extensions/default/CloseOthers/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,24 @@ define(function (require, exports, module) {
dm = brackets.getModule("document/DocumentManager"),
docCH = brackets.getModule("document/DocumentCommandHandlers"),
strings = brackets.getModule("i18n!nls/strings"),
settings = JSON.parse(require("text!settings.json")),
working_set_cmenu = Menus.getContextMenu(Menus.ContextMenuIds.WORKING_SET_MENU),
workingSetCmenu = Menus.getContextMenu(Menus.ContextMenuIds.WORKING_SET_MENU),
close_others = "file.close_others",
close_above = "file.close_above",
close_below = "file.close_below";

function handleClose(mode) {

/*
This function collects files based on passing command (close_others/above/below) and execute 'FILE_CLOSE_LIST'.
*/
function handleClose(cmd) {

var targetIndex = dm.findInWorkingSet(dm.getCurrentDocument().file.fullPath),
workingSet = dm.getWorkingSet().slice(0),
start = (mode === close_below) ? (targetIndex + 1) : 0,
end = (mode === close_above) ? (targetIndex) : (workingSet.length),
start = (cmd === close_below) ? (targetIndex + 1) : 0,
end = (cmd === close_above) ? (targetIndex) : (workingSet.length),
docList = [],
i;

if (mode === close_others) {
if (cmd === close_others) {
end--;
workingSet.splice(targetIndex, 1);
}
Expand All @@ -59,25 +61,67 @@ define(function (require, exports, module) {

CommandManager.execute(Commands.FILE_CLOSE_LIST, {documentList: docList});
}

if (settings.close_below) {
CommandManager.register(strings.CMD_FILE_CLOSE_BELOW, close_below, function () {
handleClose(close_below);
});
working_set_cmenu.addMenuItem(close_below, "", Menus.AFTER, Commands.FILE_CLOSE);
}

if (settings.close_others) {
CommandManager.register(strings.CMD_FILE_CLOSE_OTHERS, close_others, function () {
handleClose(close_others);
});
working_set_cmenu.addMenuItem(close_others, "", Menus.AFTER, Commands.FILE_CLOSE);
}

if (settings.close_above) {
CommandManager.register(strings.CMD_FILE_CLOSE_ABOVE, close_above, function () {
handleClose(close_above);
});
working_set_cmenu.addMenuItem(close_above, "", Menus.AFTER, Commands.FILE_CLOSE);

/*
Pass 'command id' and it'll let you know, whether contextmenu for that command is existing or not.
*/
function isMenuThere(cmd) {
return Menus.getMenuItem(workingSetCmenu.id + "-" + cmd) ? true : false;
}

/*
This function is responsible for add/remove context menus based on current file selection.
If there is only one file in working set, we won't show any of the three (Close Others, Close Others Above/Below).
If there is more than one file, but selected file is first / last in working set, we will show only "Close Others".
In other cases we will show all three.
*/
$(workingSetCmenu).on("beforeContextMenuOpen", function () {
var targetIndex = dm.findInWorkingSet(dm.getCurrentDocument().file.fullPath),
closeOthers = (dm.getWorkingSet().length > 1),
closeOtherAbove = (targetIndex > 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency -- you should name this closeOthersAbove -- you have closeOtherAbove

closeOthersBelow = (targetIndex < dm.getWorkingSet().length - 1);

if (closeOtherAbove && closeOthersBelow) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better, IMO if we removed the "Close Others" menu item and used "Closed Others Below" or "Close Others Above" when the selection is on the First/Last item. @larz0 what do you think?

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 "Close Others" is fine as it's pretty clear already.

if (!isMenuThere(close_above)) {

CommandManager.register(strings.CMD_FILE_CLOSE_ABOVE, close_above, function () {
handleClose(close_above);
});
workingSetCmenu.addMenuItem(close_above, "", Menus.AFTER, Commands.FILE_CLOSE);
}

if (!isMenuThere(close_below)) {
CommandManager.register(strings.CMD_FILE_CLOSE_BELOW, close_below, function () {
handleClose(close_below);
});
workingSetCmenu.addMenuItem(close_below, "", Menus.BEFORE, Commands.FILE_SAVE);
}
} else {
if (isMenuThere(close_above)) {
workingSetCmenu.removeMenuItem(close_above);
}

if (isMenuThere(close_below)) {
workingSetCmenu.removeMenuItem(close_below);
}
}

if (closeOthers) {
if (!isMenuThere(close_others)) {
CommandManager.register(strings.CMD_FILE_CLOSE_OTHERS, close_others, function () {
handleClose(close_others);
});

if (isMenuThere(close_above)) { //if "Close Others Above" exists add "Close Others" next to it
workingSetCmenu.addMenuItem(close_others, "", Menus.AFTER, close_above);
} else { //else add "Close Others" next to "Close"
workingSetCmenu.addMenuItem(close_others, "", Menus.AFTER, Commands.FILE_CLOSE);
}
}
} else {
if (isMenuThere(close_others)) {
workingSetCmenu.removeMenuItem(close_others);
}
}
});
});
5 changes: 0 additions & 5 deletions src/extensions/default/CloseOthers/settings.json
View file
Open in desktop

This file was deleted.

9 changes: 9 additions & 0 deletions src/extensions/default/CloseOthers/unittests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,25 @@ define(function (require, exports, module) {

function runCloseOthers() {
var ws = DocumentManager.getWorkingSet(),
e = new $.Event("contextmenu"),
promise;

if (ws.length > docSelectIndex) {
DocumentManager.getDocumentForPath(ws[docSelectIndex].fullPath).done(function (doc) {
DocumentManager.setCurrentDocument(doc);

e.pageX = 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to specify mouse coordinates here? If I change them to 0,0 or 500,500 does it still work or is 20,20 the only thing that works? We may want to think about a different way to exercise the handler -- using page relative mouse clicks to invoke a menu is bad. @redmunds -- do you have an opinion on this?

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 need to trigger "contextmenu" event on "working set" to open context menu popup. So only context menus (Close Others / Above / Below) will get populate. Without specifying coordinates, Menu.js gives me an error from line 960. But coordinates can be anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic depends on the context menu being displayed at 0,0 of page and context menu item being at 20,20. I agree with @JeffryBooher that it's fragile and unnecessary to test this. Instead you can use closeOthers = require("main") to include your module and call the handleClose() function directly. Note: you'll need to export it as something like _handleClose to indicate it's only meant to be public for unit testing.

Related comment: I notice that this extension only displays the "Close Others" command in the context menu when there's more than 1 item in the Working Set. I prefer to always have commands shown and then disable them appropriately. I mention this, because this test depends on context menu item being in a particular position, yet this extension makes this indeterminate for other code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JeffryBooher @redmunds

_I notice that this extension only displays the "Close Others" command in the context menu when there's more than 1 item in the Working Set. I prefer to always have commands shown and then disable them appropriately_

disable Or remove? Currently it removes unnecessary Or shows necessary "Context Menu Items" based on user file selection as like below screenshot. If we show all 3 all time and if we only disable them (as like in Google Chrome Tabs), context menu would look like lengthy in Brackets.

Image

_I think this logic depends on the context menu being displayed at 0,0 of page and context menu item being at 20,20._

Nope. we can specify like below, which means coordinates can be anything.

e.pageX = 4000;
e.pageY = 4000;

This extension removes unnecessary menus Or adds necessary menus on workingset's beforeContextMenuOpen event. For that, i need to open "Context Menu Popup" before run my tests. So only appropriate "Context Menu Items" will get populate. But clearly this is test is not dealing with "Context Menu" position.

_using page relative mouse clicks to invoke a menu is bad._

:) why? Test file's intention is to open a context menu, not open it at particular position. And if you still think, this hack would cause an error in future, it is just a test file. At maximum, tests would fail. It won't affect Brackets.

Copy link
Contributor

Choose a reason for hiding this comment

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

If value of e.pageX and e.pageY can be anything, then use -1,-1 or 0,0 and add a comment that explains value doesn't matter.

e.pageY = 20;
$("#open-files-container").trigger(e);
});

promise = CommandManager.execute(cmdToRun);
waitsForDone(promise, cmdToRun);
}

runs(function () {
expect(DocumentManager.getCurrentDocument()).not.toBe(null);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

this tab is misaligned

}

it("Close others", function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you fix the tabs in this block. lines 156-159 are misaligned

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 funny issue. It looked good in Brackets and Submlime but not in github. I deleted and recreated those lines to fix this.

Expand Down