Remember current directory for each volume.

Remember current directory for each volume.

BUG=chromium-os:29459
TEST=Changing volume preserves current directory on each volume. Clicking on the current volume changes directory to this volumes root.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133443

[Vladislav Kaznacheev, 2012-04-20 14:55:11]

https://chromiumcodereview.appspot.com/10146008/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10146008/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:237: };
nit: if you renamed these functions in place it would be easier for further
generations to look at the change history

[Oleg Eterevsky, 2012-04-20 15:00:32]

https://chromiumcodereview.appspot.com/10146008/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10146008/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:237: };
On 2012/04/20 14:55:11, Vladislav Kaznacheev wrote:
> nit: if you renamed these functions in place it would be easier for further
> generations to look at the change history

Unfortunately, they were defined as fields in

DirectoryModel.prototype = {
...
};

I had to move them out of this map literal to define them as proper methods.

[Oleg Eterevsky, 2012-04-20 15:48:41]

I've updated the CL to refactor the usage of rootsListSelection.

[SeRya, 2012-04-20 16:06:46]

https://chromiumcodereview.appspot.com/10146008/diff/5002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/directory_model.js (left):

https://chromiumcodereview.appspot.com/10146008/diff/5002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:1023:
DirectoryModel.prototype.onRootsSelectionChanged_ = function(event) {
What's wrong with onRootsSelectionChanged_? I think DirectoryModel is a wrong
place to handle clicks.

[Oleg Eterevsky, 2012-04-23 10:50:40]

https://chromiumcodereview.appspot.com/10146008/diff/5002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/directory_model.js (left):

https://chromiumcodereview.appspot.com/10146008/diff/5002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:1023:
DirectoryModel.prototype.onRootsSelectionChanged_ = function(event) {
On 2012/04/20 16:06:46, SeRya wrote:
> What's wrong with onRootsSelectionChanged_? I think DirectoryModel is a wrong
> place to handle clicks.

I agree, but this event is not fired when user clicks on the item that is
already selected. So, the click should be handled.

[Oleg Eterevsky, 2012-04-23 11:24:44]

Moved listener registration from DirectoryModel to FileManager, as Vlad
suggested.

[Oleg Eterevsky, 2012-04-23 11:46:43]

Moved the event handler to FileManager. Handle failed changeDirectory while
changing root.

[SeRya, 2012-04-23 14:23:43]

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/directory_model.js:639: var onError =
function(error) {
var onError = opt_onError || function(error) { ... }

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/directory_model.js:680:
this.currentDirByRoot_[this.getCurrentRootPath()] = dirEntry.fullPath;
Is it OK this is a page-wide state, not application-wide?

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/directory_model.js:1005:
this.changeDirectory(rootPath);
The same logic makes sense in setupPath. If user happen to navigate to a deleted
directoy it makes sense to drop her yo the root rather than to the Downloads. I
think error handling logic should be part of changeDirectory. Hardly we need
either handle such an error nor ignore it.

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/file_manager.js:1830:
this.directoryModel_.changeDirectory(entry.fullPath);
I think this code would be much more clear:

dm.changeDirectory(dm.getActivePathForRoot(entry.fullPath))

(assuming changeDirectory handles errors and getActivePathForRoot returns last
selected directory or root itself).

[Oleg Eterevsky, 2012-04-23 14:33:04]

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/directory_model.js:639: var onError =
function(error) {
On 2012/04/23 14:23:43, SeRya wrote:
> var onError = opt_onError || function(error) { ... }

Good idea. Done.

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/directory_model.js:680:
this.currentDirByRoot_[this.getCurrentRootPath()] = dirEntry.fullPath;
On 2012/04/23 14:23:43, SeRya wrote:
> Is it OK this is a page-wide state, not application-wide?

You mean that we lose the state when we close the given window? If I understand
correctly, that's what we want.

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/directory_model.js:1005:
this.changeDirectory(rootPath);
On 2012/04/23 14:23:43, SeRya wrote:
> The same logic makes sense in setupPath. If user happen to navigate to a
deleted
> directoy it makes sense to drop her yo the root rather than to the Downloads.
I
> think error handling logic should be part of changeDirectory. Hardly we need
> either handle such an error nor ignore it.

I'm not sure about it. I'd say if user just changes directory and the operation
fails, it shouldn't move him to the root, but rather should leave him in the
current directory and probably give him some error alert.

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/10146008/diff/15001/chrome/browser/res...
chrome/browser/resources/file_manager/js/file_manager.js:1830:
this.directoryModel_.changeDirectory(entry.fullPath);
On 2012/04/23 14:23:43, SeRya wrote:
> I think this code would be much more clear:
> 
> dm.changeDirectory(dm.getActivePathForRoot(entry.fullPath))
> 
> (assuming changeDirectory handles errors and getActivePathForRoot returns last
> selected directory or root itself).

I don't see how will it make the code simpler. The 'if' here handles the case
when user clicks on the already active root. It has nothing to do with any
'active' directory for root.

[SeRya, 2012-04-23 15:06:53]

lgtm