Merge lp:~haggai-eran/unity/rtl-menu-popup into lp:unity

Proposed by Haggai Eran
Status: Rejected
Rejected by: Stephen M. Webb
Proposed branch: lp:~haggai-eran/unity/rtl-menu-popup
Merge into: lp:unity
Diff against target: 92 lines (+44/-2)
3 files modified
plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp (+8/-2)
plugins/unityshell/src/unityshell.cpp (+2/-0)
services/panel-service.c (+34/-0)
To merge this branch: bzr merge lp:~haggai-eran/unity/rtl-menu-popup
Reviewer Review Type Date Requested Status
Alex Launi (community) Needs Fixing
Neil J. Patel Pending
Review via email: mp+71541@code.launchpad.net

Description of the change

Hi,

These patches should make the menus in the unity menu bar and in the indicators open to the left when using a right-to-left locale. In a right-to-left setting, the menus should open that way so that they are aligned to the right with the label of the menu or the indicator icon.

The patches change the unity service to show menus to the left of the coordinates given when using an RTL locale. It also changes unity panel to send the coordinates of the bottom right corner of the indicator label, instead of the bottom left. A similar patch was proposed for unity-2d at lp:~haggai-eran/unity-2d/4.0-rtl.

The branch depends on some changes I've proposed to the nux library, at lp:~haggai-eran/nux/rtl.

To post a comment you must log in.
Revision history for this message
Alex Launi (alexlauni) wrote :

Please add tests that prove this behaves correctly.

review: Needs Fixing
Revision history for this message
Haggai Eran (haggai-eran) wrote :

Hi Alex,

Thanks for your comment. I'm wondering what tests would be suitable. Most of the changes in this branch are visual: opening the menu to the left, instead of to the right. One change that seems more suitable to automated testing is the keyboard navigation change, which makes the right key navigate to the previous menu, instead of to the next menu. I could add a test that checks this. Should that test be under tests/unit/TestPanelService.cpp?

Revision history for this message
Alex Launi (alexlauni) wrote :

Sounds right

Revision history for this message
Stephen M. Webb (bregma) wrote :

Although this functional change is desirable, the merge proposal has grown stale.

I'm rejecting it for now. Please re-propose if you are still actively proposing it.

Revision history for this message
Haggai Eran (haggai-eran) wrote :

That's fine. Without the nux RTL patches these patches won't work anyway.

I don't have much time these days to work on updating these patches. If I ever will I'll re-propose.

Unmerged revisions

1374. By Haggai Eran

Correct navigation in the menu when using keyboard on a right-to-left locale.

1373. By Haggai Eran

Open popup menus from the bottom-right point when using a right-to-left locale.

1372. By Haggai Eran

Align popup menus to the left when in right-to-left locale.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp'
--- plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp 2011-08-03 11:37:28 +0000
+++ plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp 2011-08-15 10:35:29 +0000
@@ -97,7 +97,10 @@
97 (proxy_->image_visible() && proxy_->image_sensitive())) &&97 (proxy_->image_visible() && proxy_->image_sensitive())) &&
98 nux::GetEventButton(button_flags) != 2)98 nux::GetEventButton(button_flags) != 2)
99 {99 {
100 proxy_->ShowMenu(GetAbsoluteX() + 1, //cairo translation100 int left = GetAbsoluteX() + 1; //cairo translation
101 if (GetDirection() == nux::RightToLeft)
102 left = left + GetGeometry().width;
103 proxy_->ShowMenu(left,
101 GetAbsoluteY() + PANEL_HEIGHT,104 GetAbsoluteY() + PANEL_HEIGHT,
102 time(NULL),105 time(NULL),
103 nux::GetEventButton(button_flags));106 nux::GetEventButton(button_flags));
@@ -140,7 +143,10 @@
140143
141void PanelIndicatorObjectEntryView::Activate()144void PanelIndicatorObjectEntryView::Activate()
142{145{
143 proxy_->ShowMenu(GetAbsoluteGeometry().x + 1, //cairo translation FIXME: Make this into one function146 int left = GetAbsoluteX() + 1; //cairo translation FIXME: Make this into one function
147 if (GetDirection() == nux::RightToLeft)
148 left = left + GetGeometry().width;
149 proxy_->ShowMenu(left,
144 GetAbsoluteGeometry().y + PANEL_HEIGHT,150 GetAbsoluteGeometry().y + PANEL_HEIGHT,
145 time(NULL),151 time(NULL),
146 1);152 1);
147153
=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp 2011-08-11 07:58:12 +0000
+++ plugins/unityshell/src/unityshell.cpp 2011-08-15 10:35:29 +0000
@@ -137,6 +137,8 @@
137 StartupNotifyService::Default()->SetSnDisplay(screen->snDisplay(), screen->screenNum());137 StartupNotifyService::Default()->SetSnDisplay(screen->snDisplay(), screen->screenNum());
138138
139 nux::NuxInitialize(0);139 nux::NuxInitialize(0);
140 nux::SetDefaultDirection(gtk_widget_get_default_direction() == GTK_TEXT_DIR_RTL ?
141 nux::RightToLeft : nux::LeftToRight);
140 wt = nux::CreateFromForeignWindow(cScreen->output(),142 wt = nux::CreateFromForeignWindow(cScreen->output(),
141 glXGetCurrentContext(),143 glXGetCurrentContext(),
142 &UnityScreen::initUnity,144 &UnityScreen::initUnity,
143145
=== modified file 'services/panel-service.c'
--- services/panel-service.c 2011-07-29 07:15:54 +0000
+++ services/panel-service.c 2011-08-15 10:35:29 +0000
@@ -784,6 +784,25 @@
784784
785 *x = priv->last_x;785 *x = priv->last_x;
786 *y = priv->last_y;786 *y = priv->last_y;
787
788 if (gtk_widget_get_direction(GTK_WIDGET(menu)) == GTK_TEXT_DIR_RTL)
789 {
790 GtkAllocation size;
791 GtkBorder padding;
792
793 gtk_widget_get_allocation(GTK_WIDGET(menu), &size);
794
795 GtkStyleContext *context;
796 GtkStateFlags state;
797
798 context = gtk_widget_get_style_context (GTK_WIDGET(menu));
799 state = gtk_widget_get_state_flags (GTK_WIDGET(menu));
800
801 gtk_style_context_get_padding (context, state, &padding);
802
803 *x += padding.left - size.width + 1;
804 }
805
787 *push = TRUE;806 *push = TRUE;
788}807}
789808
@@ -1009,6 +1028,21 @@
1009 || direction == GTK_MENU_DIR_PREV)1028 || direction == GTK_MENU_DIR_PREV)
1010 return;1029 return;
10111030
1031 // Switch directions for RTL.
1032 if (gtk_widget_get_direction(GTK_WIDGET(menu)) == GTK_TEXT_DIR_RTL)
1033 {
1034 switch (direction)
1035 {
1036 case GTK_MENU_DIR_CHILD:
1037 direction = GTK_MENU_DIR_PARENT;
1038 break;
1039 case GTK_MENU_DIR_PARENT:
1040 direction = GTK_MENU_DIR_CHILD;
1041 break;
1042 default: ;
1043 }
1044 }
1045
1012 /* We don't want to distrupt going into submenus */1046 /* We don't want to distrupt going into submenus */
1013 if (direction == GTK_MENU_DIR_CHILD)1047 if (direction == GTK_MENU_DIR_CHILD)
1014 {1048 {