Merge lp:~jeremywootten/pantheon-files/fix-focus-clipping-in-icon-view into lp:~elementary-apps/pantheon-files/trunk

Proposed by Jeremy Wootten on 2017-02-05
Status: Merged
Approved by: Daniel Fore on 2017-06-14
Approved revision: 2474
Merged at revision: 2581
Proposed branch: lp:~jeremywootten/pantheon-files/fix-focus-clipping-in-icon-view
Merge into: lp:~elementary-apps/pantheon-files/trunk
Diff against target: 280 lines (+106/-63)
3 files modified
src/TextRenderer.vala (+97/-55)
src/View/AbstractDirectoryView.vala (+0/-1)
src/View/IconView.vala (+9/-7)
To merge this branch: bzr merge lp:~jeremywootten/pantheon-files/fix-focus-clipping-in-icon-view
Reviewer Review Type Date Requested Status
Zisu Andrei (community) Approve on 2017-06-13
Daniel Fore 2017-02-05 Abstain on 2017-06-08
Review via email: mp+316409@code.launchpad.net

Commit message

fixes visual glitches in the text rendering:
* clipping of the focus rectangle in icon view
* Incorrect alignment in ListView with RTL language
* Reduce excess vertical spacing of icons with short names

Description of the change

This branch builds on lp:~jeremywootten/pantheon-files/fix-1658417-long-name-in-grid-view, (now merged).

It fixes some visual glitches in the text rendering:

1) clipping of the focus/color rectangle in icon view at some zoom levels/name lengths.
2) Incorrect alignment in ListView with RTL language

It also reduces the excess vertical spacing of icons with short names.

To post a comment you must log in.
Daniel Fore (danrabbit) wrote :

Looks like this branch has merge conflicts

review: Needs Fixing
2467. By Jeremy Wootten on 2017-05-07

Merge trunk to r 2556 and resolve conflicts

Jeremy Wootten (jeremywootten) wrote :

Trunk merged to r2556 and conflicts resolved.

2468. By Jeremy Wootten on 2017-05-23

Merge trunk to r2563

2469. By Jeremy Wootten on 2017-05-23

Ensure no clipping, improve centering with LTR and RTL

Dieter Debast (ddieter) wrote :

Issue #1405853 also says that clicking on the whitespace (when the file is highlighted) doesn't select the file/folder, but this isn't fixed in this branch (maybe this can be put into a separate issue?). Dragging a selection box over this whitespace does seem to work.

Daniel Fore (danrabbit) wrote :

Changing my review to "abstain" so that it doesn't appear to need fixing on my account

review: Abstain
Jeremy Wootten (jeremywootten) wrote :

Dieter: I have removed the second, unconnected issue from #1405853 and asked the reported to refile that as a separate bug. So this branch is only intended to fix the excess whitespace and clipping issues.

Zisu Andrei (matzipan) wrote :

Some feedback. I confirm it fixes the rounded corners issue. And I confirm there is a reduction of the "excess spacing"

review: Needs Fixing
Jeremy Wootten (jeremywootten) wrote :

See replies to inline comments. Will address the code style issues.

2470. By Jeremy Wootten on 2017-06-13

Uncapitalize BORDER_RADIUS

2471. By Jeremy Wootten on 2017-06-13

Insert missing brackets

2472. By Jeremy Wootten on 2017-06-13

Remove superfluous brackets

2473. By Jeremy Wootten on 2017-06-13

Merge trunk to r2578

Zisu Andrei (matzipan) wrote :

As discussed on slack, perhaps the line below should be a comment in the code, to clarify:
> To ensure the focus_rect is at least one pixel smaller than cell_area on each side. Helps prevent clipping.

Other than that. LGTM.

review: Approve
2474. By Jeremy Wootten on 2017-06-13

