Merge lp:~jmiguelbenitez/pantheon-files/fix-1076504 into lp:~elementary-apps/pantheon-files/trunk

Proposed by Cody Garver
Status: Merged
Approved by: Cody Garver
Approved revision: 1405
Merged at revision: 1427
Proposed branch: lp:~jmiguelbenitez/pantheon-files/fix-1076504
Merge into: lp:~elementary-apps/pantheon-files/trunk
Diff against target: 68 lines (+18/-25)
1 file modified
src/fm-icon-view.c (+18/-25)
To merge this branch: bzr merge lp:~jmiguelbenitez/pantheon-files/fix-1076504
Reviewer Review Type Date Requested Status
Jeremy Wootten Approve
Review via email: mp+200208@code.launchpad.net

Commit message

Improve icon view spacing to fix bug #1076504

To post a comment you must log in.
Revision history for this message
Jeremy Wootten (jeremywootten) wrote :

Performs as specified and no conflicts found with current trunk.
There are some minor formatting problems: misalignments in diff lines 49, 53 57.

My only reservation is that the ratio of item-width to icon-size is arbitrarily set at 2 and hard-coded. I tried altering this and personally found a slightly more compact layout more aesthetically pleasing, using the "Golden Ratio" (162/100 approx). Wouldn't it be better to make this a setting or get the input of the design team as to the optimum ratio?

Also, the entire switch statement could be replaced by a single line using a fixed ratio of wrap-width to item-width of 95/100 with no discernible difference.

review: Needs Fixing
Revision history for this message
José M. Benítez (jmiguelbenitez) wrote :

Thanks, Jeremy!

I totally agree with you that the design team should have a say on this subject. I just picked some width to show how the whole thing would end up looking. I should have asked for the help of the design guys explicitly.

I wasn't sure about different ratios for each icon size either. That's why I kept the switch statement.

How should I proceed from here then?

Revision history for this message
Danielle Foré (danrabbit) wrote :

This looks great! I'm not sure the difference between 2 and 1.62 is terribly noticeable, but let's go with 1.62 just for fun :)

Revision history for this message
José M. Benítez (jmiguelbenitez) wrote :

> This looks great! I'm not sure the difference between 2 and 1.62 is terribly
> noticeable, but let's go with 1.62 just for fun :)

For every zoom level then?

Revision history for this message
José M. Benítez (jmiguelbenitez) wrote :

> > This looks great! I'm not sure the difference between 2 and 1.62 is terribly
> > noticeable, but let's go with 1.62 just for fun :)
>
> For every zoom level then?

Sorry. Yes, the item width fixed at 1.62 for every zoom level. I meant the wrap width.

1403. By José M. Benítez

Set item width in icon view to the golden ratio.

1404. By José M. Benítez

Set fixed wrap width for every zoom level in icon view.

1405. By José M. Benítez

Used preprocessor constant to improve readability.

Revision history for this message
Danielle Foré (danrabbit) wrote :

Yeah everything looks good on design side to me :) Great work!

Revision history for this message
Jeremy Wootten (jeremywootten) wrote :

Excellent! This is good to go.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/fm-icon-view.c'
--- src/fm-icon-view.c 2013-09-09 03:08:16 +0000
+++ src/fm-icon-view.c 2014-02-08 23:18:43 +0000
@@ -30,6 +30,10 @@
30G_DEFINE_TYPE (FMIconView, fm_icon_view, FM_TYPE_ABSTRACT_ICON_VIEW)30G_DEFINE_TYPE (FMIconView, fm_icon_view, FM_TYPE_ABSTRACT_ICON_VIEW)
3131
3232
33/* Golden ratio used */
34#define ICON_SIZE_TO_ITEM_WIDTH_RATIO 1.62f
35
36
33static void37static void
34fm_icon_view_class_init (FMIconViewClass *klass)38fm_icon_view_class_init (FMIconViewClass *klass)
35{39{
@@ -104,39 +108,28 @@
104fm_icon_view_zoom_level_changed (FMDirectoryView *view)108fm_icon_view_zoom_level_changed (FMDirectoryView *view)
105{109{
106 gint wrap_width;110 gint wrap_width;
111 gint icon_size;
112 gint item_width;
107113
108 g_return_if_fail (FM_IS_DIRECTORY_VIEW (view));114 g_return_if_fail (FM_IS_DIRECTORY_VIEW (view));
109115
110 /* determine the "wrap-width" depending on the "zoom-level" */116 icon_size = marlin_zoom_level_to_icon_size (view->zoom_level);
111 switch (view->zoom_level)117
112 {118 /* determine the "item-width" depending on the "zoom-level" */
113 case MARLIN_ZOOM_LEVEL_SMALLEST:119 item_width = ICON_SIZE_TO_ITEM_WIDTH_RATIO * icon_size;
114 wrap_width = 48;120
115 break;121 /* determine the "wrap-width" depending on the "zoom-level": a couple of
116122 * pixels will do */
117 case MARLIN_ZOOM_LEVEL_SMALLER:123 wrap_width = item_width - 2;
118 wrap_width = 64;
119 break;
120
121 case MARLIN_ZOOM_LEVEL_SMALL:
122 wrap_width = 72;
123 break;
124
125 case MARLIN_ZOOM_LEVEL_NORMAL:
126 wrap_width = 112;
127 break;
128
129 default:
130 wrap_width = 128;
131 break;
132 }
133124
134 /* set the new "wrap-width" for the text renderer */125 /* set the new "wrap-width" for the text renderer */
135 g_object_set (FM_DIRECTORY_VIEW (view)->name_renderer, "wrap-width", wrap_width, "zoom-level", view->zoom_level, NULL);126 g_object_set (FM_DIRECTORY_VIEW (view)->name_renderer, "wrap-width", wrap_width, "zoom-level", view->zoom_level, NULL);
136 //g_object_set (FM_DIRECTORY_VIEW (view)->name_renderer, "zoom-level", view->zoom_level, NULL);
137127
138 /* set the new "size" for the icon renderer */128 /* set the new "size" for the icon renderer */
139 g_object_set (FM_DIRECTORY_VIEW (view)->icon_renderer, "size", marlin_zoom_level_to_icon_size (view->zoom_level), "zoom-level", view->zoom_level, NULL);129 g_object_set (FM_DIRECTORY_VIEW (view)->icon_renderer, "size", icon_size, "zoom-level", view->zoom_level, NULL);
130
131 /* set the new "item-width" for the icon view */
132 g_object_set (FM_ABSTRACT_ICON_VIEW (view)->icons, "item-width", item_width, NULL);
140133
141 exo_icon_view_invalidate_sizes (FM_ABSTRACT_ICON_VIEW (view)->icons);134 exo_icon_view_invalidate_sizes (FM_ABSTRACT_ICON_VIEW (view)->icons);
142}135}

Subscribers

People subscribed via source and target branches

to all changes: