Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
279873002:
Files.app: Handle change-by-keyboard in the directory tree (Closed)
|
Created:
6 years, 7 months ago by yoshiki Modified:
6 years, 7 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFiles.app: Handle change-by-keyboard in the directory tree
This patch fixes the regression, which is that the selection change by keyboard is not handled.
BUG=371715
TEST=manually tested
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271346
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed the comments #
Total comments: 5
Patch Set 3 : Addressed the comment #Patch Set 4 : Add a comment #Patch Set 5 : rebase #
Messages
Total messages: 21 (0 generated)
@hirono, PTAL.
https://codereview.chromium.org/279873002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/directory_tree.js (right): https://codereview.chromium.org/279873002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/directory_tree.js:263: DirectoryItem.prototype.changeCurrentDirectoryToThis_ = function() { Could you add JSDoc? https://codereview.chromium.org/279873002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/directory_tree.js:263: DirectoryItem.prototype.changeCurrentDirectoryToThis_ = function() { I think this method should be a member of DirectoryTree. It accesses the entire directory model. https://codereview.chromium.org/279873002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/directory_tree.js:500: this.selectedItem.changeCurrentDirectoryToThis_(); Indent is 2.
@hirono, PTAL. https://codereview.chromium.org/279873002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/directory_tree.js (right): https://codereview.chromium.org/279873002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/directory_tree.js:263: DirectoryItem.prototype.changeCurrentDirectoryToThis_ = function() { On 2014/05/12 03:17:47, hirono wrote: > I think this method should be a member of DirectoryTree. > It accesses the entire directory model. Done. https://codereview.chromium.org/279873002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/directory_tree.js:500: this.selectedItem.changeCurrentDirectoryToThis_(); On 2014/05/12 03:17:47, hirono wrote: > Indent is 2. Done.
https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:689: * Activate the given directry. nit: Activates https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:700: DirectoryModel.prototype.activateDirectoryEntry = function( I like the method implemented in DirectoryModel. And I think the breadcrumb list and the navigation list also should use this. Or we can change the default behavior of changeDirectoryEntry method and pass a flag if we would not like to clear the selection. It ensures a programmer to select whether the selection should clears or not. If there are two methods, the programmer who found one may miss another.
https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:689: * Activate the given directry. On 2014/05/14 03:20:03, hirono wrote: > nit: Activates Done. https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:700: DirectoryModel.prototype.activateDirectoryEntry = function( On 2014/05/14 03:20:03, hirono wrote: > I like the method implemented in DirectoryModel. And I think the breadcrumb list > and the navigation list also should use this. > Or we can change the default behavior of changeDirectoryEntry method and pass a > flag if we would not like to clear the selection. It ensures a programmer to > select whether the selection should clears or not. If there are two methods, the > programmer who found one may miss another. I think it's better to keep both low level method (changeDirectoryEntry) and the high level method (activateDirectoryEntry). And I agree that we should use activateDirectoryEntry from the breadcramb and nav list in a separate patch.
https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:700: DirectoryModel.prototype.activateDirectoryEntry = function( On 2014/05/14 05:46:05, yoshiki wrote: > On 2014/05/14 03:20:03, hirono wrote: > > I like the method implemented in DirectoryModel. And I think the breadcrumb > list > > and the navigation list also should use this. > > Or we can change the default behavior of changeDirectoryEntry method and pass > a > > flag if we would not like to clear the selection. It ensures a programmer to > > select whether the selection should clears or not. If there are two methods, > the > > programmer who found one may miss another. > > I think it's better to keep both low level method (changeDirectoryEntry) and the > high level method (activateDirectoryEntry). And I agree that we should use > activateDirectoryEntry from the breadcramb and nav list in a separate patch. Maybe adding see also comments makes the relationship of two methods clear. But I have not been convinced yet a little. Mixing different level methods into one class does not sound a good thing. WDYT?
On 2014/05/14 05:59:30, hirono wrote: > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > File ui/file_manager/file_manager/foreground/js/directory_model.js (right): > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > ui/file_manager/file_manager/foreground/js/directory_model.js:700: > DirectoryModel.prototype.activateDirectoryEntry = function( > On 2014/05/14 05:46:05, yoshiki wrote: > > On 2014/05/14 03:20:03, hirono wrote: > > > I like the method implemented in DirectoryModel. And I think the breadcrumb > > list > > > and the navigation list also should use this. > > > Or we can change the default behavior of changeDirectoryEntry method and > pass > > a > > > flag if we would not like to clear the selection. It ensures a programmer to > > > select whether the selection should clears or not. If there are two methods, > > the > > > programmer who found one may miss another. > > > > I think it's better to keep both low level method (changeDirectoryEntry) and > the > > high level method (activateDirectoryEntry). And I agree that we should use > > activateDirectoryEntry from the breadcramb and nav list in a separate patch. > > Maybe adding see also comments makes the relationship of two methods clear. But > I have not been convinced yet a little. Mixing different level methods into one > class does not sound a good thing. WDYT? I don't think it's not good think. The current comment and code show the exact behavior. In my opinion, a comment of difference of two method is redundant, and it may cause inconsistency in near feature.
On 2014/05/16 06:21:16, yoshiki wrote: > On 2014/05/14 05:59:30, hirono wrote: > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > File ui/file_manager/file_manager/foreground/js/directory_model.js (right): > > > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > ui/file_manager/file_manager/foreground/js/directory_model.js:700: > > DirectoryModel.prototype.activateDirectoryEntry = function( > > On 2014/05/14 05:46:05, yoshiki wrote: > > > On 2014/05/14 03:20:03, hirono wrote: > > > > I like the method implemented in DirectoryModel. And I think the > breadcrumb > > > list > > > > and the navigation list also should use this. > > > > Or we can change the default behavior of changeDirectoryEntry method and > > pass > > > a > > > > flag if we would not like to clear the selection. It ensures a programmer > to > > > > select whether the selection should clears or not. If there are two > methods, > > > the > > > > programmer who found one may miss another. > > > > > > I think it's better to keep both low level method (changeDirectoryEntry) and > > the > > > high level method (activateDirectoryEntry). And I agree that we should use > > > activateDirectoryEntry from the breadcramb and nav list in a separate patch. > > > > Maybe adding see also comments makes the relationship of two methods clear. > But > > I have not been convinced yet a little. Mixing different level methods into > one > > class does not sound a good thing. WDYT? > > I don't think it's not good think. The current comment and code show the exact > behavior. In my opinion, a comment of difference of two method is redundant, and > it may cause inconsistency in near feature. SGTM. Only one concern I worried about is a coder who just found changeDirectoryEntry wrongly uses the method instead of activateEntry. It may cause an inconsistency of the behavior (ex. some UI clears the selection and others do not) which is hard to find. Please handle the problem somehow.
On 2014/05/16 06:46:14, hirono wrote: > On 2014/05/16 06:21:16, yoshiki wrote: > > On 2014/05/14 05:59:30, hirono wrote: > > > > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > > File ui/file_manager/file_manager/foreground/js/directory_model.js (right): > > > > > > > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > > ui/file_manager/file_manager/foreground/js/directory_model.js:700: > > > DirectoryModel.prototype.activateDirectoryEntry = function( > > > On 2014/05/14 05:46:05, yoshiki wrote: > > > > On 2014/05/14 03:20:03, hirono wrote: > > > > > I like the method implemented in DirectoryModel. And I think the > > breadcrumb > > > > list > > > > > and the navigation list also should use this. > > > > > Or we can change the default behavior of changeDirectoryEntry method and > > > pass > > > > a > > > > > flag if we would not like to clear the selection. It ensures a > programmer > > to > > > > > select whether the selection should clears or not. If there are two > > methods, > > > > the > > > > > programmer who found one may miss another. > > > > > > > > I think it's better to keep both low level method (changeDirectoryEntry) > and > > > the > > > > high level method (activateDirectoryEntry). And I agree that we should use > > > > activateDirectoryEntry from the breadcramb and nav list in a separate > patch. > > > > > > Maybe adding see also comments makes the relationship of two methods clear. > > But > > > I have not been convinced yet a little. Mixing different level methods into > > one > > > class does not sound a good thing. WDYT? > > > > I don't think it's not good think. The current comment and code show the exact > > behavior. In my opinion, a comment of difference of two method is redundant, > and > > it may cause inconsistency in near feature. > > SGTM. Only one concern I worried about is a coder who just found > changeDirectoryEntry wrongly uses the method instead of activateEntry. It may > cause an inconsistency of the behavior (ex. some UI clears the selection and > others do not) which is hard to find. Please handle the problem somehow. 'changeDirectoryEntry' just changes the current directory entry, and 'activateDirectoryEntry' activates the given entry. I think there is a little possibility to confuse and the confusing itself is not a serious problem. And I don't think this is hard to find as you said. We can easily see inconsistency once we check the behavior, and it never causes the unexpected side-effect (let me know if I missed any side-effect). If we truly prevent this possibility. we need to check the caller and cause an error if the caller is insufficient. But I think it's so over-kill.
On 2014/05/16 08:06:53, yoshiki wrote: > On 2014/05/16 06:46:14, hirono wrote: > > On 2014/05/16 06:21:16, yoshiki wrote: > > > On 2014/05/14 05:59:30, hirono wrote: > > > > > > > > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > > > File ui/file_manager/file_manager/foreground/js/directory_model.js > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > > > ui/file_manager/file_manager/foreground/js/directory_model.js:700: > > > > DirectoryModel.prototype.activateDirectoryEntry = function( > > > > On 2014/05/14 05:46:05, yoshiki wrote: > > > > > On 2014/05/14 03:20:03, hirono wrote: > > > > > > I like the method implemented in DirectoryModel. And I think the > > > breadcrumb > > > > > list > > > > > > and the navigation list also should use this. > > > > > > Or we can change the default behavior of changeDirectoryEntry method > and > > > > pass > > > > > a > > > > > > flag if we would not like to clear the selection. It ensures a > > programmer > > > to > > > > > > select whether the selection should clears or not. If there are two > > > methods, > > > > > the > > > > > > programmer who found one may miss another. > > > > > > > > > > I think it's better to keep both low level method (changeDirectoryEntry) > > and > > > > the > > > > > high level method (activateDirectoryEntry). And I agree that we should > use > > > > > activateDirectoryEntry from the breadcramb and nav list in a separate > > patch. > > > > > > > > Maybe adding see also comments makes the relationship of two methods > clear. > > > But > > > > I have not been convinced yet a little. Mixing different level methods > into > > > one > > > > class does not sound a good thing. WDYT? > > > > > > I don't think it's not good think. The current comment and code show the > exact > > > behavior. In my opinion, a comment of difference of two method is redundant, > > and > > > it may cause inconsistency in near feature. > > > > SGTM. Only one concern I worried about is a coder who just found > > changeDirectoryEntry wrongly uses the method instead of activateEntry. It may > > cause an inconsistency of the behavior (ex. some UI clears the selection and > > others do not) which is hard to find. Please handle the problem somehow. > > 'changeDirectoryEntry' just changes the current directory entry, and > 'activateDirectoryEntry' activates the given entry. I think there is a little > possibility to confuse and the confusing itself is not a serious problem. And I > don't think this is hard to find as you said. We can easily see inconsistency > once we check the behavior, and it never causes the unexpected side-effect (let > me know if I missed any side-effect). > > If we truly prevent this possibility. we need to check the caller and cause an > error if the caller is insufficient. But I think it's so over-kill. If you still have a concern about it, how about adding a guidance to JSdoc which lets the developer consider to use 'activateDirectoryEntry' instead of 'changeDirectoryEntry'. It will prevent a developer from confusing it.
On 2014/05/16 08:15:57, yoshiki wrote: > On 2014/05/16 08:06:53, yoshiki wrote: > > On 2014/05/16 06:46:14, hirono wrote: > > > On 2014/05/16 06:21:16, yoshiki wrote: > > > > On 2014/05/14 05:59:30, hirono wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > > > > File ui/file_manager/file_manager/foreground/js/directory_model.js > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/279873002/diff/20001/ui/file_manager/file_man... > > > > > ui/file_manager/file_manager/foreground/js/directory_model.js:700: > > > > > DirectoryModel.prototype.activateDirectoryEntry = function( > > > > > On 2014/05/14 05:46:05, yoshiki wrote: > > > > > > On 2014/05/14 03:20:03, hirono wrote: > > > > > > > I like the method implemented in DirectoryModel. And I think the > > > > breadcrumb > > > > > > list > > > > > > > and the navigation list also should use this. > > > > > > > Or we can change the default behavior of changeDirectoryEntry method > > and > > > > > pass > > > > > > a > > > > > > > flag if we would not like to clear the selection. It ensures a > > > programmer > > > > to > > > > > > > select whether the selection should clears or not. If there are two > > > > methods, > > > > > > the > > > > > > > programmer who found one may miss another. > > > > > > > > > > > > I think it's better to keep both low level method > (changeDirectoryEntry) > > > and > > > > > the > > > > > > high level method (activateDirectoryEntry). And I agree that we should > > use > > > > > > activateDirectoryEntry from the breadcramb and nav list in a separate > > > patch. > > > > > > > > > > Maybe adding see also comments makes the relationship of two methods > > clear. > > > > But > > > > > I have not been convinced yet a little. Mixing different level methods > > into > > > > one > > > > > class does not sound a good thing. WDYT? > > > > > > > > I don't think it's not good think. The current comment and code show the > > exact > > > > behavior. In my opinion, a comment of difference of two method is > redundant, > > > and > > > > it may cause inconsistency in near feature. > > > > > > SGTM. Only one concern I worried about is a coder who just found > > > changeDirectoryEntry wrongly uses the method instead of activateEntry. It > may > > > cause an inconsistency of the behavior (ex. some UI clears the selection and > > > others do not) which is hard to find. Please handle the problem somehow. > > > > 'changeDirectoryEntry' just changes the current directory entry, and > > 'activateDirectoryEntry' activates the given entry. I think there is a little > > possibility to confuse and the confusing itself is not a serious problem. And > I > > don't think this is hard to find as you said. We can easily see inconsistency > > once we check the behavior, and it never causes the unexpected side-effect > (let > > me know if I missed any side-effect). > > > > If we truly prevent this possibility. we need to check the caller and cause an > > error if the caller is insufficient. But I think it's so over-kill. > > If you still have a concern about it, how about adding a guidance to JSdoc which > lets the developer consider to use 'activateDirectoryEntry' instead of > 'changeDirectoryEntry'. It will prevent a developer from confusing it. I like the idea. It is enough to avoid the misusage. Thank you!
@hirono, I added the comment. PATL.
On 2014/05/16 08:29:31, yoshiki wrote: > @hirono, I added the comment. PATL. LGTM! Thank you for handling it.
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/279873002/50001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/279873002/70001
Message was sent while issue was closed.
Change committed as 271346 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
