Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907253002:
MacViews: Implement move and move*AndModifySelection commands. (Closed)
|
Created:
4 years, 8 months ago by karandeepb Modified:
4 years, 7 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Implement move and move*AndModifySelection commands.
This CL implements various move and move*AndModifySelection commands. Tests are
also added to test expectations against a dummy NSTextView.
BUG=586985
Committed: https://crrev.com/8cd49050ee5526cc61b92d189f37882377b4f617
Cr-Commit-Position: refs/heads/master@{#394632}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Add keycodes #Patch Set 3 : Split tests #
Total comments: 1
Patch Set 4 : Cleanup. #
Total comments: 23
Patch Set 5 : Address review comments. #
Total comments: 4
Patch Set 6 : Address review comments. #
Messages
Total messages: 23 (12 generated)
Description was changed from ========== Try move things. ========== to ========== MacViews: Implement move and move*AndModifySelection commands. This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 ==========
Description was changed from ========== MacViews: Implement move and move*AndModifySelection commands. This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 ========== to ========== MacViews: Implement move and move*AndModifySelection commands. This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 ==========
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. I have kept the keycodes for commands as per https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... I don't really understand the purpose these keycodes serve, so let me know if any of them should be changed. The behavior of most of the move*AndModifySelection commands needs to be fixed, for the corner case where selection direction changes. Will assign a bug and send a fix for it subsequently. https://codereview.chromium.org/1907253002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1907253002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:653: IsTextRTL(textInputClient_) ? [self moveLeft:sender] I think that I should add a GetTextDirection() public method to TextInputClient in a follow-up, since Textfield caches the directionality and hence this won't need to be recomputed. Also TextInputClient already allows setting directionality via ChangeTextDirectionAndLayoutAlignment. Thoughts? https://codereview.chromium.org/1907253002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:677: - (void)moveUp:(id)sender { Should I use the command IDS_MOVE_UP and only enable it for MacViews or is this ok? https://codereview.chromium.org/1907253002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1907253002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:942: // Test move commands against expectations set by |dummy_text_view_|. Have split the tests since these tests are slow. For example this test is taking 5 sec on my computer. Is that ok or should I try to reduce the time?
https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:639: // to Line. perhaps expand on this with a note that Up/Down also map to beginning/end of line commands. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:652: - (void)moveForward:(nullable id)sender { were the nullable annotations necessary? We haven't really started using them in chrome yet. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:244: // Make selection from |start| to |end| on installed textfield and textfield -> views::Textfield https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:262: // Test editing commands in array |selectors| against the expectations set by needs to say |selectors| is an NSArray of NSStrings. (you could also declare a array literal of SEL (~ function pointers), but they're harder to iterate over). https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:353: // NSTextView does not support specifying the selection "direction" i.e. the It does :) Perhaps try [dummy_text_view_ setSelectedRange:range affinity:(NSSelectionAffinityUpstream <or> NSSelectionAffinityDownstream) stillSelecting:NO]; https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:452: void BridgedNativeWidgetTest::TestEditingCommands(NSArray* selectors) { nit: comment https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:454: const std::wstring test_string; string16? wstring doesn't get used much. You'll probably need to have base::WideToUTF16 in the aggregate initializer list https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:456: } cases[] = {{L"abc def", false}, Can we get equivalent coverage by making these shorter? I see we would want a space, but "ab cd" should be sufficient. perhaps even "ab c"? https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:459: for (auto test_case : cases) { nit: const auto&
PTAL Trent. Thanks. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:639: // to Line. On 2016/05/16 05:34:40, tapted wrote: > perhaps expand on this with a note that Up/Down also map to beginning/end of > line commands. Done. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:652: - (void)moveForward:(nullable id)sender { On 2016/05/16 05:34:40, tapted wrote: > were the nullable annotations necessary? We haven't really started using them in > chrome yet. Removed. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:244: // Make selection from |start| to |end| on installed textfield and On 2016/05/16 05:34:41, tapted wrote: > textfield -> views::Textfield Done. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:262: // Test editing commands in array |selectors| against the expectations set by On 2016/05/16 05:34:40, tapted wrote: > needs to say |selectors| is an NSArray of NSStrings. > > (you could also declare a array literal of SEL (~ function pointers), but > they're harder to iterate over). Done. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:353: // NSTextView does not support specifying the selection "direction" i.e. the On 2016/05/16 05:34:41, tapted wrote: > It does :) > > Perhaps try [dummy_text_view_ setSelectedRange:range > affinity:(NSSelectionAffinityUpstream <or> NSSelectionAffinityDownstream) > stillSelecting:NO]; Can't get it to work. For example consider this test- TEST_F(BridgedNativeWidgetTest, XYZ) { NSTextView* dummy_text_view_ = [[NSTextView alloc] init]; [dummy_text_view_ setString: @"12345"]; // Select "23" from left to right. [dummy_text_view_ setSelectedRange: NSMakeRange(1,2) affinity:NSSelectionAffinityDownstream stillSelecting:NO]; [dummy_text_view_ doCommandBySelector:@selector(moveWordRightAndModifySelection:)]; // State should be "1|2345|". EXPECT_EQ_RANGE(NSMakeRange(1, 4) ,[dummy_text_view_ selectedRange]); // Select "23" from right to left. [dummy_text_view_ setSelectedRange: NSMakeRange(1,2) affinity:NSSelectionAffinityUpstream stillSelecting:NO]; [dummy_text_view_ doCommandBySelector:@selector(moveWordRightAndModifySelection:)]; // State should be "123|45". EXPECT_EQ_RANGE(NSMakeRange(3, 0) ,[dummy_text_view_ selectedRange]); } The last test fails, since "1|2345|" is selected. However, it passes with the current MakeSelection implementation. Also see http://stackoverflow.com/questions/20116963/how-can-i-get-the-selection-direc... https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:452: void BridgedNativeWidgetTest::TestEditingCommands(NSArray* selectors) { On 2016/05/16 05:34:40, tapted wrote: > nit: comment Doesn't the comment in the class declaration suffice? It seems we don't generally comment on function definitions. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:454: const std::wstring test_string; On 2016/05/16 05:34:41, tapted wrote: > string16? wstring doesn't get used much. You'll probably need to have > base::WideToUTF16 in the aggregate initializer list Done. Also a question. From what I saw, we use wstring for hardcoding non-ascii strings generally. Is it the case that we don't use plain std::strings for these, since the encoding will be implementation defined? https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:456: } cases[] = {{L"abc def", false}, On 2016/05/16 05:34:41, tapted wrote: > Can we get equivalent coverage by making these shorter? I see we would want a > space, but "ab cd" should be sufficient. perhaps even "ab c"? Done. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:459: for (auto test_case : cases) { On 2016/05/16 05:34:40, tapted wrote: > nit: const auto& Done.
https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:353: // NSTextView does not support specifying the selection "direction" i.e. the On 2016/05/16 11:18:06, karandeepb wrote: > On 2016/05/16 05:34:41, tapted wrote: > > It does :) > > > > Perhaps try [dummy_text_view_ setSelectedRange:range > > affinity:(NSSelectionAffinityUpstream <or> NSSelectionAffinityDownstream) > > stillSelecting:NO]; > > Can't get it to work. For example consider this test- > TEST_F(BridgedNativeWidgetTest, XYZ) { > NSTextView* dummy_text_view_ = [[NSTextView alloc] init]; > [dummy_text_view_ setString: @"12345"]; > > // Select "23" from left to right. > [dummy_text_view_ setSelectedRange: NSMakeRange(1,2) > affinity:NSSelectionAffinityDownstream stillSelecting:NO]; > > [dummy_text_view_ > doCommandBySelector:@selector(moveWordRightAndModifySelection:)]; > > // State should be "1|2345|". > EXPECT_EQ_RANGE(NSMakeRange(1, 4) ,[dummy_text_view_ selectedRange]); > > // Select "23" from right to left. > [dummy_text_view_ setSelectedRange: NSMakeRange(1,2) > affinity:NSSelectionAffinityUpstream stillSelecting:NO]; > > [dummy_text_view_ > doCommandBySelector:@selector(moveWordRightAndModifySelection:)]; > > // State should be "123|45". > EXPECT_EQ_RANGE(NSMakeRange(3, 0) ,[dummy_text_view_ selectedRange]); > } > > The last test fails, since "1|2345|" is selected. However, it passes with the > current MakeSelection implementation. > > Also see > http://stackoverflow.com/questions/20116963/how-can-i-get-the-selection-direc... Ah, can you update the comment then? i.e. we shouldn't say NSTextView doesn't support it, but something to the effect that TSTextView can accept an upstream/downstream affinity, but it doesn't work because of the reasons you specified. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:452: void BridgedNativeWidgetTest::TestEditingCommands(NSArray* selectors) { On 2016/05/16 11:18:07, karandeepb wrote: > On 2016/05/16 05:34:40, tapted wrote: > > nit: comment > > Doesn't the comment in the class declaration suffice? It seems we don't > generally comment on function definitions. Ah, I thought this was in an anon namespace for some reason. But I added that note here because this method has some extra logic that isn't captured in any comment. Specifically the range stuff. So either the comment on the earlier declaration should be expanded with a sentence like ~"This is done by selecting every substring within a set of test strings (both RTL and non-RTL) and performing every selector on both the NSTextField and the BridgedContentView hosting a focused views::TextField to ensure the resulting cursor and selection ranges match." or something equivalent inside the function here. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:454: const std::wstring test_string; On 2016/05/16 11:18:06, karandeepb wrote: > On 2016/05/16 05:34:41, tapted wrote: > > string16? wstring doesn't get used much. You'll probably need to have > > base::WideToUTF16 in the aggregate initializer list > > Done. Also a question. From what I saw, we use wstring for hardcoding non-ascii > strings generally. Is it the case that we don't use plain std::strings for > these, since the encoding will be implementation defined? yeah - wchar_t has different meanings on different platforms - I think it's even UCS4 (i.e. 32 bit) on some (maybe that's Windows in fact). https://codereview.chromium.org/1907253002/diff/180001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1907253002/diff/180001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:244: // Make selection from |start| to |end| on installed views::textfield and nit: textfield -> Textfield https://codereview.chromium.org/1907253002/diff/180001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:455: bool is_rtl; sorry - i just noticed is_rtl isn't read - should it just be a comment? or is there something to test? if you remove it this can just be an base::string16 test_strings[] (and the auto below should be replaced by base::string16)
PTAL Trent. Thanks. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:353: // NSTextView does not support specifying the selection "direction" i.e. the On 2016/05/17 04:02:57, tapted wrote: > On 2016/05/16 11:18:06, karandeepb wrote: > > On 2016/05/16 05:34:41, tapted wrote: > > > It does :) > > > > > > Perhaps try [dummy_text_view_ setSelectedRange:range > > > affinity:(NSSelectionAffinityUpstream <or> NSSelectionAffinityDownstream) > > > stillSelecting:NO]; > > > > Can't get it to work. For example consider this test- > > TEST_F(BridgedNativeWidgetTest, XYZ) { > > NSTextView* dummy_text_view_ = [[NSTextView alloc] init]; > > [dummy_text_view_ setString: @"12345"]; > > > > // Select "23" from left to right. > > [dummy_text_view_ setSelectedRange: NSMakeRange(1,2) > > affinity:NSSelectionAffinityDownstream stillSelecting:NO]; > > > > [dummy_text_view_ > > doCommandBySelector:@selector(moveWordRightAndModifySelection:)]; > > > > // State should be "1|2345|". > > EXPECT_EQ_RANGE(NSMakeRange(1, 4) ,[dummy_text_view_ selectedRange]); > > > > // Select "23" from right to left. > > [dummy_text_view_ setSelectedRange: NSMakeRange(1,2) > > affinity:NSSelectionAffinityUpstream stillSelecting:NO]; > > > > [dummy_text_view_ > > doCommandBySelector:@selector(moveWordRightAndModifySelection:)]; > > > > // State should be "123|45". > > EXPECT_EQ_RANGE(NSMakeRange(3, 0) ,[dummy_text_view_ selectedRange]); > > } > > > > The last test fails, since "1|2345|" is selected. However, it passes with the > > current MakeSelection implementation. > > > > Also see > > > http://stackoverflow.com/questions/20116963/how-can-i-get-the-selection-direc... > > Ah, can you update the comment then? i.e. we shouldn't say NSTextView doesn't > support it, but something to the effect that TSTextView can accept an > upstream/downstream affinity, but it doesn't work because of the reasons you > specified. Done. https://codereview.chromium.org/1907253002/diff/160001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:452: void BridgedNativeWidgetTest::TestEditingCommands(NSArray* selectors) { On 2016/05/17 04:02:57, tapted wrote: > On 2016/05/16 11:18:07, karandeepb wrote: > > On 2016/05/16 05:34:40, tapted wrote: > > > nit: comment > > > > Doesn't the comment in the class declaration suffice? It seems we don't > > generally comment on function definitions. > > Ah, I thought this was in an anon namespace for some reason. But I added that > note here because this method has some extra logic that isn't captured in any > comment. Specifically the range stuff. So either the comment on the earlier > declaration should be expanded with a sentence like ~"This is done by selecting > every substring within a set of test strings (both RTL and non-RTL) and > performing every selector on both the NSTextField and the BridgedContentView > hosting a focused views::TextField to ensure the resulting cursor and selection > ranges match." > > or something equivalent inside the function here. Done. https://codereview.chromium.org/1907253002/diff/180001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1907253002/diff/180001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:244: // Make selection from |start| to |end| on installed views::textfield and On 2016/05/17 04:02:57, tapted wrote: > nit: textfield -> Textfield Done. https://codereview.chromium.org/1907253002/diff/180001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:455: bool is_rtl; On 2016/05/17 04:02:57, tapted wrote: > sorry - i just noticed is_rtl isn't read - should it just be a comment? > > or is there something to test? > > if you remove it this can just be an base::string16 test_strings[] (and the auto > below should be replaced by base::string16) Removed.
sweet! lgtm
karandeepb@chromium.org changed reviewers: + msw@chromium.org
PTAL msw@. Thanks.
textfield_unittest.cc lgtm
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907253002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907253002/200001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement move and move*AndModifySelection commands. This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 ========== to ========== MacViews: Implement move and move*AndModifySelection commands. This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement move and move*AndModifySelection commands. This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 ========== to ========== MacViews: Implement move and move*AndModifySelection commands. This CL implements various move and move*AndModifySelection commands. Tests are also added to test expectations against a dummy NSTextView. BUG=586985 Committed: https://crrev.com/8cd49050ee5526cc61b92d189f37882377b4f617 Cr-Commit-Position: refs/heads/master@{#394632} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8cd49050ee5526cc61b92d189f37882377b4f617 Cr-Commit-Position: refs/heads/master@{#394632} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
