Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2344703002:
Fix task manager's default sizing. (Closed)
|
Created:
4 years, 3 months ago by Evan Stade Modified:
4 years, 2 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix task manager's default sizing.
Set default width based on contents rather than hardcoding a value (which happened to be too small once we adjusted outer padding for the dialog).
BUG=614317
Committed: https://crrev.com/6678470d3169979d811eec56e7cdea668ab751c1
Cr-Commit-Position: refs/heads/master@{#422893}
Patch Set 1 #Patch Set 2 : no dbg #
Total comments: 2
Patch Set 3 : rabbit holes #
Total comments: 2
Patch Set 4 : less work #
Total comments: 2
Patch Set 5 : . #
Total comments: 5
Patch Set 6 : rebase + commentary #
Messages
Total messages: 38 (22 generated)
Description was changed from ========== Fix task manager's default sizing. Also improve some TableView behavior BUG= ========== to ========== Fix task manager's default sizing. Set default width based on contents rather than hardcoding a value (which happened to be too small once we adjusted outer padding for the dialog). BUG=614317 ==========
estade@chromium.org changed reviewers: + sky@chromium.org
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/2344703002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/task_manager_view.cc (right): https://codereview.chromium.org/2344703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/task_manager_view.cc:150: gfx::Size contents_size(tab_table_->GetPreferredSize().width(), 270); I think you should be able to ask tab_table_parent_ directly for the preferred size and it should do the right thing. ScrollView considers the insets for the bounded case, but not the !bounded case. Can you make ScrollView do the right thing so that you only need set the height to 270 here? Seems like TableView::CreateParent should return a ScrollView too so that you can call ClipHeight directly to, but that's separate.
https://codereview.chromium.org/2344703002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/task_manager_view.cc (right): https://codereview.chromium.org/2344703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/task_manager_view.cc:150: gfx::Size contents_size(tab_table_->GetPreferredSize().width(), 270); On 2016/09/15 02:52:13, sky wrote: > I think you should be able to ask tab_table_parent_ directly for the preferred > size and it should do the right thing. ScrollView considers the insets for the > bounded case, but not the !bounded case. Can you make ScrollView do the right > thing so that you only need set the height to 270 here? Seems like > TableView::CreateParent should return a ScrollView too so that you can call > ClipHeight directly to, but that's separate. ok, done. I was wary of touching anything in TableView or ScrollView because it's complicated and I could be breaking something I'm not aware of, but this patch seems to do the right thing. I also tinkered with TableView::Layout because when there were enough tasks to create a vertical scrollbar, that scrollbar was being placed on top of the contents, obscuring part of the rightmost column (the contents weren't resized to fit the space that was left over after the scrollbar was shown).
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/2344703002/diff/40001/ui/views/controls/table... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2344703002/diff/40001/ui/views/controls/table... ui/views/controls/table/table_view.cc:319: if (viewport && width() != viewport->width() && !in_set_visible_column_width_) I believe your change will result in forcing a call to UpdateVisibleColumnSizes every time Layout is called if the preferred width is > viewport->width().
https://codereview.chromium.org/2344703002/diff/40001/ui/views/controls/table... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2344703002/diff/40001/ui/views/controls/table... ui/views/controls/table/table_view.cc:319: if (viewport && width() != viewport->width() && !in_set_visible_column_width_) On 2016/09/16 21:28:58, sky wrote: > I believe your change will result in forcing a call to UpdateVisibleColumnSizes > every time Layout is called if the preferred width is > viewport->width(). I guess that's right. We're already calling it every time the parent size changes even though we don't need to when there's a horizontal scrollbar, but I guess in the case where Layout() is called multiple times without a change to parent size this is now doing extra work. Fixed.
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2344703002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2344703002/diff/60001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:281: // If not vertically bounded, there's no specific preferred height. Is there a reason to do this? I would be inclined not to.
https://codereview.chromium.org/2344703002/diff/60001/ui/views/controls/scrol... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2344703002/diff/60001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:281: // If not vertically bounded, there's no specific preferred height. On 2016/09/22 17:39:58, sky wrote: > Is there a reason to do this? I would be inclined not to. If !is_bounded() then max_height_ will be 0, so size.height() would be 0. I suppose we can instead skip the SetToMin/Max steps if !is_bounded() https://codereview.chromium.org/2344703002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/task_manager_view.cc (right): https://codereview.chromium.org/2344703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/task_manager_view.cc:160: table_size.set_width(table_size.width() + 180); I also added this because columns that have an expand percent only report a preferred width that's just enough for the header.[1] In the case of the task manager, that header is "Task", which means all the tab/process titles longer than "Task" default to being elided. [1] https://cs.chromium.org/chromium/src/ui/views/controls/table/table_utils.cc?r...
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/2344703002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/task_manager_view.cc (right): https://codereview.chromium.org/2344703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/task_manager_view.cc:157: // The task name column expands to use excess space but also defaults to 'to to' -> 'to' https://codereview.chromium.org/2344703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/task_manager_view.cc:160: table_size.set_width(table_size.width() + 180); On 2016/09/22 22:42:47, Evan Stade wrote: > I also added this because columns that have an expand percent only report a > preferred width that's just enough for the header.[1] In the case of the task > manager, that header is "Task", which means all the tab/process titles longer > than "Task" default to being elided. > > [1] > https://cs.chromium.org/chromium/src/ui/views/controls/table/table_utils.cc?r... I recommend expanding your comment to include this information as well. That said, remember that not all locales will have 'Task' as 4 characters. Might this result in too big a table in some locales?
sorry I keep deprioritizing this. https://codereview.chromium.org/2344703002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/task_manager_view.cc (right): https://codereview.chromium.org/2344703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/task_manager_view.cc:157: // The task name column expands to use excess space but also defaults to On 2016/09/23 16:22:25, sky wrote: > 'to to' -> 'to' Done. https://codereview.chromium.org/2344703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/task_manager_view.cc:160: table_size.set_width(table_size.width() + 180); On 2016/09/23 16:22:26, sky wrote: > On 2016/09/22 22:42:47, Evan Stade wrote: > > I also added this because columns that have an expand percent only report a > > preferred width that's just enough for the header.[1] In the case of the task > > manager, that header is "Task", which means all the tab/process titles longer > > than "Task" default to being elided. > > > > [1] > > > https://cs.chromium.org/chromium/src/ui/views/controls/table/table_utils.cc?r... > > I recommend expanding your comment to include this information as well. Tried to improve/elaborate comment. > That > said, remember that not all locales will have 'Task' as 4 characters. Might this > result in too big a table in some locales? it's possible, but there would be minimal harm. If the rule of thumb is to plan for translations being up to 3x as long, that won't really amount to that much space.
LGTM
The CQ bit was checked by estade@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 task manager's default sizing. Set default width based on contents rather than hardcoding a value (which happened to be too small once we adjusted outer padding for the dialog). BUG=614317 ========== to ========== Fix task manager's default sizing. Set default width based on contents rather than hardcoding a value (which happened to be too small once we adjusted outer padding for the dialog). BUG=614317 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix task manager's default sizing. Set default width based on contents rather than hardcoding a value (which happened to be too small once we adjusted outer padding for the dialog). BUG=614317 ========== to ========== Fix task manager's default sizing. Set default width based on contents rather than hardcoding a value (which happened to be too small once we adjusted outer padding for the dialog). BUG=614317 Committed: https://crrev.com/6678470d3169979d811eec56e7cdea668ab751c1 Cr-Commit-Position: refs/heads/master@{#422893} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6678470d3169979d811eec56e7cdea668ab751c1 Cr-Commit-Position: refs/heads/master@{#422893}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
With this CL, when I open the task manager on Linux, it just expands wider and wider and wider...
Message was sent while issue was closed.
On 2016/10/05 00:21:38, Lei Zhang wrote: > With this CL, when I open the task manager on Linux, it just expands wider and > wider and wider... |last_layout_width_| jumps from 43 to 5587 ... to 7942 GetPreferredSize() in scroll_view.cc and |table_size| in task_manager_view.cc similarly gets wider and wider.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2389803006/ by thestig@chromium.org. The reason for reverting is: On Linux, the task manager window self expands and gets wider and wiser.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
