Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156423002:
Adjust width of find in page bar. (Closed)
|
Created:
4 years, 5 months ago by Evan Stade Modified:
4 years, 5 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjust width of find in page bar.
The text area now approximately fits "DesignDesignDesignDesign".
BUG=629217
Committed: https://crrev.com/043095f529a06276651d87e1f503c0d8f9867ace
Cr-Commit-Position: refs/heads/master@{#406356}
Patch Set 1 #Patch Set 2 : attempt to explain code #
Total comments: 13
Patch Set 3 : better comment #
Total comments: 2
Patch Set 4 : adopy sky's comment wording #Messages
Total messages: 23 (8 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org
https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:408: return size; Still seems like we should be including the size of the buttons calculated below (or whatever the MD equivalent is).
https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:408: return size; On 2016/07/18 23:44:54, Peter Kasting (slow) wrote: > Still seems like we should be including the size of the buttons calculated below > (or whatever the MD equivalent is). we do, via View::GetPreferredSize. It's just less manual in MD (relies on BoxLayout to calculate all that for us).
https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:94: const int kDefaultCharWidthMd = 26; Even though this constant is smaller, is the new overall size bigger because of the GetPreferredSize changes? https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:110: // to jump in size when the contents switch between empty and non-empty. Why is the jump in size problematic?
LGTM https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:94: const int kDefaultCharWidthMd = 26; When you gave your "DesignDesignDesignDesign" example, was that alongside a "0 of 0" match count label as well? If not, I'd probably increase this to be enough to fit at least both of those at once, ideally on some platform like Win 7 that apparently has really wide characters. https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:405: // space set aside for the input. This means "we want it to take up space set aside for the input", not "it will take up some of this space unless we subtract that off here", right? If so, maybe this would be slightly clearer?: Ignore the preferred size for the match count label, and just let it take up part of the space for the input textfield. This prevents the overall width from changing every time the match count text changes. https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:408: return size; On 2016/07/18 23:48:51, Evan Stade wrote: > On 2016/07/18 23:44:54, Peter Kasting (slow) wrote: > > Still seems like we should be including the size of the buttons calculated > below > > (or whatever the MD equivalent is). > > we do, via View::GetPreferredSize. It's just less manual in MD (relies on > BoxLayout to calculate all that for us). Ah. I missed that.
https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:94: const int kDefaultCharWidthMd = 26; On 2016/07/18 23:58:27, Peter Kasting (slow) wrote: > When you gave your "DesignDesignDesignDesign" example, was that alongside a "0 > of 0" match count label as well? yes indeedy > > If not, I'd probably increase this to be enough to fit at least both of those at > once, ideally on some platform like Win 7 that apparently has really wide > characters. https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:94: const int kDefaultCharWidthMd = 26; On 2016/07/18 23:56:04, sky wrote: > Even though this constant is smaller, is the new overall size bigger because of > the GetPreferredSize changes? yes (although I didn't verify this on Windows, just cros and linux) https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:110: // to jump in size when the contents switch between empty and non-empty. On 2016/07/18 23:56:04, sky wrote: > Why is the jump in size problematic? Because it makes the overall find bar width change when the match count label changes (i.e. as soon as you start typing). Instead we want the find bar to always be the same width. https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:405: // space set aside for the input. On 2016/07/18 23:58:27, Peter Kasting (slow) wrote: > This means "we want it to take up space set aside for the input", not "it will > take up some of this space unless we subtract that off here", right? > > If so, maybe this would be slightly clearer?: > > Ignore the preferred size for the match count label, and just let it take up > part of the space for the input textfield. This prevents the overall width from > changing every time the match count text changes. Done.
The CQ bit was checked by estade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:110: // to jump in size when the contents switch between empty and non-empty. On 2016/07/19 00:05:18, Evan Stade wrote: > On 2016/07/18 23:56:04, sky wrote: > > Why is the jump in size problematic? > > Because it makes the overall find bar width change when the match count label > changes (i.e. as soon as you start typing). Instead we want the find bar to > always be the same width. I'm confused. After your change the width is always 1 or the preferred width, which varies based on number of matches. Won't that result in jumping? Also, if the preferred width is based on the content won't the width vary?
https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:110: // to jump in size when the contents switch between empty and non-empty. On 2016/07/19 02:51:50, sky wrote: > On 2016/07/19 00:05:18, Evan Stade wrote: > > On 2016/07/18 23:56:04, sky wrote: > > > Why is the jump in size problematic? > > > > Because it makes the overall find bar width change when the match count label > > changes (i.e. as soon as you start typing). Instead we want the find bar to > > always be the same width. > > I'm confused. After your change the width is always 1 or the preferred width, > which varies based on number of matches. Won't that result in jumping? Also, if > the preferred width is based on the content won't the width vary? the preferred width is removed from the equation in FindBarView::GetPreferredSize below, but it has to be non-zero to begin with for the BoxLayout to put padding between it (match count text) and its siblings.
Got it, it would be better if the comment more clearly indicated this. LGTM
On 2016/07/19 17:52:58, sky wrote: > Got it, it would be better if the comment more clearly indicated this. LGTM I struggled with writing that comment. Suggestions for improvement welcome.
https://codereview.chromium.org/2156423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:108: // Always reserve at least 1dp for the match count label so the box layout Maybe just: We need to return at least one dip so that BoxLayout provides padding on each side.
https://codereview.chromium.org/2156423002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2156423002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:108: // Always reserve at least 1dp for the match count label so the box layout On 2016/07/19 18:11:41, sky wrote: > Maybe just: > We need to return at least one dip so that BoxLayout provides padding on each > side. Done.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2156423002/#ps60001 (title: "adopy sky's comment wording")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Adjust width of find in page bar. The text area now approximately fits "DesignDesignDesignDesign". BUG=629217 ========== to ========== Adjust width of find in page bar. The text area now approximately fits "DesignDesignDesignDesign". BUG=629217 Committed: https://crrev.com/043095f529a06276651d87e1f503c0d8f9867ace Cr-Commit-Position: refs/heads/master@{#406356} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/043095f529a06276651d87e1f503c0d8f9867ace Cr-Commit-Position: refs/heads/master@{#406356} |
