fix issue #4238- 'Save As on file in working set does not preserve working set order' #4437
Conversation
ghost
assigned
There was a problem hiding this comment.
This item should be added to the structure above:
{!{fullPath:string, index:number=}}
There was a problem hiding this comment.
ah, ok. I wasn't sure on the format of these comments. Thanks for the clarification.
|
This change is definitely simpler (and cleaner) than #4329, but it does cause a big flicker because the working set is redrawn twice: once when the old item is removed, and again when the new file is added. It would be nice if that could be a single redraw. |
|
Initial review complete. |
…f the item from the working set, thereby reducing the flicker otherwise caused by two redraws of the working set list
|
@gruehle Thanks again for the initial review. I've committed two changes that should take care of all of your concerns. Can you please revisit this review to confirm that you approve? |
src/document/DocumentManager.js
Outdated
There was a problem hiding this comment.
You can just use if (!suppressRedraw) here. This handles undefined, null and false correctly.
There was a problem hiding this comment.
hehehe. Javascript newbie. thanks!
|
@bchin - re-review complete. |
…ngSetView handler
|
@gruehle Latest changes have been committed. Thanks again for catching those, especially the issue about skipping the listener. Ready for review. :-) |
src/project/WorkingSetView.js
Outdated
There was a problem hiding this comment.
This should be @param {boolean=} suppressRedraw ... (we really need to have a closure compilation task to catch these kind of things...)
|
@bchin - one tiny change left. I would have done the change myself, but then I saw that the pull request cannot be automatically merged. Could you merge with master? Thanks! |
…to manually resolve conflicts)
|
@gruehle Got it. I've fixed that last comment, and then manually resolved the merge with master. Should be ready to merge pending your final approval. Thanks again! |
|
@bchin I'm still seeing a redraw after the file is removed and an additional redraw once the file is added. It seems like the suppressRedraw flag isn't working correctly, or something else is forcing a redraw. |
…de so that the working set list isn't changed at all
|
Looks great! Merging. |
fix issue #4238- 'Save As on file in working set does not preserve working set order'
alternate fix for issue #4238 that replaces previously submitted pull request #4329
The other pull request tries to more intelligently swap out the working set entries, but fails to open the newly saved document. This approach leverages the already existing workflow in opening and adding the newly saved file to the working set. However, rather than just appending the new file to the end of the working set list, the new code now supports an optional parameter which specifies where to insert it. Consequently, we can insert the file into the same working order as it was before the file-save-as event.