"close others" extension added to default extensions directory. #4590
Conversation
|
I got three warnings in "Travis Build" and notified as Failed. I gave proper pull request. But i don't know the reason for Failure. Can anybody help me in this to proceed further? |
|
@sathyamoorthi I've been getting those warnings as well. I'll mention it tomorrow. |
|
@larz0 ahhh. Thank you. I am not an expert in Github. I created this PR because i got this same error in my old PR. When i got "Travis" warnings again here, i was totally blank. Thank god, you gave positive reply. |
|
@sathyamoorthi There was a change to jsHint that is causing all builds to fail. We have a fix that will go in once we have sprint 28 wrapped up and delivered. |
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "close_others": true, | |||
| "close_above": false, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@JeffryBooher Is there a way to add and remove context menus (close others above & close others below) at runtime?
There was a problem hiding this comment.
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.
|
@JeffryBooher small bug in this pull request. Let me update this on next commit. |
ghost
assigned
|
@JeffryBooher, @TomMalbran committed new set of changes that you both mentioned. Still, i am expecting some more changes after your review. So i didn't add comments to functions that i have added in the core. Once, everything finalized we can add function headers. |
|
@sathyamoorthi are you still working on this? |
|
@JeffryBooher yes. i committed changes that you have mentioned. i'm waiting for your review. |
| */ | ||
| function removeFromWorkingSet(file, suppressRedraw) { | ||
|
|
||
| function _removeFromArrays(file) { |
There was a problem hiding this comment.
this name isn't very descriptive plus it isn't changing the behavior. Maybe _internalRemoveFromWorkingSet() is a better name?
|
Done with review. Just a couple of minor naming nits and this is ready to merge. Good Job! |
| "close_others": true, | ||
| "close_above": true, | ||
| "close_below": true | ||
| } |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Yep, I say show them. If it becomes a problem we can easily turn it off
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we have many menu items that are enabled when they shouldn't be so we are aware of the problem.
There was a problem hiding this comment.
oh. that's cool. just making sure you know this nit. then, no problem. let me do those small change and give PR.
|
@JeffryBooher changed those function names. |
| return true; | ||
| } | ||
|
|
||
| function removeFromWorkingSet(file, suppressRedraw) { |
There was a problem hiding this comment.
minor nit: this should be named removeFileFromWorkingSet()
There was a problem hiding this comment.
actually what would be better is if you could pass a string or an array to removeFromWorkingSet(). you can use $.isArray() to determine if it's an array then you don't need two different functions.
I think you could re-work the handlers for workingSetRemove so there isn't 2 different events (workingSetRemove and workingSetRemoveList) using the same technique.
There was a problem hiding this comment.
We can also use the new EcmaScript 5 Array.isArray() function.
There was a problem hiding this comment.
In project/WorkingSetView.js see _handleFileRemoved function. What is the need of below code.
// 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));
_updateFileStatusIcon($nextListItem, isDirty, canClose);
}
In comments it stated that it is used to show icon on the next file. But i removed this block and still i'm able to see icon on the next file.
There was a problem hiding this comment.
I don't think you want to remove this block. the icon shows up on mouse over so this is just to make sure that when you click the 'x' that it shows up when the item below moves up.
|
Can you merge master into this branch? There are quite a number of unit-test failures that I don't understand. I'm hoping that merging master into the branch fixes most of them. |
|
@JeffryBooher any more changes needed? |
|
@sathyamoorthi I haven't had time the past few days to look at it since we've been closing down Sprint 30. I'll look at it over the weekend. |
|
@JeffryBooher sure.np. |
|
The code looks great. There are a few unit test failures that you need to fix plus you need unit tests for close others, close above, and close below. I'd start by merging master into your branch. I think there are some failures that may be due to the fact that you're out of date with the current shell plus some others that have been fixed in master. |
|
@JeffryBooher I have added unit tests for close others, above and below. Please take a look. |
|
@sathyamoorthi looks like you forgot to add the "unittest-files" folder and the unit test files to your pull request :) |
|
@JeffryBooher Hi. Actually unittest-files folder don't have any content. Files will be created at run time inside that folder and will be deleted once everything finished well. So, to commit that folder i have added a dummy file. And i'm seeing lot of integration test failures. I'm not sure whether i broke anything by modifying |
|
@sathyamoorthi you might want to merge master into your branch snd see if that fixes the test failures. |
|
@JeffryBooher I merged master into my branch. But still i see failures. Meanwhile, no errors when i run tests directly on Master branch. |
|
I can take a look |
|
great. thanks. |
| return true; | ||
| } | ||
|
|
||
| function removeFromWorkingSet(content, suppressRedraw) { |
There was a problem hiding this comment.
All the docs for this API were lost -- please copy them from the diff above. (And update the type annotation -- I think @param {!FileEntry|Array.<FileEntry>} file ?)
|
@sathyamoorthi thanks for the heads up. This turned out to be an async issue in the cleanup temp folder unit test helper. We may still have some issues with that but it looks like @jasonsanjose fixed it in before the end of the sprint last week. I merged master into your branch and tried it again and the tests are still failing. I'm not sure why except that there are a few tests that fail in your branch that are caused by changes you made to saving and the dirty flag. Merge master into your branch and fix the Document and Document Command Handlers integration test failures and we will go from there. I suspect that the API changes you made may be impacting these tests. Hopefully the other tests will fix themselves with the merge from master. |
|
@JeffryBooher Fixed all integration test failures. Please do another review. |
|
@sathyamoorthi thanks. Unfortunately it's too late to take for Sprint 32 I'll nominate this for Sprint 33 if the tests pass. |
|
@JeffryBooher sure. no problem. Let me know, if it needs any more changes. |
|
Looks good. All tests pass. Merging! Thanks @sathyamoorthi! |
"close others" extension added to default extensions directory.
|
@JeffryBooher Wow ! Thanks for pulled it in. Let me know if it needs any change at any time in future. |
#4469