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

"close others" extension added to default extensions directory. #4590

Merged
merged 13 commits into from
Oct 9, 2013
17 changes: 11 additions & 6 deletions src/document/DocumentCommandHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -917,12 +917,17 @@ define(function (require, exports, module) {
promptOnly = commandData && commandData.promptOnly;

var unsavedDocs = [];
DocumentManager.getWorkingSet().forEach(function (file) {
var doc = DocumentManager.getOpenDocumentForPath(file.fullPath);
if (doc && doc.isDirty) {
unsavedDocs.push(doc);
}
});

if (commandData.unsavedDocs) {
unsavedDocs = commandData.unsavedDocs;
} else {
DocumentManager.getWorkingSet().forEach(function (file) {
var doc = DocumentManager.getOpenDocumentForPath(file.fullPath);
if (doc && doc.isDirty) {
unsavedDocs.push(doc);
}
});
}

if (unsavedDocs.length === 0) {
// No unsaved changes, so we can proceed without a prompt
Expand Down
87 changes: 87 additions & 0 deletions src/extensions/default/CloseOthers/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright (c) 2013 Adobe Systems Incorporated. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
* to deal in the Software without restriction, including without limitation
* the rights to use, copy, modify, merge, publish, distribute, sublicense,
* and/or sell copies of the Software, and to permit persons to whom the
* Software is furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
*
*/

/*jslint vars: true, plusplus: true, devel: true, nomen: true, regexp: true, indent: 4, maxerr: 50 */
/*global define, $, brackets, window, document */

define(function (require, exports, module) {
"use strict";

var Menus = brackets.getModule("command/Menus"),
CommandManager = brackets.getModule("command/CommandManager"),
Commands = brackets.getModule("command/Commands"),
dm = brackets.getModule("document/DocumentManager"),
settings = JSON.parse(require("text!settings.json")),
working_set_cmenu = Menus.getContextMenu(Menus.ContextMenuIds.WORKING_SET_MENU);

function handleClose(mode) {

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),
unsavedDocs = [],
doc,
i;

if (mode === "close_others") {
end--;
workingSet.splice(targetIndex, 1);
}

for (i = start; i < end; i++) {
doc = dm.getOpenDocumentForPath(workingSet[i].fullPath);

if (doc && doc.isDirty) {
unsavedDocs.push(doc);
}
}

CommandManager.execute(Commands.FILE_CLOSE_ALL, {promptOnly: true, unsavedDocs: unsavedDocs}).done(function () {
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 needs to be refactored since it's not closing all documents in this case. Just make it a function in DocumentManager (e.g. DocumentManager.closeDocuments() ) -- and just call that function directly from here.

The Close All command handler code in DocumentCommandHandlers.js can be refactored so that it calls DocumentManager.closeDocuments() and just passes it the array of files from the working set. There you can do the work of checking for unsaved documents and prompt to save them and close the rest. Then you don't need the code inside this execute promise as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about just passing an array of documents to close as the command data of FILE_CLOSE_ALL. Then inside handleFileCloseAll it would use this list of files or the entire working set if the list is undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TomMalbran isn't that basically what it's doing now (sans the actually closing of documents)? Feels cleaner if handleFileCloseAll() just handles closing all documents as its name implies and refactor the logic to into a function in DocumentManager that we can pass an array to and it does the heavy lifting.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JeffryBooher Is actually similar to what you are saying, which I just understood. So closeDocuments(documents, promptOnly) would be exactly the same as the current (in master) handleFileCloseAll but replacing DocumentManager.getWorkingSet() with documents and handleFileCloseAll would call it passing DocumentManager.getWorkingSet() as the documents array. This is what I was thinking of, with the difference of still using the Command, instead of calling the Document Manager directly. Any way of doing it would be fine.

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 handleFileClose is also having prompt. If i move prompt code from handleFileCloseAll to DocumentManager.closeDocuments, we will have prompt code in two different files (DocumentManager and DocumentCommandHandler) for the same functionality.

In this case, we should make handleFileClose and handleFileCloseAll to single function with file array as input param. Something like this.

//this function will do both 'handleFileClose' and 'handleFileCloseAll' functionalities
function handleFileClose(files) {
         DocumentManager.closeDocuments(files);
}  

function DocumentManager.closeDocuments(files) {

        var dirtyFiles = getDirtyFiles(files);

        if(dirtyFiles.length === 1) {
                 //prompt for single file.
        } else {
                 //prompt for multiple files.
        }

}

But again to save documents i have to refer handleFileSave in DocumentCommandHandler.

Another biggest problem here, handleFileClose and handleFileCloseAll are referred by many files. So eventually we have to change them all. So it may seems like biggest change. But at the end, we will have good code. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sathyamoorthi all of that code inside of handleFileCloseAll needs to move to the new function including the prompt. Alternately you could create a new command (e.g. FILE_CLOSE_DOCUMENT_LIST) and pass the list in with the command data. That might be easier and less code. You can invoke that command from your extension and have the command handler call a function that both it and handleFileCloseAll can call (passing in the commandData).

I think you may need to add another function to DocumentManager to handle closing of files from a list. You will need to check that if you're closing the currently open document that you clear the current one from the editor and handle closing all of the files in the list from the working set.

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 This seems ok. Let me give a try. Just confirming, after implementing a new command FILE_CLOSE_DOCUMENT_LIST, handleFileCloseAll will look like this.

function handleFileCloseAll(commandData) {
      var  promptOnly = commandData && commandData.promptOnly;

      CommandManager.execute(Commands.FILE_CLOSE_DOCUMENT_LIST, 
      {promptOnly: promptOnly , docList: DocumentManager.getWorkingSet()});
 }

Am i correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sathyamoorthi No need to execute another command. I would implement it like this:

function _doCloseDocumentList(list, promptOnly) {
// code to do the closing...
}

function handleFileCloseDocumentList(commandData) {
_doCloseDocumentList (commandData.documentList, commandData.promptOnly);
}

function handleFileCloseAll(commandData) {
_doCloseDocumentList (DocumentManager.getWorkingSet, commandData.promptOnly);
}

for (i = start; i < end; i++) {
dm.removeFromWorkingSet(workingSet[i]);
}
});
}

if (settings.close_below) {
CommandManager.register("Close Others Below", "file.close_below", function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

These strings should be localizable.

See this wiki page: https://github.com/adobe/brackets/wiki/Localization. It's actually quite simple. You can just put up english strings for now and let the community update them as necessary.

handleClose("close_below");
});
working_set_cmenu.addMenuItem("file.close_below", "", Menus.AFTER, Commands.FILE_CLOSE);
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be moved as constants to the start of the file. Using camel case for them might look better too.

}

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

if (settings.close_above) {
CommandManager.register("Close Others Above", "file.close_above", function () {
handleClose("close_above");
});
working_set_cmenu.addMenuItem("file.close_above", "", Menus.AFTER, Commands.FILE_CLOSE);
}
});
5 changes: 5 additions & 0 deletions src/extensions/default/CloseOthers/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"close_others": true,
"close_above": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should default these to true or users will not discover them. The ideal way is to turn them on by default and let users turn them off.

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 Is there a way to add and remove context menus (close others above & close others below) at runtime?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the menus are created in/from JavaScript after the shell launches, they are all created at runtime. You should be able to add/remove/disable the menu items at any time.

"close_below": false
}
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 Shall we enable only "close others"? Because, it would annoy users by showing other two (close_above and close_below) all the time. If i once know how to add and remove menus dynamically we can add them. what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I say show them. If it becomes a problem we can easily turn it off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. consider if there is only one file in the working set, it will still show "Close Others", "Close Others Above" and "Close Others Below". So i should work next to add and remove menus intelligently based user click. So if you wish, i can down those two flags and give pull request for 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 considered it and the convention is to disable the menu items which we have a story for and we will be working soon. So leave them enabled for now and then we'll have to fix it so they are disabled when the first/last item are selected later.

Copy link
Contributor

Choose a reason for hiding this comment

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

we have many menu items that are enabled when they shouldn't be so we are aware of the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. that's cool. just making sure you know this nit. then, no problem. let me do those small change and give PR.