Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838023002:
Only do Smart Select if one word is selected. (Closed)
DescriptionOnly do Smart Select if one word is selected.
There is a bug where after "Select All" Smart Select will suggest a different
(smaller) selection. This CL checks if the selection only has one word. Since
longpress/double tap only selects one word, if we have multiple words we can assume
it comes from "Select All" and not do Smart Select.
BUG=714106
Review-Url: https://codereview.chromium.org/2838023002
Cr-Commit-Position: refs/heads/master@{#466856}
Committed: https://chromium.googlesource.com/chromium/src/+/03f5058e764e8b2b01d571f29426075a83f6049b
Patch Set 1 #Patch Set 2 : added todo #
Total comments: 4
Patch Set 3 : aelias comments #
Total comments: 1
Patch Set 4 : timav nit #Messages
Total messages: 20 (12 generated)
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. BUG=714106 ========== to ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. This CL checks if the selected text to see if the selection has multiple words. Since longpress/double tap only selects one word, if we have multiple words we can assume it comes from "Select All" and not do Smart Select. BUG=714106 ==========
Description was changed from ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. This CL checks if the selected text to see if the selection has multiple words. Since longpress/double tap only selects one word, if we have multiple words we can assume it comes from "Select All" and not do Smart Select. BUG=714106 ========== to ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. This CL checks if the selected text to see if the selection only has one word. Since longpress/double tap only selects one word, if we have multiple words we can assume it comes from "Select All" and not do Smart Select. BUG=714106 ==========
Description was changed from ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. This CL checks if the selected text to see if the selection only has one word. Since longpress/double tap only selects one word, if we have multiple words we can assume it comes from "Select All" and not do Smart Select. BUG=714106 ========== to ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. This CL checks if the selection only has one word. Since longpress/double tap only selects one word, if we have multiple words we can assume it comes from "Select All" and not do Smart Select. BUG=714106 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org, timav@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2838023002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2838023002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:847: // TODO(amaralp): Find a better way to know that SELECTION_HANDLES_SHOWN wasn't Please link the bug number in this comment: "http://crbug.com/714106" https://codereview.chromium.org/2838023002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:850: !getSelectedText().isEmpty() && !getSelectedText().contains(" "); " " is a bit too narrow of a condition, I'd like to make sure \n and nbsp are also included. For about 's.matches(".*\\s+.*")' from http://stackoverflow.com/questions/4067809/how-can-i-find-whitespace-space-in...
https://codereview.chromium.org/2838023002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2838023002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:847: // TODO(amaralp): Find a better way to know that SELECTION_HANDLES_SHOWN wasn't On 2017/04/24 at 23:59:19, aelias wrote: > Please link the bug number in this comment: "http://crbug.com/714106" Done. https://codereview.chromium.org/2838023002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:850: !getSelectedText().isEmpty() && !getSelectedText().contains(" "); On 2017/04/24 at 23:59:19, aelias wrote: > " " is a bit too narrow of a condition, I'd like to make sure \n and nbsp are also included. For about 's.matches(".*\\s+.*")' from http://stackoverflow.com/questions/4067809/how-can-i-find-whitespace-space-in... Done.
lgtm
lgtm % nit https://codereview.chromium.org/2838023002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2838023002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:847: // TODO(amaralp): Find a better way to know that SELECTION_HANDLES_SHOWN wasn't nit: Could you, please, add some explanation why we are doing this in addition to the link to the bug. Maybe something like "When this event comes as the result of SelectAll, we should not classify for a better selection range (http://...). Assume that two or more selected words means SelectAll. TODO: Find a better way to know..." I'm sure you can formulate it better.
The CQ bit was checked by amaralp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, timav@chromium.org Link to the patchset: https://codereview.chromium.org/2838023002/#ps60001 (title: "timav nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493081090368510,
"parent_rev": "4829139c6fd22953b4ae9b0cb3bc686362af03cf", "commit_rev":
"03f5058e764e8b2b01d571f29426075a83f6049b"}
Message was sent while issue was closed.
Description was changed from ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. This CL checks if the selection only has one word. Since longpress/double tap only selects one word, if we have multiple words we can assume it comes from "Select All" and not do Smart Select. BUG=714106 ========== to ========== Only do Smart Select if one word is selected. There is a bug where after "Select All" Smart Select will suggest a different (smaller) selection. This CL checks if the selection only has one word. Since longpress/double tap only selects one word, if we have multiple words we can assume it comes from "Select All" and not do Smart Select. BUG=714106 Review-Url: https://codereview.chromium.org/2838023002 Cr-Commit-Position: refs/heads/master@{#466856} Committed: https://chromium.googlesource.com/chromium/src/+/03f5058e764e8b2b01d571f29426... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/03f5058e764e8b2b01d571f29426...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2848793002/ by aelias@chromium.org. The reason for reverting is: Null pointer crash http://crbug.com/715526 BUG=715526. |
