Merge lp:~sil2100/unity-2d/trunk.lp839628 into lp:unity-2d

Proposed by Łukasz Zemczak on 2011-09-27
Status: Merged
Approved by: Alberto Mardegan on 2011-09-29
Approved revision: 747
Merged at revision: 745
Proposed branch: lp:~sil2100/unity-2d/trunk.lp839628
Merge into: lp:unity-2d
Diff against target: 314 lines (+117/-5)
12 files modified
libunity-2d-private/src/indicatorswidget.cpp (+4/-0)
libunity-2d-private/src/indicatorswidget.h (+2/-0)
libunity-2d-private/src/unity2dpanel.h (+3/-0)
panel/app/panelmanager.cpp (+24/-1)
panel/app/panelmanager.h (+1/-0)
panel/applets/appname/appnameapplet.cpp (+31/-0)
panel/applets/appname/appnameapplet.h (+1/-0)
panel/applets/appname/menubarwidget.cpp (+14/-4)
panel/applets/appname/menubarwidget.h (+2/-0)
panel/applets/indicator/CMakeLists.txt (+2/-0)
panel/applets/indicator/indicatorapplet.cpp (+31/-0)
panel/applets/indicator/indicatorapplet.h (+2/-0)
To merge this branch: bzr merge lp:~sil2100/unity-2d/trunk.lp839628
Reviewer Review Type Date Requested Status
Alberto Mardegan (community) 2011-09-27 Approve on 2011-09-29
Review via email: mp+77192@code.launchpad.net

Commit message

[panel] Handle the F10 key press, starting the panel menu navigation when possible.

Fixes: https://bugs.launchpad.net/unity/+bug/839628
Fixes: https://bugs.launchpad.net/unity-2d/+bug/853766

Description of the change

Fixed the https://bugs.launchpad.net/unity/+bug/839628 missing F10 handling issue in a similar way as it is done in Unity-3D. We are registering the F10 hotkey and, on key press, popping up the first menu of the application. If no application is visible, we pop up the first indicator menu.

To post a comment you must log in.
Łukasz Zemczak (sil2100) wrote :

I fixed some issues that were left over after the merge/rebase. I hope everything is alright now!

lp:~sil2100/unity-2d/trunk.lp839628 updated on 2011-09-27
745. By Łukasz Zemczak on 2011-09-27

[panel] Fixed some merge leftovers.

Alberto Mardegan (mardy) wrote :

Thanks Łukasz!

I like it more than before, but there are a couple of things which are IMHO a bit ugly. Having a separate class helps avoid duplication of code, but in this specific case the little amount duplicated code is not enough to justify the onVisible variable and the protected beforeOpen() method, which make the code much harder to understand.
Better keep the small code duplication, which is short and simple.

Style issue: I find isUserVisibleApp quite hard to understand without reading the code which assigns to it; maybe "isActiveApplicationVisible" is more clear?

review: Needs Fixing
lp:~sil2100/unity-2d/trunk.lp839628 updated on 2011-09-28
746. By Łukasz Zemczak on 2011-09-28

[panel][lib] Reverting back to the approach from r743.
More code duplication, but better looking.

Łukasz Zemczak (sil2100) wrote :

> Thanks Łukasz!
>
> I like it more than before, but there are a couple of things which are IMHO a
> bit ugly. Having a separate class helps avoid duplication of code, but in this
> specific case the little amount duplicated code is not enough to justify the
> onVisible variable and the protected beforeOpen() method, which make the code
> much harder to understand.
> Better keep the small code duplication, which is short and simple.
>
> Style issue: I find isUserVisibleApp quite hard to understand without reading
> the code which assigns to it; maybe "isActiveApplicationVisible" is more
> clear?

Thanks! I've reverted to the old fix. How does it look now?

Alberto Mardegan (mardy) wrote :

Looks great, Łukasz!

