Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22167002:
Cleanup: Remove unnecessary property: VolumeList.currentVolume_ (Closed)
|
Created:
7 years, 4 months ago by yoshiki Modified:
7 years, 4 months ago Reviewers:
mtomasz CC:
chromium-reviews, rginda+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCleanup: Remove unnecessary property: VolumeList.currentVolume_
This property can be removed. This patch also fixes a small bug on changing the selection on the list.
BUG=none
TEST=manual
R=mtomasz@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215817
Patch Set 1 #
Total comments: 5
Patch Set 2 : addressed comment #Patch Set 3 : addressed comment #
Total comments: 2
Messages
Total messages: 10 (0 generated)
@mtomasz: PTAL. Thanks.
https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:315: if (!newPath || this.directoryModel.getCurrentDirEntry().fullPath == newPath) directoryModel -> directoryModel_ https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:315: if (!newPath || this.directoryModel.getCurrentDirEntry().fullPath == newPath) I think this will not work as it should. getCurrentDirEntry().fullPath will return the current path, not the current root.
@mtomasz: PTAL https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:315: if (!newPath || this.directoryModel.getCurrentDirEntry().fullPath == newPath) On 2013/08/05 12:17:57, mtomasz wrote: > directoryModel -> directoryModel_ Done. https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:315: if (!newPath || this.directoryModel.getCurrentDirEntry().fullPath == newPath) On 2013/08/05 12:17:57, mtomasz wrote: > I think this will not work as it should. getCurrentDirEntry().fullPath will > return the current path, not the current root. It should work. Previous code was wrong. Before introducing folder shortcut feature, each root had only an item. If the selection is changed, the roots of old and new items must be different. But now, there may be two or more items with same root, so comparing root is not enough. We need to compare the entire path.
https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/volume_list.js:315: if (!newPath || this.directoryModel.getCurrentDirEntry().fullPath == newPath) On 2013/08/05 16:08:01, yoshiki wrote: > On 2013/08/05 12:17:57, mtomasz wrote: > > I think this will not work as it should. getCurrentDirEntry().fullPath will > > return the current path, not the current root. > > It should work. Previous code was wrong. > > Before introducing folder shortcut feature, each root had only an item. If the > selection is changed, the roots of old and new items must be different. But now, > there may be two or more items with same root, so comparing root is not enough. > We need to compare the entire path. I see. My concern is that this check was used to avoid reselecting the current item I guess. Now, it will sometimes allow to reselect and sometimes not. Eg. when we are in a subdirectory of Drive and then try to change to Drive. This check will return false and will allow to reselect. So, I have two questions: 1. Is this check necessary? If not, then can we remove it? 2. Why not checking the selection model if the current selected index == index? If we can't (1) nor (2) then please just add a comment describing the purpose of this check.
On 2013/08/05 16:25:51, mtomasz wrote: > https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... > File chrome/browser/resources/file_manager/js/volume_list.js (right): > > https://codereview.chromium.org/22167002/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/volume_list.js:315: if (!newPath || > this.directoryModel.getCurrentDirEntry().fullPath == newPath) > On 2013/08/05 16:08:01, yoshiki wrote: > > On 2013/08/05 12:17:57, mtomasz wrote: > > > I think this will not work as it should. getCurrentDirEntry().fullPath will > > > return the current path, not the current root. > > > > It should work. Previous code was wrong. > > > > Before introducing folder shortcut feature, each root had only an item. If the > > selection is changed, the roots of old and new items must be different. But > now, > > there may be two or more items with same root, so comparing root is not > enough. > > We need to compare the entire path. > > I see. My concern is that this check was used to avoid reselecting the current > item I guess. Now, it will sometimes allow to reselect and sometimes not. Eg. > when we are in a subdirectory of Drive and then try to change to Drive. This > check will return false and will allow to reselect. > > So, I have two questions: > 1. Is this check necessary? If not, then can we remove it? This prevents the double-moving to the current directory (I added a comment). Even if we remove it, it would work but consume some extra CPU usage. > 2. Why not checking the selection model if the current selected index == index? In current code, this code should be called from onSelectionChanged, and the formula is always true since the given index is the selected index. > If we can't (1) nor (2) then please just add a comment describing the purpose of > this check.
https://codereview.chromium.org/22167002/diff/9001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22167002/diff/9001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/volume_list.js:318: // Prevents double-moving to the current directory. The problem here is that it won't work when being in a subdirectory. This check will fail when clicking on 'Drive' and having opened some subdirectory on Drive. It should stop from reselecting, but it won't. That's why I was thinking if it is better to remove this check, since it doesn't work in many cases and it is misleading. Also, is it possible to get a selectByIndex call on the item which is already selected?
https://codereview.chromium.org/22167002/diff/9001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/js/volume_list.js (right): https://codereview.chromium.org/22167002/diff/9001/chrome/browser/resources/f... chrome/browser/resources/file_manager/js/volume_list.js:318: // Prevents double-moving to the current directory. On 2013/08/06 03:46:20, mtomasz wrote: > The problem here is that it won't work when being in a subdirectory. This check > will fail when clicking on 'Drive' and having opened some subdirectory on Drive. > It should stop from reselecting, but it won't. > > That's why I was thinking if it is better to remove this check, since it doesn't > work in many cases and it is misleading. Also, is it possible to get a > selectByIndex call on the item which is already selected? Sorry, I misunderstood this method. Please ignore this comment.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/22167002/9001
Message was sent while issue was closed.
Committed patchset #3 manually as r215817 (presubmit successful). |
