Merge lp:~tigrangab/slingshot/rtl_fix into lp:~elementary-pantheon/slingshot/trunk

Proposed by Tigran Gabrielyan
Status: Rejected
Rejected by: Cody Garver
Proposed branch: lp:~tigrangab/slingshot/rtl_fix
Merge into: lp:~elementary-pantheon/slingshot/trunk
Diff against target: 239 lines (+76/-51)
1 file modified
src/SlingshotView.vala (+76/-51)
To merge this branch: bzr merge lp:~tigrangab/slingshot/rtl_fix
Reviewer Review Type Date Requested Status
David Gomes (community) Needs Fixing
Review via email: mp+184029@code.launchpad.net

Description of the change

Needs this granite branch to work correctly: https://code.launchpad.net/~tigrangab/granite/popover_fix/+merge/184027 There's a bug in arrow placement if it needs to be top right.

To post a comment you must log in.
Revision history for this message
Andrea Basso (voluntatefaber) wrote :

I'm afraid that's really not enoguh, RTL goes way deeper than that, whereas your patch could solve the positioning problem, things like page-scrolling and keyboard shortcuts are still hardcoded as in a LTR prospective.

Revision history for this message
Tigran Gabrielyan (tigrangab) wrote :

Wasn't aware there were more issues. I'll install a RTL language to actually test it.

Revision history for this message
Tigran Gabrielyan (tigrangab) wrote :

Can you check it now and make sure I didn't miss anything and it works correctly?

Revision history for this message
Andrea Basso (voluntatefaber) wrote :

I'm currently not at home, so I unfortunately cannot test it.

Just looking at the code, though, are you sure keyboard shortcuts works fine, especially in turning pages?

Revision history for this message
Tigran Gabrielyan (tigrangab) wrote :

No :) I was doing that at 2AM, I'm working on it.

Revision history for this message
Tigran Gabrielyan (tigrangab) wrote :

Shouldn't the first page in RTL still be full?

https://launchpadlibrarian.net/147514797/applications.png That's the first page but only half are full. Is that intended or another RTL bug in slingshot?

Revision history for this message
Andrea Basso (voluntatefaber) wrote :

I'd say that's definitely a bug. And also Slingshot position, I guess you
appear in top-left corner...

Revision history for this message
Tigran Gabrielyan (tigrangab) wrote :

Please test this when you get a chance. If something doesn't work as expected, please let me know how it should work. Thanks.

Revision history for this message
Andrea Basso (voluntatefaber) wrote :

I'll do that as soon as I get back :)
Il giorno 06/set/2013 08:59, "Tigran Gabrielyan" <email address hidden> ha
scritto:

> Please test this when you get a chance. If something doesn't work as
> expected, please let me know how it should work. Thanks.
> --
> https://code.launchpad.net/~tigrangab/slingshot/rtl_fix/+merge/184029
> Your team elementary Pantheon team is requested to review the proposed
> merge of lp:~tigrangab/slingshot/rtl_fix into lp:slingshot.
>

Revision history for this message
David Gomes (davidgomes) wrote :

I need someone to test this or teach me how to. Should I change my language to Hebrew or something?

Revision history for this message
Tigran Gabrielyan (tigrangab) wrote :

I left a note on the PopOver merge request about it. Add setDefaultDirection(Gtk.TextDirection.RTL); in the constructor of SlingshotView.vala. You'll still have English but it will behave RTL-ly

Revision history for this message
David Gomes (davidgomes) wrote :

Remove diff line 236, 73. Add newline after diff line 49. I tested and it worked, good job.

review: Needs Fixing
Revision history for this message
David Gomes (davidgomes) wrote :

This is DEPENDENT on a Granite bug fix which HAS NOT been released on Luna yet. So we can only merge this to stable PPA when Granite gets a push to the stable PPA with that bug fix.

Revision history for this message
Sergey "Shnatsel" Davidoff (shnatsel) wrote :

Can we merge this now? At least as an Isis-only fix?

Unmerged revisions

381. By Tigran Gabrielyan

Fix RTL key navigation

380. By Tigran Gabrielyan

Fix keyboard and scroll navigation for RTL

379. By Tigran Gabrielyan

