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

Open file from tree#12

Merged
peterflynn merged 22 commits into
masterfrom
open-file-from-tree
Dec 16, 2011
Merged

Open file from tree#12
peterflynn merged 22 commits into
masterfrom
open-file-from-tree

Conversation

@njx
Copy link
Copy Markdown

@njx njx commented Dec 14, 2011

Implemented opening a file by clicking on it in the file tree, and also implemented File > Open to show a file dialog that the user can select from.

As part of this, I created a simple CommandManager to start to decouple parts of the UI from each other. When a file in the tree is clicked, the ProjectManager tells the CommandManager to execute the "file.open" command. This command is registered in brackets.js as part of the boot sequence.

Eventually we can add things like key binding, and perhaps undo/redo handling, to the CommandManager. Also, we should consider factoring out some of the code in brackets.js out into a more formal application controller.

Comment thread src/CommandManager.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fwiw, the official Closure docs put the type before the parameter name. I don't know if it also accepts this reverse order (which I actually think is nicer... so maybe it's worth testing whether this will work?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Let's follow the official docs.

@njx
Copy link
Copy Markdown
Author

njx commented Dec 15, 2011

FYI, this is not yet ready for master--waiting on finalizing Ty's pull request for the open file API.

Comment thread src/Commands.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should use JSDoc comment style for top-level declarations

Comment thread src/brackets.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The first time I read this I thought it meant the browser title bar... should we call it something different to disambiguate?

Comment thread src/brackets.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does our FileEntry.file() take an error callback as a 2nd arg, like the PhoneGap one? If so, I wonder if we should add a "TODO" error callback like you did above with the reader. That way we'll have TODOs for all known cases of missing error handling.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants