Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10342010:
Add gdata content search to file_manager (Closed)
|
Created:
8 years, 7 months ago by tbarzic Modified:
8 years, 7 months ago CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd gdata content search to file_manager
Search results will be shown under virtual path gdata/.search/<query>.
Content search will be triggered whenever we try to enumerate directory with
the mentioned file path format.
Entries returned by search results will have file names formatted as
<resource_id>.<real_file_name>, so we have to strip resource_id part before
displaying name or selecting target path for copy/move operations.
When user enters some input x in search box(on gdata only),
file manager will change directory to /gdata/.search/x in
which search results will be displayed
TEST = manual
BUG=chromium-os:27539
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137536
Patch Set 1 #Patch Set 2 : nits #Patch Set 3 : some fixes #
Total comments: 30
Patch Set 4 : feedback #
Total comments: 14
Patch Set 5 : .. #Patch Set 6 : style nits #
Total comments: 8
Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : stil bit dirty #Patch Set 10 : some cleanup #Patch Set 11 : . #
Total comments: 14
Patch Set 12 : . #Patch Set 13 : . #Patch Set 14 : . #
Total comments: 18
Patch Set 15 : . #Patch Set 16 : few nits #Patch Set 17 : remove search for saveas #Patch Set 18 : . #
Total comments: 42
Patch Set 19 : rebase #Patch Set 20 : . #Patch Set 21 : rebase + few fixes realted to 'no results' div #Patch Set 22 : nit #Patch Set 23 : fix indent #
Messages
Total messages: 20 (0 generated)
hey guys, can you take a look at this. btw. if you think more than few review iteration will be needed, let me know, so I can stay up a bit later than usual tomorrow.. Toni
On 2012/05/03 03:24:13, tbarzic wrote: > hey guys, > can you take a look at this. > > btw. if you think more than few review iteration will be needed, let me know, so > I can stay up a bit later than usual tomorrow.. > > Toni oh, and this needs https://chromiumcodereview.appspot.com/10274002/ to work..
https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_copy_manager.js:498: // Note that if the entry is GData search result, it can only be imediate typo: imediate https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_copy_manager.js:505: task.registerRename(gdataSearchResult.fileName, I think, you have to register full paths here instead of just file names. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_copy_manager.js:508: originalPath = gdataSearchResult.displayName; For directory, you do: 1. register rename 2. change fileName to displayName 3. apply rename (from fileName to displayName) I think, you should just skip step2 (or some cryptic names like 'accidental_resource_id.a.txt' will be renamed twice). https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:981: var shouldCreate = util.shouldCreateOnGDataPath( This is not very intuitive: you ask for something about gdata, while it is not always gdata. Consider renaming the method to something like |util.is{Special|System}ReadonlyDirectory|. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:2885: if (i == 1 && pathNames[0] == 'gdata' && pathNames[1] == '.search') { |path == util.getGDataSearchRoot()| ? https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:4306: this.directoryModel_.changeDirectory('/gdata/'); 1. Use |'/' + DirectoryModel.GDATA_DIRECTORY| 2. I think, we should save the current directory when user types the first symbol in search box, and restore it here. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... File chrome/browser/resources/file_manager/js/util.js (right): https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:554: * or undefined if the path is not gdata search result path. Use null instead of undefined.
https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... File chrome/browser/resources/file_manager/js/util.js (right): https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:510: getGDataSearchRootPath: function() { Define constants instead? /** * @const * @type {string} */ var GDATA_SEARCH_ROOT_PATH = '/gdata/.search'; /** * @const * @type {Array.<string>} */ var GDATA_SEARCH_ROOT_COMPONENTS = GDATA_SEARCH_ROOT_PATH.split('/'); https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:518: * @param {string} Search query. Please add the parameter name: @param {string} query Search query. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:529: * @param {string} Path which is being tested. Please add the parameter name: @param {string} path Path which is being tested. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:549: * files fileName in virtual search file system, and display name we that ... display name that ... https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:552: * @param {string} The potential gdata search result path. Please add the parameter name. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:554: * or undefined if the path is not gdata search result path. @return {Object.<string,string>?} https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:559: // Nothing to do it the path is not under gdata search root path. ... to do IF the path ... https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:566: // Search result should be formated like: formatted
https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_copy_manager.js:498: // Note that if the entry is GData search result, it can only be imediate On 2012/05/03 11:21:47, dgozman wrote: > typo: imediate Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_copy_manager.js:505: task.registerRename(gdataSearchResult.fileName, On 2012/05/03 11:21:47, dgozman wrote: > I think, you have to register full paths here instead of just file names. Don't we register paths relative to sourceDir. Since dirs for which we have to this renaming can only be immediate children of the source dir, this should be ok. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_copy_manager.js:508: originalPath = gdataSearchResult.displayName; On 2012/05/03 11:21:47, dgozman wrote: > For directory, you do: > 1. register rename > 2. change fileName to displayName > 3. apply rename (from fileName to displayName) > > I think, you should just skip step2 (or some cryptic names like > 'accidental_resource_id.a.txt' will be renamed twice). registered rename paths are automatically appended '/', so we also have to do step 2 for directories (apply rename won't change the original path because it doesn't have a trailing /). I thought about doing this in FileCopyManager.Task.setEntries, but I'd have to register renames without trailing slash then, which would increase the risk that we apply rename for a entry whose dir/file name only partially matches the registered rename. (and, to be honest, this seems fragile enough :/) https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:981: var shouldCreate = util.shouldCreateOnGDataPath( On 2012/05/03 11:21:47, dgozman wrote: > This is not very intuitive: you ask for something about gdata, while it is not > always gdata. > > Consider renaming the method to something like > |util.is{Special|System}ReadonlyDirectory|. Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:2885: if (i == 1 && pathNames[0] == 'gdata' && pathNames[1] == '.search') { On 2012/05/03 11:21:47, dgozman wrote: > |path == util.getGDataSearchRoot()| ? Yep, you're right, I haven't read the method carefully enough.. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/file_manager.js:4306: this.directoryModel_.changeDirectory('/gdata/'); On 2012/05/03 11:21:47, dgozman wrote: > 1. Use |'/' + DirectoryModel.GDATA_DIRECTORY| > 2. I think, we should save the current directory when user types the first > symbol in search box, and restore it here. Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... File chrome/browser/resources/file_manager/js/util.js (right): https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:510: getGDataSearchRootPath: function() { On 2012/05/03 15:52:24, Oleg wrote: > Define constants instead? > > /** > * @const > * @type {string} > */ > var GDATA_SEARCH_ROOT_PATH = '/gdata/.search'; > > /** > * @const > * @type {Array.<string>} > */ > var GDATA_SEARCH_ROOT_COMPONENTS = GDATA_SEARCH_ROOT_PATH.split('/'); Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:518: * @param {string} Search query. On 2012/05/03 15:52:24, Oleg wrote: > Please add the parameter name: > > @param {string} query Search query. Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:529: * @param {string} Path which is being tested. On 2012/05/03 15:52:24, Oleg wrote: > Please add the parameter name: > > @param {string} path Path which is being tested. Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:549: * files fileName in virtual search file system, and display name we that On 2012/05/03 15:52:24, Oleg wrote: > ... display name that ... Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:552: * @param {string} The potential gdata search result path. On 2012/05/03 15:52:24, Oleg wrote: > Please add the parameter name. Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:554: * or undefined if the path is not gdata search result path. On 2012/05/03 15:52:24, Oleg wrote: > @return {Object.<string,string>?} Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:554: * or undefined if the path is not gdata search result path. On 2012/05/03 11:21:47, dgozman wrote: > Use null instead of undefined. Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:559: // Nothing to do it the path is not under gdata search root path. On 2012/05/03 15:52:24, Oleg wrote: > ... to do IF the path ... Done. https://chromiumcodereview.appspot.com/10342010/diff/4001/chrome/browser/reso... chrome/browser/resources/file_manager/js/util.js:566: // Search result should be formated like: On 2012/05/03 15:52:24, Oleg wrote: > formatted Done.
https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:992: this.fileTransferController_.queryPasteCommandEnabled() && What about dropping files into the search results? https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2888: div.classList.add('breadcrumb-last'); breadcrumb-last in the middle of the path looks strange. May be it should be renamed. if (i == pathNames.length - 1 || path == util.GDATA_SEARCH_ROOT_PATH) {...} is shorter. https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3603: this.directoryModel_.doesExist(newName, function(exists, isFile) { I'd suggest to isolate the UI from knowlage about name transformation. Let's DirectoryModel handles it. doesExist and renameEntry should add the resource_id if neccessary, DirectoryModel should expose getDieplayName(entry) method. https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4294: if (!this.isOnGData()) { I think this is too low level logic. It should be placed into the DirectoryModel. Let's introduce DirectoryModel.prototype.search = function(queryString) https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4307: this.directoryModel_.changeDirectory( changeDirectory causes putting the new directory into the browser history. I don't think it's a good idea to add one record per key press into the history. https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4316: '/' + DirectoryModel.GDATA_DIRECTORY; You set GData root even if search started in a subfolder.
This version looks good. If you will move some code to DirectoryModel (as Sergey suggests), I will take another look. https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... File chrome/browser/resources/file_manager/js/util.js (right): https://chromiumcodereview.appspot.com/10342010/diff/10002/chrome/browser/res... chrome/browser/resources/file_manager/js/util.js:551: return this.GDATA_SEARCH_ROOT_PATH + '/' + query; I'm almost sure that presubmit check will warn about this. Better use |util.GDATA....| here and in other constant usages.
http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:992: this.fileTransferController_.queryPasteCommandEnabled() && On 2012/05/04 10:26:17, SeRya wrote: > What about dropping files into the search results? Done. http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:2888: div.classList.add('breadcrumb-last'); On 2012/05/04 10:26:17, SeRya wrote: > breadcrumb-last in the middle of the path looks strange. May be it should be > renamed. > > if (i == pathNames.length - 1 || path == util.GDATA_SEARCH_ROOT_PATH) {...} is > shorter. Done http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:3603: this.directoryModel_.doesExist(newName, function(exists, isFile) { On 2012/05/04 10:26:17, SeRya wrote: > I'd suggest to isolate the UI from knowlage about name transformation. Let's > DirectoryModel handles it. > > doesExist and renameEntry should add the resource_id if neccessary, > DirectoryModel should expose getDieplayName(entry) method. Really good point, that's much cleaner :) http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:4294: if (!this.isOnGData()) { On 2012/05/04 10:26:17, SeRya wrote: > I think this is too low level logic. It should be placed into the > DirectoryModel. Let's introduce DirectoryModel.prototype.search = > function(queryString) Done. http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:4307: this.directoryModel_.changeDirectory( On 2012/05/04 10:26:17, SeRya wrote: > changeDirectory causes putting the new directory into the browser history. I > don't think it's a good idea to add one record per key press into the history. Done. http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:4316: '/' + DirectoryModel.GDATA_DIRECTORY; On 2012/05/04 10:26:17, SeRya wrote: > You set GData root even if search started in a subfolder. are you sure? What am I missing? http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/util.js (right): http://codereview.chromium.org/10342010/diff/10002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/util.js:551: return this.GDATA_SEARCH_ROOT_PATH + '/' + query; On 2012/05/04 11:39:36, dgozman wrote: > I'm almost sure that presubmit check will warn about this. Better use > |util.GDATA....| here and in other constant usages. Done.
https://chromiumcodereview.appspot.com/10342010/diff/17002/chrome/browser/res... File chrome/browser/resources/file_manager/js/directory_model.js (right): https://chromiumcodereview.appspot.com/10342010/diff/17002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:662: searchResultName ? searchResultName.resourceId + '.' + newName : newName; Code duplication. Create a method. https://chromiumcodereview.appspot.com/10342010/diff/17002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1179: this.lastNonGDataSearchDirPath_; As I mentioned before I don't think that changing directory is an appropriate method for performing search. I suggest to introduce shaddow directory which would be used passed to DirectoryModel.Scanner to retrieve the file list. Just set the shaddow directory and call 'rescan'. https://chromiumcodereview.appspot.com/10342010/diff/17002/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/17002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2870: div.textContent = i == 0 ? this.getRootLabel_(path) : You are going to show in breadcrumbs path like "gdata > .search > firestkeyword secondkeyword > directory", right? If so i think: 1. '.search' looks strangely in breadcrumbs. It should be replaced with a user friendly string. 2. I think double click to a directory should navigate to the directory in its original path. Path like ".. > search query > dir1 > dir2" looks confusing. Please, clarify it with the UX team. 3. Search query in breadcrumbs duplicate contents of the search box. I think it shouldn't be displayed. Please, clarify it with the UX team. https://chromiumcodereview.appspot.com/10342010/diff/17002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3395: var shouldReplaceHistory = I think when search query change DirectoryModel musn't fire the directory-change event at all. It breaks abstraction. Searching and changing current directory are different events. File Manager shouldn't check free space and do many other things on pressing a key in the search box.
Please take another look.. http://codereview.chromium.org/10342010/diff/17002/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/directory_model.js (right): http://codereview.chromium.org/10342010/diff/17002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/directory_model.js:662: searchResultName ? searchResultName.resourceId + '.' + newName : newName; On 2012/05/07 14:06:00, SeRya wrote: > Code duplication. Create a method. Done. http://codereview.chromium.org/10342010/diff/17002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/directory_model.js:1179: this.lastNonGDataSearchDirPath_; On 2012/05/07 14:06:00, SeRya wrote: > As I mentioned before I don't think that changing directory is an appropriate > method for performing search. > > I suggest to introduce shaddow directory which would be used passed to > DirectoryModel.Scanner to retrieve the file list. Just set the shaddow directory > and call 'rescan'. Yep, this is much cleaner, thanks for suggestion ;) Done. http://codereview.chromium.org/10342010/diff/17002/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/10342010/diff/17002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:2870: div.textContent = i == 0 ? this.getRootLabel_(path) : On 2012/05/07 14:06:00, SeRya wrote: > You are going to show in breadcrumbs path like "gdata > .search > firestkeyword > secondkeyword > directory", right? > > If so i think: > 1. '.search' looks strangely in breadcrumbs. It should be replaced with a user > friendly string. > 2. I think double click to a directory should navigate to the directory in its > original path. Path like ".. > search query > dir1 > dir2" looks confusing. > Please, clarify it with the UX team. > 3. Search query in breadcrumbs duplicate contents of the search box. I think it > shouldn't be displayed. Please, clarify it with the UX team. With the newest version we will modifying breadcrumbs for search path only in extreme case when the user types search path in the address bar (the case about which we shouldn't care to much), so this should probably be good enough to handle that case. What do you think? http://codereview.chromium.org/10342010/diff/17002/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:3395: var shouldReplaceHistory = On 2012/05/07 14:06:00, SeRya wrote: > I think when search query change DirectoryModel musn't fire the directory-change > event at all. It breaks abstraction. Searching and changing current directory > are different events. File Manager shouldn't check free space and do many other > things on pressing a key in the search box. Done.
https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... File chrome/browser/resources/file_manager/js/directory_model.js (right): https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:640: DirectoryModel.prototype.doesExist = function(entry, newName, callback) { May be rename newName to newDisplayName (here and above)? https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:661: DirectoryModel.prototype.getNameToUseInRenaming_ = function(entry, newName) { This method effectively is opposite to getDisplayName. It would be easier to understand if it was named like getEntryName(entry, displayName) and placed next to getDisplayName. https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1201: this.currentDirEntry_ = searchDir; It's better but still flawed. You shouldn't change currentDirEntry_ not firing the directory-change event. It makes UI inconsistent with logical state. Let's have this.searchDirEntry_ and use it in createScanner_ in the following manner: return new DirectoryModel.Scanner(this.searchDirEntry_ || this.currentDirEntry_). You also may fire special event like search-query-changed if you need to reflect changes in UI. https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1003: this.directoryModel_.getCurrentDirEntry().fullPath); I think here should not be difference between local directory search and gdata search. In both cases creating a directory makes little sense. In case of local directory search new directory may immediately disappear as it doesn't correspond to the query before the user gave it a meaningful name. It should be this.directoryModel_.isSearch() or something like that. https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3362: // TODO(tbarzic): Is this still used? It's obsolete code. Please remove it. https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3441: if (this.directoryModel_.shouldClearSearch(oldDirEntry, newDirEntry)) I think the search box should be cleared unconditionally on each directory change. In case of search in a local directory keeping searching makes little sense as well. https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... File chrome/browser/resources/file_manager/js/util.js (right): https://chromiumcodereview.appspot.com/10342010/diff/28001/chrome/browser/res... chrome/browser/resources/file_manager/js/util.js:551: isGDataSearchPath: function(path) { Most of code for analyzing file paths placed into DirectoryModel. Please, consider moving it over there (as well as GDATA_SEARCH_ROOT_PATH and so on).
.
http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/directory_model.js (right): http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/directory_model.js:640: DirectoryModel.prototype.doesExist = function(entry, newName, callback) { On 2012/05/10 08:44:16, SeRya wrote: > May be rename newName to newDisplayName (here and above)? Done. http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/directory_model.js:661: DirectoryModel.prototype.getNameToUseInRenaming_ = function(entry, newName) { On 2012/05/10 08:44:16, SeRya wrote: > This method effectively is opposite to getDisplayName. It would be easier to > understand if it was named like getEntryName(entry, displayName) and placed next > to getDisplayName. Done. http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/directory_model.js:1201: this.currentDirEntry_ = searchDir; On 2012/05/10 08:44:16, SeRya wrote: > It's better but still flawed. You shouldn't change currentDirEntry_ not firing > the directory-change event. It makes UI inconsistent with logical state. > > Let's have this.searchDirEntry_ and use it in createScanner_ in the following > manner: > > return new DirectoryModel.Scanner(this.searchDirEntry_ || > this.currentDirEntry_). > > You also may fire special event like search-query-changed if you need to reflect > changes in UI. Done. http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:1003: this.directoryModel_.getCurrentDirEntry().fullPath); On 2012/05/10 08:44:16, SeRya wrote: > I think here should not be difference between local directory search and gdata > search. In both cases creating a directory makes little sense. In case of local > directory search new directory may immediately disappear as it doesn't > correspond to the query before the user gave it a meaningful name. > > It should be this.directoryModel_.isSearch() or something like that. Done. http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:3362: // TODO(tbarzic): Is this still used? On 2012/05/10 08:44:16, SeRya wrote: > It's obsolete code. Please remove it. Yeah, I though so, but wanted to check before I remove it :) http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:3441: if (this.directoryModel_.shouldClearSearch(oldDirEntry, newDirEntry)) On 2012/05/10 08:44:16, SeRya wrote: > I think the search box should be cleared unconditionally on each directory > change. In case of search in a local directory keeping searching makes little > sense as well. Yeah, I found that a bit strange too, but had decided to keep original behaviour.. Done. http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/util.js (right): http://codereview.chromium.org/10342010/diff/28001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/util.js:551: isGDataSearchPath: function(path) { On 2012/05/10 08:44:16, SeRya wrote: > Most of code for analyzing file paths placed into DirectoryModel. Please, > consider moving it over there (as well as GDATA_SEARCH_ROOT_PATH and so on). Done.
http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:1011: shouldCreate; I recalled that canExecute_ only handles context menu commands. This code doesn't affect Ctrl-V. Please ensure that Ctrl-C in Downloads and Ctrl-V in search results doesn't cause copying or break something. Looks like this check must be in FileTransferManager.canPaste_. http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:1286: this.directoryModel_.getSearchOrCurrentDirEntry().fullPath; We gonna prevent paste and drop operation into search into search results. So in the search state this event may be safely ignored. this.directoryModel_.getCurrentDirPath() should be used. http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:2826: var dirPath = this.directoryModel_.getSearchOrCurrentDirEntry().fullPath; It should be getCurrentDirPath(). I think the gallery should not show /gdata/.search/... in the URL bar. It should show current directory path intitially and then canonical (resolved with getPathForDriveSearchResult) paths. http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:2846: readonlyDirName: By the way, it's unlikely renaming in the gallery works properly in the search results. Event worse it's unlikely it properly show file names. http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:3553: if (encodeURI(event.fileUrl) == this.getSearchOrCurrentDirectoryURL()) Are you sure event.fileUrl may ever point to "/gdata/.search"? Most likely it only points to canonical file paths. If so comparing URL with search entry doesn't makes sense. On the other hand any change in /gdata/... may affect search results. http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:4144: urls: [currentDirUrl + encodeURIComponent(filename)], It's gonna construct URL like filesystem://.../gdata/.search/new-file-name.ext. I don't think it makes sense. http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:4168: this.resolvePath(this.getCurrentDirectory() + '/' + filename, I don't think it makes sense to check file existence in current directory if search results is active. May be we should disable OK button on the "Save as" dialog when no file selected of file name in the input box is not equal to the selected file name. Or even better we may disable search in this dialog at all. Please, clarify it with the UX team. http://codereview.chromium.org/10342010/diff/15005/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/file_manager.js:4190: files.push(currentDirUrl + encodeURIComponent(entry.name)); May be just entry.toURL()? Is it OK to not resolve it with getPathForDriveSearchResult?
Regarding the gallery. Problem with gallery is that it shares some features with the file manager but doesn't share code. It need to be fixes. For now I feel we need to make sure galley doesn't display human unfriendly file names and doesn't not break on file renaming (the easiest way is making search results read only in the gallery).
PTAL https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1011: shouldCreate; On 2012/05/11 07:17:23, SeRya wrote: > I recalled that canExecute_ only handles context menu commands. This code > doesn't affect Ctrl-V. Please ensure that Ctrl-C in Downloads and Ctrl-V in > search results doesn't cause copying or break something. Looks like this check > must be in FileTransferManager.canPaste_. i have it in FilTransferManager.canPasteOrDrop_ https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1286: this.directoryModel_.getSearchOrCurrentDirEntry().fullPath; On 2012/05/11 07:17:23, SeRya wrote: > We gonna prevent paste and drop operation into search into search results. So in > the search state this event may be safely ignored. > this.directoryModel_.getCurrentDirPath() should be used. yeah, I agree https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2826: var dirPath = this.directoryModel_.getSearchOrCurrentDirEntry().fullPath; On 2012/05/11 07:17:23, SeRya wrote: > It should be getCurrentDirPath(). I think the gallery should not show > /gdata/.search/... in the URL bar. It should show current directory path > intitially and then canonical (resolved with getPathForDriveSearchResult) paths. Done. https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2846: readonlyDirName: On 2012/05/11 07:17:23, SeRya wrote: > By the way, it's unlikely renaming in the gallery works properly in the search > results. Event worse it's unlikely it properly show file names. > gallery is already readonly on gdata, but yes file names weren't displayed nicely. I fixed this, but to be hones I'm not too thrilled with the solution (seems kinda hacky; and I'm not sure it scales well). https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3553: if (encodeURI(event.fileUrl) == this.getSearchOrCurrentDirectoryURL()) On 2012/05/11 07:17:23, SeRya wrote: > Are you sure event.fileUrl may ever point to "/gdata/.search"? Most likely it > only points to canonical file paths. If so comparing URL with search entry > doesn't makes sense. On the other hand any change in /gdata/... may affect > search results. Done. https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4144: urls: [currentDirUrl + encodeURIComponent(filename)], On 2012/05/11 07:17:23, SeRya wrote: > It's gonna construct URL like filesystem://.../gdata/.search/new-file-name.ext. > I don't think it makes sense. we shouldn't get here in save-as dialog https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4168: this.resolvePath(this.getCurrentDirectory() + '/' + filename, On 2012/05/11 07:17:23, SeRya wrote: > I don't think it makes sense to check file existence in current directory if > search results is active. May be we should disable OK button on the "Save as" > dialog when no file selected of file name in the input box is not equal to the > selected file name. Or even better we may disable search in this dialog at all. > Please, clarify it with the UX team. grr..I had left you a comment here, but forgot to publish it last night :/ (basically, that save as dialog is TBD; same for the media player) I chatted with josh about this, and we decided that removing search box from save as dialog is probably the way to go. https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4190: files.push(currentDirUrl + encodeURIComponent(entry.name)); On 2012/05/11 07:17:23, SeRya wrote: > May be just entry.toURL()? Is it OK to not resolve it with > getPathForDriveSearchResult? I don't think it's "necessary", but it would probably be a good idea.. https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4190: files.push(currentDirUrl + encodeURIComponent(entry.name)); On 2012/05/11 07:17:23, SeRya wrote: > May be just entry.toURL()? Is it OK to not resolve it with > getPathForDriveSearchResult? Done.
LGTM with nots. https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/15005/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1286: this.directoryModel_.getSearchOrCurrentDirEntry().fullPath; So please replece with this.directoryModel_.getCurrentDirPath(). On 2012/05/14 20:44:17, tbarzic wrote: > On 2012/05/11 07:17:23, SeRya wrote: > > We gonna prevent paste and drop operation into search into search results. So > in > > the search state this event may be safely ignored. > > this.directoryModel_.getCurrentDirPath() should be used. > > yeah, I agree https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1011: shouldCreate; Please remove this change. queryPasteCommandEnabled already hanle this case and we should not duplicate the logic. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1286: this.directoryModel_.getSearchOrCurrentDirEntry().fullPath; Please use getCurrentDirPath() here. Add if (this.directoryModel_.isSearching()) return; since you anyway don't correctly update search results. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1876: this.directoryModel_.getSearchOrCurrentDirEntry().name == '' ? This is obsolete code. DirectoryModel doesn't fill the file list with mounted volumes when the current directory is root. We don't Please lave fileName.textContent = displayName; https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3101: this.directoryModel_.getSearchOrCurrentDirEntry().toURL(); It will fail if the current directory is unmounted GDATA. Please fix. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_transfer_controller.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_transfer_controller.js:486: return this.directoryModel_.getSearchOrCurrentDirEntry(); currentDirectory doesn't make sense for search results. It's mostly used to check if files gonna be pasted into the same directory thy have being coped from. We don't allow paste to search results. Ideally we should only set sourceDir if all the coped files are from the same directory. However it would require resolving each entry name before putting it into the collection. I suggest to not set sourceDir for search results or set it to empty string.
Unfortunately, still some problems here... https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/directory_model.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:567: * operations while renaming. It the given entry is not a gdata search result typo: it -> if https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:568: * entry, |newName| will be used. typo: newName -> displayName https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:576: DirectoryModel.prototype.getEntryName_ = function(entry, displayName) { Confusing name. I'd say that getEntryName == entry.name. Consider something like getEntryNameAfterRename. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:630: if (self.currentDirEntry_ != currentEntry) This should probably be: if (self.getSearchOrCurrentDirEntry() != currentEntry) https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:716: util.resolvePath(this.getSearchOrCurrentDirEntry(), entryName, What if new renamed name does not conform to search query? I guess, the renamed entry will not be found in search directory. Should we look into entry's real parent directory? https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1238: // Create shadow directory which will contein search results. typo: contein https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1265: this.removeFilter('searchbox'); We may not have this filter present. This will probably result in exception. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1340: path.search(DirectoryModel.GDATA_SEARCH_ROOT_PATH + '/') == 0; search -> indexOf ? https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1524: // We don't want search enabled in save as dialog. I don't really get why. I would like to search for 'birthday' directory and save an attached photo right there. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1876: this.directoryModel_.getSearchOrCurrentDirEntry().name == '' ? On 2012/05/15 06:17:01, SeRya wrote: > This is obsolete code. DirectoryModel doesn't fill the file list with mounted > volumes when the current directory is root. We don't Please lave > > fileName.textContent = displayName; +1 https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2805: * Does preprocessing of url list to open before calling |doO[enGallery_|. typo: doO[enGallery https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2871: var singleSelection = urls.length == 1; This is wrong now. If the case of single selection, we change urls to be the whole file list. You should pass original |singleSelection| here. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3337: (this.isOnGData() && this.directoryModel_.isSearching())) { Why can't I search for a file and overwrite it? https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3408: var changeToPath = path || entry.fullPath; Changing to entry.fullPath is not an option. Just don't change directory at all instead. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4179: var currentDirUrl = this.getSearchOrCurrentDirectoryURL(); You can't save a new file into the search directory. Where should it go? We should prohibit this. But if user just wants to overwrite a file, let her do that. See |resolveCallback| below. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/util.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/util.js:533: callback([]); return missed
https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/directory_model.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:567: * operations while renaming. It the given entry is not a gdata search result On 2012/05/15 11:25:06, dgozman wrote: > typo: it -> if Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:568: * entry, |newName| will be used. On 2012/05/15 11:25:06, dgozman wrote: > typo: newName -> displayName Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:576: DirectoryModel.prototype.getEntryName_ = function(entry, displayName) { On 2012/05/15 11:25:06, dgozman wrote: > Confusing name. I'd say that getEntryName == entry.name. > Consider something like getEntryNameAfterRename. Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:630: if (self.currentDirEntry_ != currentEntry) On 2012/05/15 11:25:06, dgozman wrote: > This should probably be: > if (self.getSearchOrCurrentDirEntry() != currentEntry) Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:716: util.resolvePath(this.getSearchOrCurrentDirEntry(), entryName, On 2012/05/15 11:25:06, dgozman wrote: > What if new renamed name does not conform to search query? I guess, the renamed > entry will not be found in search directory. > > Should we look into entry's real parent directory? this case is handled in gdata code (a bit hacky, but I hope to get rid of it soon) https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1238: // Create shadow directory which will contein search results. On 2012/05/15 11:25:06, dgozman wrote: > typo: contein Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1265: this.removeFilter('searchbox'); On 2012/05/15 11:25:06, dgozman wrote: > We may not have this filter present. This will probably result in exception. I haven't seen the exception when we are removing nonexistent filter, but added check for that in removeFilter() Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/directory_model.js:1340: path.search(DirectoryModel.GDATA_SEARCH_ROOT_PATH + '/') == 0; On 2012/05/15 11:25:06, dgozman wrote: > search -> indexOf ? Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1011: shouldCreate; On 2012/05/15 06:17:01, SeRya wrote: > Please remove this change. queryPasteCommandEnabled already hanle this case and > we should not duplicate the logic. Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1286: this.directoryModel_.getSearchOrCurrentDirEntry().fullPath; On 2012/05/15 06:17:01, SeRya wrote: > Please use getCurrentDirPath() here. > Add > > if (this.directoryModel_.isSearching()) > return; > > since you anyway don't correctly update search results. done. additionally, updating search results for non gdata dirs should work fine. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1524: // We don't want search enabled in save as dialog. On 2012/05/15 11:25:06, dgozman wrote: > I don't really get why. I would like to search for 'birthday' directory and save > an attached photo right there. yeah, I agree with you that would be useful :) I plan to revisit this really soon, but for now, I'd just leave it disabled (I synced with josh about this, and he's fine with that). https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1876: this.directoryModel_.getSearchOrCurrentDirEntry().name == '' ? On 2012/05/15 06:17:01, SeRya wrote: > This is obsolete code. DirectoryModel doesn't fill the file list with mounted > volumes when the current directory is root. We don't Please lave > > fileName.textContent = displayName; Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:1876: this.directoryModel_.getSearchOrCurrentDirEntry().name == '' ? On 2012/05/15 11:25:06, dgozman wrote: > On 2012/05/15 06:17:01, SeRya wrote: > > This is obsolete code. DirectoryModel doesn't fill the file list with mounted > > volumes when the current directory is root. We don't Please lave > > > > fileName.textContent = displayName; > > +1 Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2805: * Does preprocessing of url list to open before calling |doO[enGallery_|. On 2012/05/15 11:25:06, dgozman wrote: > typo: doO[enGallery Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:2871: var singleSelection = urls.length == 1; On 2012/05/15 11:25:06, dgozman wrote: > This is wrong now. If the case of single selection, we change urls to be the > whole file list. You should pass original |singleSelection| here. Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3101: this.directoryModel_.getSearchOrCurrentDirEntry().toURL(); On 2012/05/15 06:17:01, SeRya wrote: > It will fail if the current directory is unmounted GDATA. Please fix. it will have the same behaviour as getCurrentDirectoryURL (it will return ''). What is desired result here for unmounted gdata? url for path '/drive'? https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3337: (this.isOnGData() && this.directoryModel_.isSearching())) { On 2012/05/15 11:25:06, dgozman wrote: > Why can't I search for a file and overwrite it? Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:3408: var changeToPath = path || entry.fullPath; On 2012/05/15 11:25:06, dgozman wrote: > Changing to entry.fullPath is not an option. Just don't change directory at all > instead. Done. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_manager.js:4179: var currentDirUrl = this.getSearchOrCurrentDirectoryURL(); On 2012/05/15 11:25:06, dgozman wrote: > You can't save a new file into the search directory. Where should it go? > > We should prohibit this. But if user just wants to overwrite a file, let her do > that. See |resolveCallback| below. save is currently disabled for search. https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/file_transfer_controller.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/file_transfer_controller.js:486: return this.directoryModel_.getSearchOrCurrentDirEntry(); On 2012/05/15 06:17:01, SeRya wrote: > currentDirectory doesn't make sense for search results. It's mostly used to > check if files gonna be pasted into the same directory thy have being coped > from. We don't allow paste to search results. > > Ideally we should only set sourceDir if all the coped files are from the same > directory. However it would require resolving each entry name before putting it > into the collection. > > I suggest to not set sourceDir for search results or set it to empty string. setting sourceDir to empty string would break copy operations from search dirs (we have some checks that expect it to be set), so I'd rather leave this as it is, and maybe make the changes you proposed in a follow up patch (I'd hate to make this patch more complex unless I really need to) https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... File chrome/browser/resources/file_manager/js/util.js (right): https://chromiumcodereview.appspot.com/10342010/diff/31002/chrome/browser/res... chrome/browser/resources/file_manager/js/util.js:533: callback([]); On 2012/05/15 11:25:06, dgozman wrote: > return missed Done.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