Show slingshot on right side of screen if language is RTL

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/SlingshotView.vala'
2--- src/SlingshotView.vala 2013-09-04 19:08:08 +0000
3+++ src/SlingshotView.vala 2013-09-06 06:54:15 +0000
4@@ -61,6 +61,9 @@
5 private int search_view_position = 0;
6 private Modality modality;
7
8+ // Used for RTL support
9+ private int direction = 1;
10+
11 // Sizes
12 public int columns {
13 get {
14@@ -111,6 +114,10 @@
15 setup_ui ();
16 connect_signals ();
17
18+ if (get_default_direction () == Gtk.TextDirection.RTL) {
19+ direction = -1;
20+ }
21+
22 debug ("Apps loaded");
23
24 }
25@@ -281,17 +288,19 @@
26
27 }
28
29- private void reposition (bool show=true) {
30-
31+ private void reposition (bool show = true) {
32 debug("Repositioning");
33-
34+
35+ var direction = get_default_direction ();
36 Gdk.Rectangle monitor_dimensions, app_launcher_pos;
37 screen.get_monitor_geometry (this.screen.get_primary_monitor(), out monitor_dimensions);
38- app_launcher_pos = Gdk.Rectangle () { x = monitor_dimensions.x,
39- y = monitor_dimensions.y,
40- width = 100,
41- height = 30
42- };
43+
44+ app_launcher_pos = Gdk.Rectangle () {
45+ x = (direction == Gtk.TextDirection.RTL) ? monitor_dimensions.width : monitor_dimensions.x,
46+ y = monitor_dimensions.y,
47+ width = 100,
48+ height = 30
49+ };
50 move_to_rect (app_launcher_pos, show);
51 }
52
53@@ -391,15 +400,17 @@
54 case "Left":
55 if (modality == Modality.NORMAL_VIEW) {
56 if (event.state == Gdk.ModifierType.SHIFT_MASK) // Shift + Left
57- page_switcher.set_active (page_switcher.active - 1);
58+ page_switcher.set_active (page_switcher.active - direction);
59 else
60- normal_move_focus (-1, 0);
61+ normal_move_focus (-direction, 0);
62 } else if (modality == Modality.CATEGORY_VIEW) {
63 if (event.state == Gdk.ModifierType.SHIFT_MASK) // Shift + Left
64- category_view.switcher.set_active (category_view.switcher.active - 1);
65- else if (!searchbar.has_focus) {//the user has already selected an AppEntry
66- category_move_focus (-1, 0);
67- }
68+ category_view.switcher.set_active (category_view.switcher.active - direction);
69+ else if (searchbar.has_focus && direction == -1) // there's no AppEntry selected, the user is switching category
70+ top_left_focus ();
71+ else
72+ category_move_focus (-direction, 0);
73+
74 } else
75 return base.key_press_event (event);
76 break;
77@@ -407,16 +418,16 @@
78 case "Right":
79 if (modality == Modality.NORMAL_VIEW) {
80 if (event.state == Gdk.ModifierType.SHIFT_MASK) // Shift + Right
81- page_switcher.set_active (page_switcher.active + 1);
82+ page_switcher.set_active (page_switcher.active + direction);
83 else
84- normal_move_focus (+1, 0);
85+ normal_move_focus (direction, 0);
86 } else if (modality == Modality.CATEGORY_VIEW) {
87 if (event.state == Gdk.ModifierType.SHIFT_MASK) // Shift + Right
88- category_view.switcher.set_active (category_view.switcher.active + 1);
89- else if (searchbar.has_focus) // there's no AppEntry selected, the user is switching category
90+ category_view.switcher.set_active (category_view.switcher.active + direction);
91+ else if (searchbar.has_focus && direction == 1) // there's no AppEntry selected, the user is switching category
92 top_left_focus ();
93 else //the user has already selected an AppEntry
94- category_move_focus (+1, 0);
95+ category_move_focus (direction, 0);
96 } else {
97 return base.key_press_event (event);
98 }
99@@ -463,7 +474,7 @@
100
101 case "Page_Up":
102 if (modality == Modality.NORMAL_VIEW) {
103- page_switcher.set_active (page_switcher.active - 1);
104+ page_switcher.set_active (page_switcher.active - direction);
105 if (page_switcher.active != 0) // we don't wanna lose focus if we don't actually change page
106 searchbar.grab_focus (); // this is because otherwise focus isn't the current page
107 } else if (modality == Modality.CATEGORY_VIEW) {
108@@ -474,7 +485,7 @@
109
110 case "Page_Down":
111 if (modality == Modality.NORMAL_VIEW) {
112- page_switcher.set_active (page_switcher.active + 1);
113+ page_switcher.set_active (page_switcher.active + direction);
114 if (page_switcher.active != grid_view.get_n_pages () - 1) // we don't wanna lose focus if we don't actually change page
115 searchbar.grab_focus (); //this is because otherwise focus isn't the current page
116 } else if (modality == Modality.CATEGORY_VIEW) {
117@@ -542,32 +553,33 @@
118
119 }
120
121+ private void scroll_page (int direction) {
122+ if (modality == Modality.NORMAL_VIEW) {
123+ page_switcher.set_active (page_switcher.active + direction);
124+ } else if (modality == Modality.SEARCH_VIEW) {
125+ if (direction == this.direction) {
126+ search_view_up ();
127+ } else {
128+ search_view_down ();
129+ }
130+ } else {
131+ category_view.switcher.set_active (category_view.switcher.active + direction);
132+ }
133+ }
134+
135 public override bool scroll_event (EventScroll event) {
136-
137 switch (event.direction.to_string ()) {
138 case "GDK_SCROLL_UP":
139 case "GDK_SCROLL_LEFT":
140- if (modality == Modality.NORMAL_VIEW)
141- page_switcher.set_active (page_switcher.active - 1);
142- else if (modality == Modality.SEARCH_VIEW)
143- search_view_up ();
144- else
145- category_view.switcher.set_active (category_view.switcher.active - 1);
146+ scroll_page (-direction);
147 break;
148 case "GDK_SCROLL_DOWN":
149 case "GDK_SCROLL_RIGHT":
150- if (modality == Modality.NORMAL_VIEW)
151- page_switcher.set_active (page_switcher.active + 1);
152- else if (modality == Modality.SEARCH_VIEW)
153- search_view_down ();
154- else
155- category_view.switcher.set_active (category_view.switcher.active + 1);
156+ scroll_page (direction);
157 break;
158-
159 }
160
161 return false;
162-
163 }
164
165 public void show_slingshot () {
166@@ -590,34 +602,40 @@
167 Wnck.Screen.get_default ().force_update ();
168 if (w != null)
169 w.activate (Gdk.x11_get_server_time (this.get_window ()));
170- }
171+ }
172
173 private void move_page (int step) {
174-
175 debug ("Moving: step = " + step.to_string ());
176-
177+
178 if (step == 0)
179 return;
180- if (step < 0 && current_position >= 0) //Left border
181- return;
182- if (step > 0 && (-current_position) >= ((grid_view.get_n_pages () - 1) * grid_view.get_page_columns () * 130)) //Right border
183- return;
184-
185+
186+ int max_position = (grid_view.get_n_pages () - 1) * grid_view.get_page_columns () * 130;
187+
188+ if (step < 0) {
189+ if ((direction == 1 && current_position >= 0) || (direction == -1 && current_position <= -max_position)) {
190+ return;
191+ }
192+ } else {
193+ if ((direction == 1 && current_position <= -max_position) || (direction == -1 && current_position >= 0)) {
194+ return;
195+ }
196+ }
197+
198 int count = 0;
199- int increment = -step*130*columns/10;
200- Timeout.add (30/columns, () => {
201+ int increment = -step * 130 * columns / 10 * direction;
202
203+ Timeout.add (30 / columns, () => {
204 if (count >= 10) {
205- current_position += -step*130*columns - 10*increment; //We adjust to end of the page
206+ current_position += -step * 130 * columns - 10 * increment * direction; //We adjust to end of the page
207 view_manager.move (grid_view, current_position, 0);
208 return false;
209 }
210-
211+
212 current_position += increment;
213 view_manager.move (grid_view, current_position, 0);
214 count++;
215 return true;
216-
217 }, Priority.DEFAULT_IDLE);
218 }
219
220@@ -744,10 +762,17 @@
221 grid_view.append (app_entry);
222 app_entry.show_all ();
223 }
224-
225- view_manager.move (grid_view, 0, 0);
226+
227 current_position = 0;
228
229+ if (get_default_direction () == Gtk.TextDirection.RTL) {
230+ int width;
231+ view_manager.get_size_request (out width, null);
232+ current_position = (grid_view.get_n_pages () - 1) * width * -1;
233+ }
234+
235+ view_manager.move (grid_view, current_position, 0);
236+
237 }
238
239 private void read_settings (bool first_start = false, bool check_columns = true, bool check_rows = true) {

Subscribers

People subscribed via source and target branches