Add comment

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/TextRenderer.vala'
2--- src/TextRenderer.vala 2017-02-15 18:54:50 +0000
3+++ src/TextRenderer.vala 2017-06-13 16:37:16 +0000
4@@ -20,18 +20,35 @@
5 public class TextRenderer: Gtk.CellRendererText {
6
7 const int MAX_LINES = 5;
8- const uint BORDER_RADIUS = 6;
9+ private int border_radius = 6;
10
11 public Marlin.ZoomLevel zoom_level {get; set;}
12- public bool follow_state {get; set;}
13 public new string background { set; private get;}
14 public GOF.File? file {set; private get;}
15+ private int _item_width;
16+ public int item_width {
17+ set {
18+ _item_width = value;
19+ if (xalign == 0.5) {
20+ wrap_width = _item_width - 2 * (focus_border_width + (int)xpad + border_radius);
21+ } else {
22+ wrap_width = -1;
23+ }
24+
25+ border_radius = 5 + wrap_width / 40;
26+ }
27+
28+ private get {
29+ return _item_width;
30+ }
31+ }
32+
33 public int text_width;
34 public int text_height;
35
36- int char_width;
37 int char_height;
38 int focus_border_width;
39+
40 Pango.Layout layout;
41 Gtk.Widget widget;
42 Marlin.AbstractEditableLabel entry;
43@@ -81,19 +98,29 @@
44 style_context.save ();
45 style_context.set_state (state);
46
47- if (follow_state || background != null)
48- draw_focus (cr, cell_area, flags, style_context, state);
49-
50- int x_offset, y_offset;
51- get_offsets (cell_area, text_width, text_height, xalign, out x_offset, out y_offset);
52-
53- /* Adjust text offsets for best appearance in each view */
54- if (xalign == 0.5f) { /* Icon view */
55- x_offset = (cell_area.width - this.wrap_width) / 2;
56- y_offset += focus_border_width + (int)ypad;
57+ int x_offset, y_offset, focus_rect_width, focus_rect_height;
58+ draw_focus (cr, cell_area, flags, style_context, state, out x_offset, out y_offset,
59+ out focus_rect_width, out focus_rect_height);
60+
61+ /* Position text relative to the focus rectangle */
62+ if (xalign == 0.5f) {
63+ x_offset = (cell_area.width - wrap_width) / 2;
64+
65+ if (widget.get_direction () == Gtk.TextDirection.RTL) {
66+ x_offset -= focus_border_width;
67+ } else {
68+ x_offset += focus_border_width;
69+ }
70+
71+ y_offset += (focus_rect_height - text_height) / 2;
72 } else {
73- x_offset += focus_border_width + 2 * (int)xpad;
74- y_offset += focus_border_width;
75+ y_offset = (cell_area.height - char_height) / 2;
76+
77+ if (widget.get_direction () == Gtk.TextDirection.RTL) {
78+ x_offset += (focus_rect_width - text_width) - focus_border_width * 4;
79+ } else {
80+ x_offset += focus_border_width * 2 + (int)xpad;
81+ }
82 }
83
84 style_context.render_layout (cr,
85@@ -189,8 +216,6 @@
86 private void set_widget (Gtk.Widget? _widget) {
87 Pango.FontMetrics metrics;
88 Pango.Context context;
89- int focus_padding;
90- int focus_line_width;
91
92 if (_widget == widget)
93 return;
94@@ -208,16 +233,19 @@
95 layout.set_auto_dir (false);
96 layout.set_single_paragraph_mode (true);
97 metrics = context.get_metrics (layout.get_font_description (), context.get_language ());
98- char_width = (metrics.get_approximate_char_width () + 512 ) >> 10;
99 char_height = (metrics.get_ascent () + metrics.get_descent () + 512) >> 10;
100- if (wrap_width < 0)
101+
102+ if (wrap_width < 0) {
103 (this as Gtk.CellRenderer).set_fixed_size (-1, char_height);
104+ }
105
106+ int focus_padding;
107+ int focus_line_width;
108 widget.style_get ("focus-padding", out focus_padding, "focus-line-width", out focus_line_width);
109 focus_border_width = int.max (focus_padding + focus_line_width, 2);
110+
111 } else {
112 layout = null;
113- char_width = 0;
114 char_height = 0;
115 }
116 }
117@@ -256,40 +284,49 @@
118 Gdk.Rectangle cell_area,
119 Gtk.CellRendererState flags,
120 Gtk.StyleContext style_context,
121- Gtk.StateFlags state) {
122+ Gtk.StateFlags state,
123+ out int x_offset,
124+ out int y_offset,
125+ out int focus_rect_width,
126+ out int focus_rect_height) {
127+
128 bool selected = false;
129- float x;
130- int x_offset, y_offset, focus_rect_width, focus_rect_height;
131-
132- if (follow_state)
133- selected = ((flags & Gtk.CellRendererState.SELECTED) == Gtk.CellRendererState.SELECTED);
134-
135- focus_rect_width = text_width + 4 * this.focus_border_width;
136- focus_rect_height = text_height + 2 * this.focus_border_width;
137-
138- if (widget.get_direction () == Gtk.TextDirection.RTL)
139- x = 1.0f - xalign;
140- else
141- x = xalign;
142-
143- get_offsets (cell_area, focus_rect_width, focus_rect_height, x, out x_offset, out y_offset);
144+ focus_rect_width = 0;
145+ focus_rect_height = 0;
146+ x_offset = 0;
147+ y_offset = 0;
148+
149+ selected = ((flags & Gtk.CellRendererState.SELECTED) == Gtk.CellRendererState.SELECTED);
150+
151+ focus_rect_height = text_height + 2 * (this.focus_border_width + border_radius);
152+ focus_rect_width = text_width + 4 * (this.focus_border_width + border_radius);
153+
154+ /* Ensure that focus_rect is at least one pixel small than cell_area on each side */
155+ focus_rect_width = int.min (focus_rect_width, cell_area.width - 2);
156+ focus_rect_height = int.min (focus_rect_height, cell_area.height -2);
157+
158+ get_offsets (cell_area, focus_rect_width, focus_rect_height, out x_offset, out y_offset);
159+
160+
161
162 /* render the background if selected or colorized */
163 if (selected || this.background != null) {
164- int x0 = cell_area.x + x_offset + (int)xpad;
165- int y0 = cell_area.y + y_offset + (int)ypad;
166+ int x0 = cell_area.x + x_offset;
167+ int y0 = cell_area.y + y_offset;
168 int x1 = x0 + focus_rect_width;
169 int y1 = y0 + focus_rect_height;
170-
171- cr.move_to (x0 + BORDER_RADIUS, y0);
172- cr.line_to (x1 - BORDER_RADIUS, y0);
173- cr.curve_to (x1 - BORDER_RADIUS, y0, x1, y0, x1, y0 + BORDER_RADIUS);
174- cr.line_to (x1, y1 - BORDER_RADIUS);
175- cr.curve_to (x1, y1 - BORDER_RADIUS, x1, y1, x1 - BORDER_RADIUS, y1);
176- cr.line_to (x0 + BORDER_RADIUS, y1);
177- cr.curve_to (x0 + BORDER_RADIUS, y1, x0, y1, x0, y1 - BORDER_RADIUS);
178- cr.line_to (x0, y0 + BORDER_RADIUS);
179- cr.curve_to (x0, y0 + BORDER_RADIUS, x0, y0, x0 + BORDER_RADIUS, y0);
180+ if (x1 >= cell_area.x + cell_area.width) {
181+ x1 = cell_area.x + cell_area.width - 1;
182+ }
183+ cr.move_to (x0 + border_radius, y0);
184+ cr.line_to (x1 - border_radius, y0);
185+ cr.curve_to (x1 - border_radius, y0, x1, y0, x1, y0 + border_radius);
186+ cr.line_to (x1, y1 - border_radius);
187+ cr.curve_to (x1, y1 - border_radius, x1, y1, x1 - border_radius, y1);
188+ cr.line_to (x0 + border_radius, y1);
189+ cr.curve_to (x0 + border_radius, y1, x0, y1, x0, y1 - border_radius);
190+ cr.line_to (x0, y0 + border_radius);
191+ cr.curve_to (x0, y0 + border_radius, x0, y0, x0 + border_radius, y0);
192
193 Gdk.RGBA color ={};
194 if (background != null && !selected) {
195@@ -304,25 +341,30 @@
196 cr.fill ();
197 }
198 /* draw the focus indicator */
199- if (follow_state && (flags & Gtk.CellRendererState.FOCUSED) != 0)
200+ if ((flags & Gtk.CellRendererState.FOCUSED) != 0) {
201 style_context.render_focus (cr,
202 cell_area.x + x_offset,
203 cell_area.y + y_offset,
204 focus_rect_width,
205 focus_rect_height);
206+ }
207 }
208
209 private void get_offsets (Gdk.Rectangle cell_area,
210 int width,
211 int height,
212- float x,
213 out int x_offset,
214 out int y_offset) {
215- x_offset = (int)(x * (cell_area.width - width - 2 * (int)xpad));
216- x_offset = int.max (x_offset, 0);
217-
218- y_offset = (int)(yalign * (cell_area.height - height - 2 * (int)ypad));
219- y_offset = int.max (y_offset, 0);
220+
221+ if (widget.get_direction () == Gtk.TextDirection.RTL) {
222+ x_offset = (int)((1.0f - xalign) * (cell_area.width - width));
223+ x_offset -= (int)xpad;
224+ } else {
225+ x_offset = (int)(xalign * (cell_area.width - width));
226+ x_offset += (int)xpad;
227+ }
228+
229+ y_offset = (int)(yalign * (cell_area.height - height));
230 }
231 }
232 }
233
234=== modified file 'src/View/AbstractDirectoryView.vala'
235--- src/View/AbstractDirectoryView.vala 2017-06-06 09:00:33 +0000
236+++ src/View/AbstractDirectoryView.vala 2017-06-13 16:37:16 +0000
237@@ -352,7 +352,6 @@
238
239 protected virtual void set_up_name_renderer () {
240 name_renderer.editable = false;
241- name_renderer.follow_state = true;
242 name_renderer.edited.connect (on_name_edited);
243 name_renderer.editing_canceled.connect (on_name_editing_canceled);
244 name_renderer.editing_started.connect (on_name_editing_started);
245
246=== modified file 'src/View/IconView.vala'
247--- src/View/IconView.vala 2017-06-05 14:48:38 +0000
248+++ src/View/IconView.vala 2017-06-13 16:37:16 +0000
249@@ -34,6 +34,7 @@
250 tree.set_selection_mode (Gtk.SelectionMode.MULTIPLE);
251 tree.set_columns (-1);
252 tree.set_reorderable (false);
253+ tree.set_item_padding (3);
254
255 name_renderer = new Marlin.TextRenderer (Marlin.ViewMode.ICON);
256 set_up_name_renderer ();
257@@ -106,15 +107,16 @@
258 }
259
260 public override void change_zoom_level () {
261+ int spacing = (int)((double)icon_size * (0.3 - zoom_level * 0.03));
262+ int item_width = (int)((double)icon_size * (2.5 - zoom_level * 0.2));
263 if (tree != null) {
264- tree.set_column_spacing ((int)((double)icon_size * (0.3 - zoom_level * 0.03)));
265- tree.set_item_width ((int)((double)icon_size * (2.5 - zoom_level * 0.2)));
266-
267- name_renderer.set_property ("wrap-width", tree.get_item_width ());
268- name_renderer.set_property ("zoom-level", zoom_level);
269-
270- base.change_zoom_level ();
271+ tree.set_column_spacing (spacing);
272+ tree.set_item_width (item_width);
273 }
274+ name_renderer.item_width = item_width;
275+ name_renderer.set_property ("zoom-level", zoom_level);
276+
277+ base.change_zoom_level ();
278 }
279
280 public override GLib.List<Gtk.TreePath> get_selected_paths () {

Subscribers

People subscribed via source and target branches

to all changes: