Project manager revamp #9015

dangoor · 2014-09-08T16:23:10Z

This is an updated/rebased ProjectManager revamp for #8788. This replaces #8885.

Here's my current list of issues/things to test that I'm looking into:

Failing ProjectManager tests
Switching project not updating the tree?
- when opening brackets-git (which has a src folder) src is not getting updated. A merge algorithm problem?
Selected directory is missing trailing slash in ProjectModel._selections
brackets-git won't refresh on filesystem changes
- need to ensure that when an item has to refresh its object is replaced
Test project base URL functionality
Right clicking on another node while the context menu is up does not move the context to that other node
Clear TODOs in new code
Rather than selectIfVisible the old code looks at whether the current doc has become visible. curDoc = DocumentManager.getCurrentDocument(); and then if (_hasFileSelectionFocus() && curDoc && data)
Error cases for create and rename
Creation errors are not visible (that window timeout thing)
Check for invalid filenames on rename
The double click to rename shouldn't take effect if it's been too long since the first click
Control-double click toggles siblings
Alt-double click is supposed to collapse subtree but does not seem to do anything for me.
Improve _fileSystemChange handler?
ProjectManager.showInTree needs to expand nodes and not do anything if the path is outside the project or doesn't exist
Don't select directories
ProjectManager.refreshFileTree needs to make sure it doesn't run multiple times and has the 1 second delay between runs if there's one pending. Also double check that it's returning a promise that's resolved when it's done.
Run all tests
Positioning is wrong when node is scrolled into view (too far left)
Verify ProjectManager events
- projectRefresh doesn't seem likely to be fired!
Can ViewUtils.getFileEntryDisplay go away?
Close dropdowns on scroll (Menus.closeAll())
ProjectManager._fileSystemRename may not be used currently, but should be.
Make the actionCreator private in ProjectManager
Make treeData private in FTVM
Delete jstree
The old JSTree code would not select a directory if you click the disclosure triangle, but would select it if you click on the name. Do we care about that? That was probably because there was no differentiation between context and selection.
Find In not working on directory [Project Manager Revamp] Search in Folder doesn't work #8944
make (allow?) iconProviders to pass in a string

ingorichter · 2014-09-08T17:21:52Z

okay, I'm going to make my changes in this PR now. Thanks for going through all this pain merging master with your branch.

The DOM structure has changed a little in the tree.

This was very hard to track down because our promises and event handlers swallowed an exception thrown in FileTreeViewModel.

The project files cache was not being cleared on every file system change, as it had been previously. Also removed duplicate projectClose event.

…s opening files from working set

marcelgerber · 2014-09-09T12:52:56Z

@dangoor Apparently, we need to set devel: true in .jshintrc in order to get the Travis build fixed.

…to ProjectManager

…ed errors opening files from working set" This reverts commit 83b95c4.

…for top-level directories, but not for subdirectories yet (see failing test)

dangoor · 2014-09-18T15:05:32Z

I'm seeing this error when I launch Brackets:

Exception when calling a 'brackets done loading' handler: AppInit.js:95
TypeError: Cannot call method 'innerHeight' of undefined
    at Object._updateLayout (file:///Applications/Brackets.app/Contents/dev/src/view/MainViewManager.js:896:30)
    at Object.o.event.dispatch (file:///Applications/Brackets.app/Contents/dev/src/thirdparty/jquery-2.1.0.min.js:3:6055)
    at Object.r.handle (file:///Applications/Brackets.app/Contents/dev/src/thirdparty/jquery-2.1.0.min.js:3:2830)
    at Object.o.event.trigger (file:///Applications/Brackets.app/Contents/dev/src/thirdparty/jquery-2.1.0.min.js:3:5163)
    at Object.<anonymous> (file:///Applications/Brackets.app/Contents/dev/src/thirdparty/jquery-2.1.0.min.js:3:11000)
    at Function.o.extend.each (file:///Applications/Brackets.app/Contents/dev/src/thirdparty/jquery-2.1.0.min.js:2:2877)
    at o.fn.o.each (file:///Applications/Brackets.app/Contents/dev/src/thirdparty/jquery-2.1.0.min.js:2:818)
    at o.fn.extend.trigger (file:///Applications/Brackets.app/Contents/dev/src/thirdparty/jquery-2.1.0.min.js:3:10976)
    at triggerUpdateLayout (file:///Applications/Brackets.app/Contents/dev/src/view/WorkspaceManager.js:111:20)
    at Object.recomputeLayout (file:///Applications/Brackets.app/Contents/dev/src/view/WorkspaceManager.js:233:9) AppInit.js:96

Not sure if that happens on master.

MiguelCastillo · 2014-09-18T15:13:13Z

src/project/ProjectModel.js

+     * @param {Array.<File>=} additionalFiles Additional files to include (for example, the WorkingSet)
+     *          Only adds files that are *not* under the project root or untitled documents.
+     *
+     * @return {$.Promise} Promise that is resolved with an Array of File objects.


nit: everywhere in this file we are referring to jQuery.Promise instead of $.Promise.

Both are in use in the code... $.Promise seems to be in the lead with 180 matches vs. 70 matches for jQuery.Promise. Maybe I'll just switch them all to $.Promise.

By which I mean switch all of them in the new ProjectManager files

ingorichter · 2014-09-18T17:10:28Z

@dangoor yes, I see this error too. And yes, I still can't comment on ProjectManager.js.

ingorichter · 2014-09-18T19:15:30Z

Hey @dangoor, here is my list of comments for ProjectManager.js

L159 it seems that e is not used anymore or docs are missing
L413 to create the context we could call _getProjectViewStateContext() (like in _savePreferences
L476 wouldn't it be better to wait for the promise that is returned from model.reopenNodes before d will be resolved?
L585 is forceRender still required?
L593 can we remove this line and the comment above when this is no longer required?
L593 we should include the err in the rejected promise
L1216 where is shouldShow() defined? Or is this outdated information?
L1250 I think the comment is not quite right anymore. We should mention that we accept a React.DOM.ins instance or a jQuery object

General:

use $.Promise or jQuery.Promise

dangoor · 2014-09-18T20:24:47Z

I tried replacing the forceRender property with a call to forceUpdate and ultimately came to the conclusion that forceUpdate wasn't working the way it says it should. I've posted a message in the React group about this.

ingorichter · 2014-09-18T21:37:41Z

I'm eager to see their answer.

dangoor · 2014-09-19T12:53:30Z

No word from the React people, so I've gone ahead and documented forceRender, since it works...

dangoor · 2014-09-19T12:55:21Z

@ingorichter Are you fine with my proposed behavior for control-alt-click?

if the node is open, close it and all of its children
if the node is closed, open it and its immediate children

I think this is more consistent than the old behavior, even if it's not 100% consistent. (Opening all children could potentially freeze up Brackets if you've got a lot of files...)

dangoor · 2014-09-19T13:07:35Z

Regarding your comments about ProjectManager.js:

L159 it seems that e is not used anymore or docs are missing
- It's the jQuery event object. I've updated the docs.
L413 to create the context we could call _getProjectViewStateContext() (like in _savePreferences
- Good call. Fixed
L476 wouldn't it be better to wait for the promise that is returned from model.reopenNodes before d will be resolved?
- Not really because that can be done asynchronously and is not required to get the UI in front of the user. I agree this doesn't seem ideal, but I think the time to fix it (especially since this is private API) is when someone tackles turning _loadProject into something more reasonable).
L585 is forceRender still required?
- Yes. I updated the docs.
L593 can we remove this line and the comment above when this is no longer required?
- Removed. It no longer works this way
L593 we should include the err in the rejected promise
- I think this line number is wrong. I'm assuming you mean line 693 in _unwatchProjectRoot
L1216 where is shouldShow() defined? Or is this outdated information?
- updated to reflect that this is now in ProjectModel
L1250 I think the comment is not quite right anymore. We should mention that we accept a React.DOM.ins instance or a jQuery object
- updated

dangoor · 2014-09-19T13:11:04Z

Changes pushed.

Also turn on es5 mode for JSLint and JSHint so we can call .delete directly. We wouldn't support any non-es5 browsers anyhow.

ingorichter · 2014-09-19T20:50:54Z

@dangoor I'm okay with your changes to the control+alt+click behavior.
Yes, I meant 693. Sorry for the confusion.

ingorichter · 2014-09-19T22:30:01Z

I think the travis failure needs to be fixed before we can merge it into master.

dangoor · 2014-09-20T02:17:19Z

Grr... that's really annoying. JSHint is complaining about a JSLint option.

dangoor · 2014-09-20T02:34:21Z

OK, found a combo that makes both JSHint and JSLint happy.

dangoor · 2014-09-22T12:15:56Z

@ingorichter since it sounds like you're ready to merge into master, I won't make any more changes on the branch. I'll target additional bug fixes from this point against master.

Project manager revamp

ingorichter · 2014-09-22T15:41:50Z

Thanks @dangoor. Merged.

Rebased project manager

5181528

dangoor and others added 5 commits September 8, 2014 14:50

Changes based on review comments

836ea68

Fix merge error (WORKING_SET_VIEW) and update working set test.

fdc7e68

The DOM structure has changed a little in the tree.

Fix problem with DragAndDrop test.

058bc25

This was very hard to track down because our promises and event handlers swallowed an exception thrown in FileTreeViewModel.

Fix problem with InlineEditorProviders-test.

bd31aad

The project files cache was not being cleared on every file system change, as it had been previously. Also removed duplicate projectClose event.

- fixed typo in FileViewController.WORKINGSET_VIEW which caused error…

83b95c4

…s opening files from working set

dangoor added 4 commits September 9, 2014 15:49

Fix context menu behavior when you right click from one node to another.

793d425

Merge branch 'ProjectManager' of https://github.com/adobe/brackets in…

4032116

…to ProjectManager

Revert "- fixed typo in FileViewController.WORKINGSET_VIEW which caus…

576f35c

…ed errors opening files from working set" This reverts commit 83b95c4.

Another couple of WORKINGSET_VIEW cleanups

d797cf8

dangoor mentioned this pull request Sep 9, 2014

[REVIEW ONLY] Project manager revamp #8885

Closed

dangoor and others added 17 commits September 9, 2014 16:24

Right-clicks should not bubble up.

a2ed861

Close menus on scroll.

d5734ef

Fix a nit

b56bcc2

Better logic for selecting current file when opening a directory.

dbc7ff6

Remove dependency on FileViewController in ProjectModel

849701c

showInTree is fully implemented, directories are not selectable.

6f8085b

Allow console statements

d21a090

- first implementation of expand/collapse subdirectories. This works …

08427ab

…for top-level directories, but not for subdirectories yet (see failing test)

Check for invalid filenames on rename

11a7085

Restore original refresh behavior (once per second max)

9066335

Fix renaming and project switching. Set width on rename input.

503534b

Make FileTreeViewModel.treeData and ProjectManager.actionCreator private

0e05c37

Remove jstree.

afce297

Icon providers can pass in strings or jQuery objects now

82346ae

Fix test failures caused by making treeData private.

967caec

Fix a hidden file selection bug (and ProjectManager integration tests)

f7b27b8

- merged latest head

8d7be8d

Respond to review comments

6ae61cc

MiguelCastillo reviewed Sep 18, 2014
View reviewed changes

Consistent use of $.Promise in the new code.

9faf2d9

dangoor added 2 commits September 19, 2014 08:26

Merge branch 'master' into ProjectManager

242ffdf

Document forceRender

5b5c2b3

Respond to review comments

def653b

ctrl-alt-click on a directory recursively closes the whole subtree.

0a42f07

Also turn on es5 mode for JSLint and JSHint so we can call .delete directly. We wouldn't support any non-es5 browsers anyhow.

dangoor mentioned this pull request Sep 19, 2014

Clicking an Image file in the project tree calls FileOpen command twice #6513

Closed

es5 option apparently causes other issues.

7869529

Move the JSLint es5 option for FileTreeViewModel to make JSHint happy.

5469b7c

Mark-Simulacrum mentioned this pull request Sep 22, 2014

Add encodeFilePath to FileUtils to convert file paths into encoded strings (replace metacharacters such as %, @, $, #, etc.) #9190

Merged

ingorichter added a commit that referenced this pull request Sep 22, 2014

Merge pull request #9015 from adobe/ProjectManager

d094245

Project manager revamp

ingorichter merged commit d094245 into master Sep 22, 2014

dangoor deleted the ProjectManager branch September 23, 2014 14:36

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Project manager revamp brackets-archive/bracketsIssues#8053

Open

30 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project manager revamp #9015

Project manager revamp #9015

dangoor commented Sep 8, 2014

ingorichter commented Sep 8, 2014

marcelgerber commented Sep 9, 2014

dangoor commented Sep 18, 2014

MiguelCastillo Sep 18, 2014

dangoor Sep 18, 2014

dangoor Sep 18, 2014

ingorichter commented Sep 18, 2014

ingorichter commented Sep 18, 2014

dangoor commented Sep 18, 2014

ingorichter commented Sep 18, 2014

dangoor commented Sep 19, 2014

dangoor commented Sep 19, 2014

dangoor commented Sep 19, 2014

dangoor commented Sep 19, 2014

ingorichter commented Sep 19, 2014

ingorichter commented Sep 19, 2014

dangoor commented Sep 20, 2014

dangoor commented Sep 20, 2014

dangoor commented Sep 22, 2014

ingorichter commented Sep 22, 2014

Project manager revamp #9015

Project manager revamp #9015

Conversation

dangoor commented Sep 8, 2014

ingorichter commented Sep 8, 2014

marcelgerber commented Sep 9, 2014

dangoor commented Sep 18, 2014

MiguelCastillo Sep 18, 2014

Choose a reason for hiding this comment

dangoor Sep 18, 2014

Choose a reason for hiding this comment

dangoor Sep 18, 2014

Choose a reason for hiding this comment

ingorichter commented Sep 18, 2014

ingorichter commented Sep 18, 2014

dangoor commented Sep 18, 2014

ingorichter commented Sep 18, 2014

dangoor commented Sep 19, 2014

dangoor commented Sep 19, 2014

dangoor commented Sep 19, 2014

dangoor commented Sep 19, 2014

ingorichter commented Sep 19, 2014

ingorichter commented Sep 19, 2014

dangoor commented Sep 20, 2014

dangoor commented Sep 20, 2014

dangoor commented Sep 22, 2014

ingorichter commented Sep 22, 2014