Close Others extension improvements. #5497
Conversation
ghost
assigned 
| @@ -126,16 +126,25 @@ define(function (require, exports, module) { | |||
|
|
|||
| function runCloseOthers() { | |||
| var ws = DocumentManager.getWorkingSet(), | |||
| e = new jQuery.Event("contextmenu"), | |||
There was a problem hiding this comment.
why jQuery instead of $ ?
There was a problem hiding this comment.
Ok. let me modify it. If there is any other changes, please do let me know.
|
@JeffryBooher Hi, I did the changes that you had mentioned. Does it need any other changes? |
| $(workingSetCmenu).on("beforeContextMenuOpen", function () { | ||
| var targetIndex = dm.findInWorkingSet(dm.getCurrentDocument().file.fullPath), | ||
| closeOthers = (dm.getWorkingSet().length > 1), | ||
| closeOtherAbove = (targetIndex > 0), |
There was a problem hiding this comment.
For consistency -- you should name this closeOthersAbove -- you have closeOtherAbove
|
Drive-by comment, since it looks like you're cleaning up changes introduced with the Close Others extension: modifying the |
|
@JeffryBooher Changed that. Sorry i failed to see it. @iwehrman I did the change that you have proposed. But just curious, why do you think it would cause error? I think, if |
|
Yes, also other clients would like to be notified when file paths change, so just rewriting the path is not sufficient. Also some clients might maintain sets of files that are keyed by path, and changing the path of the underlying file without notifying them could cause problems. And, as I said, we're working on changes, which are coming soon, to the filesystem infrastructure such that rewriting the path will cause an explicit error. |
|
|
||
| runs(function () { | ||
| expect(DocumentManager.getCurrentDocument()).not.toBe(null); | ||
| }); | ||
| } | ||
|
|
||
| it("Close others", function () { |
There was a problem hiding this comment.
can you fix the tabs in this block. lines 156-159 are misaligned
There was a problem hiding this comment.
This is funny issue. It looked good in Brackets and Submlime but not in github. I deleted and recreated those lines to fix this.
|
|
||
| `pageX` and `pageY` can be any number. our only intention is to open context menu. but it can open at anywhere. | ||
| */ | ||
| cm.open({pageX: 0, pageY: 0}); |
There was a problem hiding this comment.
@JeffryBooher @redmunds. I replaced triggering contextmenu event with contextmenu.open. Hope you may like it this time.
|
We had ported some of these changes to the filesystem branch (to fix the code that modifies |
* 'Close Others' with one unsaved file did nothing (after choosing to save) -- this was a merge error in doSave(). * 'Close Others' with multiple unsaved files doesn't close working set items that have never been opened yet (after choosing to save) -- appears to be a bug in the code we copied from PR #5497 (saveFileList() & _doCloseDocumentList() were not on the same page about the list of files meant that was returned from saveFileList()). Added a note in the PR. Also rename 'savedFiles' vars to avoid confusion over whether they include files that weren't dirty. And improve docs.
|
See fa8c824 for details. |
|
Sure. Let me know once you merge |
|
@sathyamoorthi you might want to merge off of that file system branch to see how things are progressing and to make your merge as seamless as possible. Closing this pull request until you've had a chance to look at the changes coming in the new File System API and you can re-open when it's ready. |
|
@JeffryBooher i gave simple PR #5866 to fix #5800. Let me give other improvements in separate PR once they merge What do you think? |
PR #4590 follow up.
1.Small fix added to retain Current Document after closing list of files.
2.Code added to dynamically add / remove context menus based working set file selection.
CC: @JeffryBooher