adobe / brackets

An open source code editor for the web, written in JavaScript, HTML and CSS.
http://brackets.io
MIT License
33.25k stars 7.63k forks source link

Eliminate DocumentManager.closeFullEditor() - move logic elsewhere #513

Open peterflynn opened 12 years ago

peterflynn commented 12 years ago

Randy and I talked about this a bit during pull #508. It's weird that DocumentManager owns part of the UI-oriented business logic for closing full editors. Having this method in DocumentManager feels odd, and leads to confusion when contrasted with notifyFileDeleted(), which performs similar actions.

There are a few ways we might tackle this...

Lazy proposal: Wait until we have file/directory watchers. Then DocumentManager can listen directly to the our newfound "file/directory model," and notifyFileDeleted() would become a private listener function. Having only closeFullEditor() be a public API doesn't feel so bad.

Minimal-churn proposal: Make removeFromWorkingSet() public and redefine it to basically do everything that closeFullEditor() does (i.e. what it does now, plus ensuring the removed file isn't the currentDocument). The FILE_CLOSE command would call this method, and we could remove closeFullEditor().

We could also factor out a getNextEditor() API that could be shared between the ensure-not-currentDocument logic and future commands like Ctrl+Tab.

Low-churn proposal: Make removeFromWorkingSet() public but have it not do anything more than it does today. Also add a public getNextEditor() as above. Then the FILE_CLOSE command would basically do what closeFullEditor() does today, leaning on those two DocumentManager APIs.

Downside: DocumentManager still needs that same behavior, in the case where a document's file has been wholly deleted on disk. So there'd be some code duplication between DocumentCommandHandlers (FILE_CLOSE) and DocumentManager (notifyFileDeleted()).

Higher-churn 'clean' proposal: It feels a bit archaic that DocumentManager owns the working set and currentDocument state at all. So, move that stuff to EditorManager; DocumentManager would only manage the list of 'open'/active (refCount > 0) Documents. EditorManager would have a public API much like today's closeFullEditor(). FILE_CLOSE would call it directly, and in the case of deletion EditorManager could probably just listen directly for its Editors' "lostContent" event.

This decouples things a bit more cleanly, but it involves moving a lot of code/APIs from DocumentManager to EditorManager, and updating all callers accordingly.

njx commented 12 years ago

QRB reviewed

peterflynn commented 10 years ago

Could maybe be tackled in a batch cleanup along with #1451 & #5706

marcelgerber commented 9 years ago

The method is now deprecated (I guess due to Split View, but I haven't looked into it), so this can probably be closed.