Merge lp:~zeitgeist/activity-log-manager/smaller-exclude-icons into lp:activity-log-manager

Status: Rejected
Rejected by: Manish Sinha (मनीष सिन्हा) on 2013-07-18
Proposed branch: lp:~zeitgeist/activity-log-manager/smaller-exclude-icons
Merge into: lp:activity-log-manager
Diff against target: 34 lines (+4/-4)
1 file modified
src/unified-privacy.vala (+4/-4)
To merge this branch: bzr merge lp:~zeitgeist/activity-log-manager/smaller-exclude-icons
Reviewer Review Type Date Requested Status
Rico Tzschichholz 2013-07-09 Disapprove on 2013-07-09
Matthew Paul Thomas (community) design 2013-07-09 Needs Fixing on 2013-07-09
Jeremy Bicha 2013-07-09 Pending
Review via email: mp+173678@code.launchpad.net

Description of the change

Fixes LP: #1198364 where exclude list has smaller icons

Looks like: http://i.imgur.com/zPwGMPp.png

To post a comment you must log in.
Matthew Paul Thomas (mpt) wrote :

This looks promising, but all icons should be the same size. Folder and document icons should be just as small as app icons.

review: Needs Fixing (design)
Rico Tzschichholz (ricotz) wrote :

I don't see the need in reducing the icon-sizes. The problem which you want to fix is not caused by them. The underlying widget structure is statically sized and therefore doesn't adapt to different font-sizes.

And yes, all icons should have the same sizes.

So this approach is not the right thing to do imo. (which makes the bug invalid as well)

review: Disapprove
Matthew Paul Thomas (mpt) wrote :

Ah, I didn't read the bug report, sorry.

I think that for consistency, icons in this list should be laid out exactly the same way as icons in menu items are. That does mean they should be smaller. However, Rico is right that that wouldn't actually fix the vertical alignment of the text. In the screenshot, the text for the previous-sized folder icons is too high, but the text for the newly-sized application icons is too low. It should be vertically centered, just like text in menu items.

Unmerged revisions

76. By Manish Sinha (मनीष सिन्हा) on 2013-07-09

Fixes LP: #1198364 where exclude list has smaller icons

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/unified-privacy.vala'
2--- src/unified-privacy.vala 2013-07-09 01:34:49 +0000
3+++ src/unified-privacy.vala 2013-07-09 09:33:25 +0000
4@@ -481,7 +481,7 @@
5 else if (dir.to_string() == "G_USER_DIRECTORY_PUBLIC_SHARE")
6 nautilus_icon = new ThemedIcon("folder-publicshare");
7 if(nautilus_icon != null) {
8- var pixbuf = ApplicationsTreeView.get_pixbuf_from_gio_icon(nautilus_icon, 24);
9+ var pixbuf = ApplicationsTreeView.get_pixbuf_from_gio_icon(nautilus_icon, 16);
10 if (pixbuf != null)
11 icon = pixbuf;
12 }
13@@ -519,7 +519,7 @@
14 private void add_app_to_view (string app) {
15 DesktopAppInfo app_info = new DesktopAppInfo(app);
16 if (app_info != null ) {
17- var pix = ApplicationsTreeView.get_pixbuf_from_gio_icon(app_info.get_icon(), 24);
18+ var pix = ApplicationsTreeView.get_pixbuf_from_gio_icon(app_info.get_icon(), 16);
19 var markup = ApplicationsTreeView.markup_for_app(app_info);
20 TreeIter iter;
21 this.exception_list_store.append(out iter);
22@@ -564,11 +564,11 @@
23 //Based on gnome-contacts contacts-cell-renderer-shape.vala
24 public class ExceptionCellRenderer : CellRenderer {
25
26- private const int PIXBUF_SIZE = 24;
27+ private const int PIXBUF_SIZE = 16;
28 private const int xspacing = 3;
29
30 private const int default_width = 60;
31- private const int renderer_height = 30;
32+ private const int renderer_height = 22;
33
34 private Widget current_widget;
35

Subscribers

People subscribed via source and target branches