Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1975653002:
add 'langauge settings' link to the translate bubble advanced view (Closed)
|
Created:
4 years, 7 months ago by ftang Modified:
4 years, 6 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd 'langauge settings' link to the translate bubble advanced view
BUG=611268
Committed: https://crrev.com/e83e46d01ab24945c9e1e3b456e5ecdb2c2a373a
Cr-Commit-Position: refs/heads/master@{#401635}
Patch Set 1 #
Total comments: 7
Patch Set 2 : change according to review comments #Patch Set 3 : refactor code into TranslateBubbleViewModel::OpenLanguageSettingsPage() based on review request #Patch Set 4 : rollback to keep the code in the view and controller to avoid linking / dependency issue #Patch Set 5 : add logging #
Total comments: 4
Patch Set 6 : shortern the local varible name and increase the y pos of the link by 1 to vertical align the basel… #
Total comments: 4
Patch Set 7 : revised based on review feedback #Messages
Total messages: 48 (15 generated)
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975653002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975653002/1
ftang@chromium.org changed reviewers: + groby@chromium.org
Please add a screenshot of the new UI (on bug, or link) https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:446: [self addLinkButtonWithText:message I think advancedLanguageSettingsLinkButton_ can be a local variable - you don't use it anywhere else. https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:463: // origin of the link up. Up relative to what? Do you mean center it w/ the done button, or do I misread the code? https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:654: GURL url = chrome::GetSettingsUrl(chrome::kLanguageOptionsSubPage); It would be nice if this could be a shared function on the model :) (I know, I repeat myself)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
change according to review comments
refactor code into TranslateBubbleViewModel::OpenLanguageSettingsPage() based on review request
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975653002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
I did what you ask me to do to refactor the code from view code into the model code. But now the andriod build break in linking. I suspect it is ok to let the view code to depend on the chrome::getSettingsUrl but not ok to let the model code to do so. https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:446: [self addLinkButtonWithText:message On 2016/05/12 01:23:40, groby wrote: > I think advancedLanguageSettingsLinkButton_ can be a local variable - you don't > use it anywhere else. Done. https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:463: // origin of the link up. On 2016/05/12 01:23:40, groby wrote: > Up relative to what? Do you mean center it w/ the done button, or do I misread > the code? Done. https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:654: GURL url = chrome::GetSettingsUrl(chrome::kLanguageOptionsSubPage); On 2016/05/12 01:23:39, groby wrote: > It would be nice if this could be a shared function on the model :) (I know, I > repeat myself) are you talking about TranslateBubbleModel ?
rollback to keep the code in the view and controller to avoid linking / dependency issue
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975653002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975653002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. I don't have a good place to put the common code due to dependency issue. The code to make a common code may be much more line than duplicate the code. https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:654: GURL url = chrome::GetSettingsUrl(chrome::kLanguageOptionsSubPage); On 2016/05/12 04:45:21, ftang wrote: > On 2016/05/12 01:23:39, groby wrote: > > It would be nice if this could be a shared function on the model :) (I know, I > > repeat myself) > > are you talking about TranslateBubbleModel ? I try to move it to TranslateBubbleModel or TranslateUIDelegate but either will cause linking/dependcy issues. I have to rollback to put into .mm file and view.cc file.
ping
add logging
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975653002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975653002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Is there a screenshot? https://codereview.chromium.org/1975653002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:451: NSButton* advancedLanguageSettingsLinkButton = Why is this a button? Should this be a hyperlink?
PATL https://codereview.chromium.org/1975653002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:451: NSButton* advancedLanguageSettingsLinkButton = On 2016/05/18 23:08:16, groby wrote: > Why is this a button? Should this be a hyperlink? I just reuse the pre-existing addLinkButtonWithText which show the "options" link in these view. What Cocoa class do you have in mind for this instead?
https://codereview.chromium.org/1975653002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:451: NSButton* advancedLanguageSettingsLinkButton = On 2016/05/18 23:57:42, ftang wrote: > On 2016/05/18 23:08:16, groby wrote: > > Why is this a button? Should this be a hyperlink? > > I just reuse the pre-existing addLinkButtonWithText which show the "options" > link in these view. What Cocoa class do you have in mind for this instead? I was thinking of HyperLinkTextView - but addLinkButton seems to be a valid way as well... https://codereview.chromium.org/1975653002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:470: yPos = (NSHeight([advancedDoneButton_ frame]) - You don't want to center this with the button - you instead want to align the baselines of the text in the buttons and the link. Eyeballing the screenshot, you want to nudge the link 1pt up. (Please leave a comment you're adjusting to align text baselines)
shortern the local varible name and increase the y pos of the link by 1 to vertical align the baseline of the link text and button text.
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975653002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1975653002/100001
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1975653002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:474: // the text inside the link veritcal align with the text inside the buttons. nit:vertically https://codereview.chromium.org/1975653002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:476: NSHeight([languageSettingsLinkButton frame])) / 2); You probably want to use ceil/floor on this, unless the difference is guaranteed to be even. Fractional positioning causes blurry-looking text. (Sorry, forgot this in last review)
floor the yPos of the 'Language setting' link based on review suggestion
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975653002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/1975653002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1975653002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:474: // the text inside the link veritcal align with the text inside the buttons. On 2016/05/23 18:57:07, groby wrote: > nit:vertically Done. https://codereview.chromium.org/1975653002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:476: NSHeight([languageSettingsLinkButton frame])) / 2); On 2016/05/23 18:57:07, groby wrote: > You probably want to use ceil/floor on this, unless the difference is guaranteed > to be even. Fractional positioning causes blurry-looking text. (Sorry, forgot > this in last review) Done.
LGTM
The CQ bit was checked by ftang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1975653002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== add 'langauge settings' link to the translate bubble advanced view BUG=611268 ========== to ========== add 'langauge settings' link to the translate bubble advanced view BUG=611268 Committed: https://crrev.com/e83e46d01ab24945c9e1e3b456e5ecdb2c2a373a Cr-Commit-Position: refs/heads/master@{#401635} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e83e46d01ab24945c9e1e3b456e5ecdb2c2a373a Cr-Commit-Position: refs/heads/master@{#401635} |