Just a couple of things:
- Remove the addition of an empty line in appnameapplet.cpp:142

- One little change that might help keep the code simple: remove the entries() method from IndicatorsWidget, and instead add a method "showFirstMenu()" which you can call from both filters.

- Is the setOpened() method really needed? Why doesn't the indicator emit the entryActivated() signal in this case?

review: Needs Fixing
Łukasz Zemczak (sil2100) wrote :

> Looks great, Łukasz!
>
> Just a couple of things:
> - Remove the addition of an empty line in appnameapplet.cpp:142

Will do that!

> - One little change that might help keep the code simple: remove the entries()
> method from IndicatorsWidget, and instead add a method "showFirstMenu()" which
> you can call from both filters.

hm, this might be a problem. Because indeed, the entries() method is called in both filters, but in the IndicatorApplet it is called from an IndicatorsWidget object, where in AppNameApplet it is called from an MenuBarWidget object - where both these classes are just derivatives of QWidget. So just adding a showFirstMenu() instead would not really decrease the code size, since I would have to add the same method doing the same in 2 classes. In fact, I had a method like this in the beginning (when I only handled application menus), but dropped it because such a method has potentially only one use - to show the menu. Where an entries() method can be used in the future for other (yet unplanned) things.

I could, of course, create a class - for instance, WidgetWithIndicators, that would simply be a QWidget with the showFirstMenu()/entries() method, but I think that would be unnecessary bloating of the namespace.

> - Is the setOpened() method really needed? Why doesn't the indicator emit the
> entryActivated() signal in this case?

I just noticed that without it, the menubar acts strangely i.e. the application menu appears in a random place. I think it's because of the fact, that the updateWidgets() method that shows the menu bar and ensures that it's properly adjusted gets always called before the entryActivated() signal. And we need the m_isOpened to be true before the updateWidgets() call, otherwise the behavior is wrong.

unity-2d-panel: [DEBUG] eventFilter()
unity-2d-panel: [DEBUG] updateWidgets()
unity-2d-panel: [DEBUG] entryActivated()

So I though that instead of fighting with this, it would be best just to create that method and call it beforehand.

lp:~sil2100/unity-2d/trunk.lp839628 updated on 2011-09-28
747. By Łukasz Zemczak on 2011-09-28

[panel] Whitespace fixes.

Łukasz Zemczak (sil2100) wrote :

I propose using this solution for now at least. Can't come up for a nicer scheme for setOpened()...

Łukasz Zemczak (sil2100) wrote :
Florian Boucault (fboucault) wrote :

Since having setOpened does not seem to have any negative functional impact and that the feature is very important for accessibility, and that the freeze is tonight, I suggest we merge it now with a FIXME.

Alberto Mardegan (mardy) wrote :

