Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2229733002:
Fix index changed when no image in the directory in gallery (Closed)
|
Created:
4 years, 4 months ago by harukam Modified:
4 years, 4 months ago Reviewers:
fukino CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix index changed when no image in the directory in gallery
BUG=607852
TEST=manually tested
Committed: https://crrev.com/0197e54830c124fa205bb206959cb284c81ad39f
Cr-Commit-Position: refs/heads/master@{#410986}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix index changed when no image in the directory in gallery #
Total comments: 2
Patch Set 3 : Fix index changed when no image in the directory in gallery #Messages
Total messages: 19 (10 generated)
harukam@chromium.org changed reviewers: + fukino@chromium.org, harukam@chromium.org
PTAL https://codereview.chromium.org/2229733002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/2229733002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/slide_mode.js:922: return -1; "slideshowButton" icon shows because selectedIndex can be change by hitting right/left button, and "selectionModel_.selectedIndexes.length" become 1. (see ui/file_manager/gallery/js/gallery.js, line 820-822) I'm not exactly sure where to add this two lines. Do you think it's unnatural to return -1 in get"Next"SelectedIndex_?
https://codereview.chromium.org/2229733002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/2229733002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/slide_mode.js:922: return -1; On 2016/08/09 07:18:57, harukam1 wrote: > "slideshowButton" icon shows because selectedIndex can be change by hitting > right/left button, and "selectionModel_.selectedIndexes.length" become 1. (see > ui/file_manager/gallery/js/gallery.js, line 820-822) > > I'm not exactly sure where to add this two lines. > Do you think it's unnatural to return -1 in get"Next"SelectedIndex_? It's weird that selectionModel_.selectedIndexes.length returns 1 when there is no item. Could you check the code in Gallery where we remove the images? I guess this issue can be fixed by clearing the selection model when we remove all items.
Description was changed from ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested ========== to ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested ==========
harukam@chromium.org changed reviewers: - harukam@chromium.org
harukam@chromium.org changed reviewers: + harukam@chromium.org
PTAL https://codereview.chromium.org/2229733002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/2229733002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/slide_mode.js:922: return -1; On 2016/08/09 07:49:18, fukino wrote: > On 2016/08/09 07:18:57, harukam1 wrote: > > "slideshowButton" icon shows because selectedIndex can be change by hitting > > right/left button, and "selectionModel_.selectedIndexes.length" become 1. (see > > ui/file_manager/gallery/js/gallery.js, line 820-822) > > > > I'm not exactly sure where to add this two lines. > > Do you think it's unnatural to return -1 in get"Next"SelectedIndex_? > > It's weird that selectionModel_.selectedIndexes.length returns 1 when there is > no item. > Could you check the code in Gallery where we remove the images? > I guess this issue can be fixed by clearing the selection model when we remove > all items. We have decided to modify advanceWithKeyboard method per offline discussion.
https://codereview.chromium.org/2229733002/diff/20001/ui/file_manager/gallery... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/2229733002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:930: if (this.selectionModel_.selectedIndex === -1) What we want to do is "If there is no item, do not change the selectedIndex." So, could you check the item's count instead of selectedIndex? It is possible that selectedIndex is -1 but there are some items.
PTAL https://codereview.chromium.org/2229733002/diff/20001/ui/file_manager/gallery... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/2229733002/diff/20001/ui/file_manager/gallery... ui/file_manager/gallery/js/slide_mode.js:930: if (this.selectionModel_.selectedIndex === -1) On 2016/08/10 04:39:46, fukino wrote: > What we want to do is "If there is no item, do not change the selectedIndex." > So, could you check the item's count instead of selectedIndex? > > It is possible that selectedIndex is -1 but there are some items. Acknowledged.
fukino@chromium.org changed reviewers: - harukam@chromium.org
lgtm
The CQ bit was checked by harukam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested ========== to ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested ========== to ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested Committed: https://crrev.com/0197e54830c124fa205bb206959cb284c81ad39f Cr-Commit-Position: refs/heads/master@{#410986} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0197e54830c124fa205bb206959cb284c81ad39f Cr-Commit-Position: refs/heads/master@{#410986}
Message was sent while issue was closed.
Description was changed from ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested Committed: https://crrev.com/0197e54830c124fa205bb206959cb284c81ad39f Cr-Commit-Position: refs/heads/master@{#410986} ========== to ========== Fix index changed when no image in the directory in gallery BUG=607852 TEST=manually tested Committed: https://crrev.com/0197e54830c124fa205bb206959cb284c81ad39f Cr-Commit-Position: refs/heads/master@{#410986} ==========
Message was sent while issue was closed.
fukino@chromium.org changed reviewers: - harukam@chromium.org |