OK, let's get it merged, and continue looking into making it better.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libunity-2d-private/src/indicatorswidget.cpp'
2--- libunity-2d-private/src/indicatorswidget.cpp 2011-09-13 17:55:17 +0000
3+++ libunity-2d-private/src/indicatorswidget.cpp 2011-09-28 13:12:26 +0000
4@@ -114,5 +114,9 @@
5 }
6 }
7
8+QList<IndicatorEntryWidget*> IndicatorsWidget::entries() const
9+{
10+ return m_entries;
11+}
12
13 #include "indicatorswidget.moc"
14
15=== modified file 'libunity-2d-private/src/indicatorswidget.h'
16--- libunity-2d-private/src/indicatorswidget.h 2011-09-13 17:55:17 +0000
17+++ libunity-2d-private/src/indicatorswidget.h 2011-09-28 13:12:26 +0000
18@@ -50,6 +50,8 @@
19 void addIndicator(const unity::indicator::Indicator::Ptr& indicator);
20 void removeIndicator(const unity::indicator::Indicator::Ptr& indicator);
21
22+ QList<IndicatorEntryWidget*> entries() const;
23+
24 private:
25 QHBoxLayout* m_layout;
26 IndicatorsManager* m_indicatorsManager;
27
28=== modified file 'libunity-2d-private/src/unity2dpanel.h'
29--- libunity-2d-private/src/unity2dpanel.h 2011-08-22 10:38:32 +0000
30+++ libunity-2d-private/src/unity2dpanel.h 2011-09-28 13:12:26 +0000
31@@ -27,6 +27,7 @@
32
33 // Qt
34 #include <QWidget>
35+#include <QEvent>
36
37 struct Unity2dPanelPrivate;
38 class Unity2dPanel : public QWidget
39@@ -51,6 +52,8 @@
40 TopEdge
41 };
42
43+ static const QEvent::Type SHOW_FIRST_MENU_EVENT = QEvent::User;
44+
45 Unity2dPanel(bool requiresTransparency = false, QWidget* parent = 0);
46 ~Unity2dPanel();
47
48
49=== modified file 'panel/app/panelmanager.cpp'
50--- panel/app/panelmanager.cpp 2011-09-01 08:39:55 +0000
51+++ panel/app/panelmanager.cpp 2011-09-28 13:12:26 +0000
52@@ -26,6 +26,8 @@
53 #include <config.h>
54 #include <panelstyle.h>
55 #include <indicatorsmanager.h>
56+#include <hotkeymonitor.h>
57+#include <hotkey.h>
58
59 // Unity
60 #include <unity2dpanel.h>
61@@ -124,6 +126,11 @@
62 panel->move(desktop->screenGeometry(i).topLeft());
63 }
64 connect(desktop, SIGNAL(screenCountChanged(int)), SLOT(onScreenCountChanged(int)));
65+
66+ /* A F10 keypress opens the first menu of the visible application or of the first
67+ indicator on the panel */
68+ Hotkey* F10 = HotkeyMonitor::instance().getHotkeyFor(Qt::Key_F10, Qt::NoModifier);
69+ connect(F10, SIGNAL(released()), SLOT(onF10Pressed()));
70 }
71
72 PanelManager::~PanelManager()
73@@ -161,7 +168,7 @@
74 << "installed plugin providing it.";
75 } else {
76 if (screen == leftmost || !onlyLeftmost) {
77- QWidget *applet = provider->createApplet(panel);
78+ QWidget* applet = provider->createApplet(panel);
79 if (applet == 0) {
80 qWarning() << "The panel applet plugin for" << appletName
81 << "did not return a valid plugin.";
82@@ -218,5 +225,21 @@
83 }
84 }
85
86+void PanelManager::onF10Pressed()
87+{
88+ QDesktopWidget* desktop = QApplication::desktop();
89+ int screen = desktop->screenNumber(QCursor::pos());
90+ Unity2dPanel* panel;
91+
92+ if (screen >= m_panels.size()) {
93+ return;
94+ }
95+ panel = m_panels[screen];
96+ if (panel != NULL) {
97+ QEvent* event = new QEvent(Unity2dPanel::SHOW_FIRST_MENU_EVENT);
98+ QCoreApplication::postEvent(panel, event);
99+ }
100+}
101+
102 #include "panelmanager.moc"
103
104
105=== modified file 'panel/app/panelmanager.h'
106--- panel/app/panelmanager.h 2011-08-30 10:41:50 +0000
107+++ panel/app/panelmanager.h 2011-09-28 13:12:26 +0000
108@@ -49,6 +49,7 @@
109
110 private Q_SLOTS:
111 void onScreenCountChanged(int newCount);
112+ void onF10Pressed();
113 };
114
115 #endif // PanelManager_H
116
117=== modified file 'panel/applets/appname/appnameapplet.cpp'
118--- panel/applets/appname/appnameapplet.cpp 2011-09-27 10:53:22 +0000
119+++ panel/applets/appname/appnameapplet.cpp 2011-09-28 13:12:26 +0000
120@@ -33,6 +33,9 @@
121 #include <debug_p.h>
122 #include <keyboardmodifiersmonitor.h>
123 #include <launcherclient.h>
124+#include <hotkey.h>
125+#include <hotkeymonitor.h>
126+#include <indicatorentrywidget.h>
127
128 // Bamf
129 #include <bamf-application.h>
130@@ -214,6 +217,10 @@
131 layout->addWidget(d->m_label);
132 layout->addWidget(d->m_menuBarWidget);
133
134+ if (panel != NULL) {
135+ panel->installEventFilter(this);
136+ }
137+
138 updateWidgets();
139 }
140
141@@ -311,4 +318,28 @@
142 }
143 }
144
145+bool AppNameApplet::eventFilter(QObject* watched, QEvent* event)
146+{
147+ if (event->type() == Unity2dPanel::SHOW_FIRST_MENU_EVENT) {
148+ BamfApplication* app = BamfMatcher::get_default().active_application();
149+ bool isActiveAppVisible = app ? app->user_visible() : false;
150+ if (isActiveAppVisible) {
151+ d->m_menuBarWidget->setOpened(true);
152+
153+ QList<IndicatorEntryWidget*> list = d->m_menuBarWidget->entries();
154+ if (!list.isEmpty()) {
155+ IndicatorEntryWidget* el = list.first();
156+ if (el != NULL) {
157+ el->showMenu(Qt::NoButton);
158+ }
159+ }
160+ return true;
161+ } else {
162+ return false;
163+ }
164+ } else {
165+ return QWidget::eventFilter(watched, event);
166+ }
167+}
168+
169 #include "appnameapplet.moc"
170
171=== modified file 'panel/applets/appname/appnameapplet.h'
172--- panel/applets/appname/appnameapplet.h 2011-09-27 10:53:22 +0000
173+++ panel/applets/appname/appnameapplet.h 2011-09-28 13:12:26 +0000
174@@ -44,6 +44,7 @@
175 void mousePressEvent(QMouseEvent*);
176 void mouseReleaseEvent(QMouseEvent*);
177 void mouseMoveEvent(QMouseEvent*);
178+ bool eventFilter(QObject*, QEvent*);
179
180 private Q_SLOTS:
181 void updateWidgets();
182
183=== modified file 'panel/applets/appname/menubarwidget.cpp'
184--- panel/applets/appname/menubarwidget.cpp 2011-09-19 08:52:51 +0000
185+++ panel/applets/appname/menubarwidget.cpp 2011-09-28 13:12:26 +0000
186@@ -72,6 +72,19 @@
187 return m_isOpened;
188 }
189
190+void MenuBarWidget::setOpened(bool opened)
191+{
192+ if (m_isOpened != opened) {
193+ m_isOpened = opened;
194+ isOpenedChanged();
195+ }
196+}
197+
198+QList<IndicatorEntryWidget*> MenuBarWidget::entries() const
199+{
200+ return m_widgetList;
201+}
202+
203 void MenuBarWidget::onObjectAdded(const unity::indicator::Indicator::Ptr& indicator)
204 {
205 QString name = QString::fromStdString(indicator->name());
206@@ -159,10 +172,7 @@
207 }
208 }
209 }
210- if (m_isOpened != isOpened) {
211- m_isOpened = isOpened;
212- isOpenedChanged();
213- }
214+ setOpened(isOpened);
215 }
216
217 #include "menubarwidget.moc"
218
219=== modified file 'panel/applets/appname/menubarwidget.h'
220--- panel/applets/appname/menubarwidget.h 2011-09-16 14:47:32 +0000
221+++ panel/applets/appname/menubarwidget.h 2011-09-28 13:12:26 +0000
222@@ -45,6 +45,8 @@
223
224 bool isEmpty() const;
225 bool isOpened() const;
226+ void setOpened(bool opened);
227+ QList<IndicatorEntryWidget*> entries() const;
228
229 Q_SIGNALS:
230 void isOpenedChanged();
231
232=== modified file 'panel/applets/indicator/CMakeLists.txt'
233--- panel/applets/indicator/CMakeLists.txt 2011-08-22 09:39:50 +0000
234+++ panel/applets/indicator/CMakeLists.txt 2011-09-28 13:12:26 +0000
235@@ -12,6 +12,7 @@
236 # Dependencies
237 pkg_check_modules(DBUSMENUQT REQUIRED dbusmenu-qt)
238 pkg_check_modules(INDICATOR REQUIRED indicator)
239+pkg_check_modules(QTBAMF REQUIRED libqtbamf)
240
241 # Get indicator dirs from pkgconfig
242 read_pkg_variable(INDICATOR_DIR indicator indicatordir)
243@@ -31,6 +32,7 @@
244 ${GTK_INCLUDE_DIRS}
245 ${INDICATOR_INCLUDE_DIRS}
246 ${CMAKE_CURRENT_BINARY_DIR}
247+ ${QTBAMF_INCLUDE_DIRS}
248 ${UNITYCORE_INCLUDE_DIRS}
249 ${libunity-2d-private_SOURCE_DIR}/src
250 )
251
252=== modified file 'panel/applets/indicator/indicatorapplet.cpp'
253--- panel/applets/indicator/indicatorapplet.cpp 2011-09-09 02:22:24 +0000
254+++ panel/applets/indicator/indicatorapplet.cpp 2011-09-28 13:12:26 +0000
255@@ -26,8 +26,13 @@
256 #include <debug_p.h>
257 #include <indicatorsmanager.h>
258 #include <indicatorswidget.h>
259+#include <indicatorentrywidget.h>
260 #include <unity2dpanel.h>
261
262+// Bamf
263+#include <bamf-application.h>
264+#include <bamf-matcher.h>
265+
266 // Qt
267 #include <QHBoxLayout>
268
269@@ -53,6 +58,32 @@
270
271 m_indicatorsWidget = new IndicatorsWidget(m_indicatorsManager);
272 layout->addWidget(m_indicatorsWidget);
273+
274+ if (panel != NULL) {
275+ panel->installEventFilter(this);
276+ }
277+}
278+
279+bool IndicatorApplet::eventFilter(QObject* watched, QEvent* event)
280+{
281+ if (event->type() == Unity2dPanel::SHOW_FIRST_MENU_EVENT) {
282+ BamfApplication* app = BamfMatcher::get_default().active_application();
283+ bool isActiveAppVisible = app ? app->user_visible() : false;
284+ if (!isActiveAppVisible) {
285+ QList<IndicatorEntryWidget*> list = m_indicatorsWidget->entries();
286+ if (!list.isEmpty()) {
287+ IndicatorEntryWidget* el = list.first();
288+ if (el != NULL) {
289+ el->showMenu(Qt::NoButton);
290+ }
291+ }
292+ return true;
293+ } else {
294+ return false;
295+ }
296+ } else {
297+ return QWidget::eventFilter(watched, event);
298+ }
299 }
300
301 void IndicatorApplet::onObjectAdded(Indicator::Ptr const& indicator)
302
303=== modified file 'panel/applets/indicator/indicatorapplet.h'
304--- panel/applets/indicator/indicatorapplet.h 2011-09-09 02:22:24 +0000
305+++ panel/applets/indicator/indicatorapplet.h 2011-09-28 13:12:26 +0000
306@@ -38,6 +38,8 @@
307 public:
308 IndicatorApplet(Unity2dPanel* panel);
309
310+ bool eventFilter(QObject*, QEvent*);
311+
312 private:
313 Q_DISABLE_COPY(IndicatorApplet)
314 IndicatorsManager* m_indicatorsManager;

Subscribers

People subscribed via source and target branches