Merge lp:~thiago-nsantos/unity-2d/launcher-width into lp:unity-2d

Proposed by Thiago Neves on 2011-10-20
Status: Rejected
Rejected by: Albert Astals Cid on 2013-11-29
Proposed branch: lp:~thiago-nsantos/unity-2d/launcher-width
Merge into: lp:unity-2d
Diff against target: 216 lines (+44/-8) 11 files modified
To merge this branch: bzr merge lp:~thiago-nsantos/unity-2d/launcher-width
Reviewer Review Type Date Requested Status
Albert Astals Cid Disapprove on 2013-11-29
Gerry Boland Needs Fixing on 2011-10-28
Florian Boucault 2011-10-20 Pending
Review via email: mp+79974@code.launchpad.net

Description of the Change

Make Unity 2D launcher width configurable.

To post a comment you must log in.
767. By Thiago Neves on 2011-10-23

Adjust 'places' to be drawed at the right 'left' position.

768. By Thiago Neves on 2011-10-23

Adjust 'spread' to be drawn at the right 'left' position.

Gerry Boland (gerboland) wrote :

Hey Thiago, thanks for the MR!

Running your code straight off (max width of 66), I find that the icons for some apps like Firefox, VirtualBox, QtCreator, Inkscape, Banshee, Skype, are all not being found, and I'm getting these errors:

unity-2d-launcher: [WARNING] QImage GImageUtils::imageForIconString(const QString&, int, GtkIconTheme*): Failed to load icon: "virtualbox"
unity-2d-launcher: [WARNING] QList<QVariant> IconUtilities::getColorsFromIcon(QUrl, QSize) const: Unable to load icon in getColorsFromIcon from QUrl( "image://icons/virtualbox" )
unity-2d-launcher: [CRITICAL] GdkPixbuf: gdk_pixbuf_scale_simple: assertion `dest_width > 0' failed
unity-2d-launcher: [CRITICAL] GLib-GObject: g_object_ref: assertion `G_IS_OBJECT (object)' failed

Also I've a few concerns with some "magic" numbers you're using in the QML. Say if the width of the 1-pixel line on the right of the launcher is widened, we would have to re-adjust all of your numbers to fix layout.

Finally, have you looked into the possibility of updating the launcher dynamically when its dconf configuration is changed, instead of having to kill and restart it?

review: Needs Fixing
Thiago Neves (thiago-nsantos) wrote :

> Hey Thiago, thanks for the MR!

Thanks for reviewing!

> Running your code straight off (max width of 66), I find that the icons for some apps like
> Firefox, VirtualBox, QtCreator, Inkscape, Banshee, Skype, are all not being found, and I'm
> getting these errors:
> unity-2d-launcher: [WARNING] QImage GImageUtils::imageForIconString(const QString&, int,
> GtkIconTheme*): Failed to load icon: "virtualbox"
> unity-2d-launcher: [WARNING] QList<QVariant> IconUtilities::getColorsFromIcon(QUrl, QSize) const:
> Unable to load icon in getColorsFromIcon from QUrl( "image://icons/virtualbox" )
> unity-2d-launcher: [CRITICAL] GdkPixbuf: gdk_pixbuf_scale_simple: assertion `dest_width > 0'
> failed unity-2d-launcher: [CRITICAL] GLib-GObject: g_object_ref: assertion `G_IS_OBJECT (object)'
> failed

I didn't noticed that because I have erased all icons. I'll take a look at this.

> Also I've a few concerns with some "magic" numbers you're using in the QML. Say if the width of
> the 1-pixel line on the right of the launcher is widened, we would have to re-adjust all of your
> numbers to fix layout.

I got those "magic" numbers used in the QML just by subtracting the old values and now subtracting the parametized value from the old differences, ie.: the tileSize old value was 54 and panel width 66 (66 - 54 = 12), and now it is parent.width - 8 (I found that 8 looks better than 12).

> Finally, have you looked into the possibility of updating the launcher dynamically when its dconf
> configuration is changed, instead of having to kill and restart it?
Do, I didn't. But I think it is easy to change. I'll take a look at this too.

I'll work on those points. And I'll change branch Status from Mature to Development :-D.

Gerry Boland (gerboland) wrote :

> I got those "magic" numbers used in the QML just by subtracting the old values
> and now subtracting the parametized value from the old differences, ie.: the
> tileSize old value was 54 and panel width 66 (66 - 54 = 12), and now it is
> parent.width - 8 (I found that 8 looks better than 12).

Yep, understood, but I'll have a look and see if there's a mathematical way to derive these numbers. We need to verify that the launcher looks exactly the same at width=66 as it does before, so care is needed.

> > Finally, have you looked into the possibility of updating the launcher
> dynamically when its dconf
> > configuration is changed, instead of having to kill and restart it?
> Do, I didn't. But I think it is easy to change. I'll take a look at this too.

Great, thanks!

> I'll work on those points. And I'll change branch Status from Mature to
> Development :-D.

Nice work, it's looking good, and lots of people will be happy to see this merged.

Gerry Boland (gerboland) wrote :

I did an comparison of the before & after of the launcher, check it out:

http://imageshack.us/photo/my-images/100/launchercompare.png/

generated with (compare launcher-old.png launcher-new.png launcher-compate.png).

I think that the tiles are 4 pixels too large with your patch, which causes all the red. Check your magic numbers! :)

Thiago Neves (thiago-nsantos) wrote :

It appears that those warning messages aren't related to the icon images not showing. I have reverted LauncherList.qml and now all icons appear ok. But this have created a side effect, 'spread' icon doesn't follow other icons size.

769. By Thiago Neves on 2011-10-29

Icons are not showing for some applications. LauncherList.qml was reverted to its original contents.

Thiago Neves (thiago-nsantos) wrote :

I mean, LauncherList.qml is now subtracting the same difference value as it was originaly.

Albert Astals Cid (aacid) wrote :

Doesn't apply cleanly to lp:unity-2d anymore

review: Needs Fixing
Albert Astals Cid (aacid) wrote :

Cleaning up my review request queue[1], i guess noone cares if i reject this, right?

[1] https://code.launchpad.net/~aacid/+activereviews

review: Disapprove
Thiago Neves (thiago-nsantos) wrote :

Okey :~(

Please, let me know if I can help again.

2013/11/29 Albert Astals Cid <email address hidden>

> The proposal to merge lp:~thiago-nsantos/unity-2d/launcher-width into
> lp:unity-2d has been updated.
>
> Status: Needs review => Rejected
>
> For more details, see:
>
> https://code.launchpad.net/~thiago-nsantos/unity-2d/launcher-width/+merge/79974
> --
>
> https://code.launchpad.net/~thiago-nsantos/unity-2d/launcher-width/+merge/79974
> You are the owner of lp:~thiago-nsantos/unity-2d/launcher-width.
>

Unmerged revisions

769. By Thiago Neves on 2011-10-29

Icons are not showing for some applications. LauncherList.qml was reverted to its original contents.

768. By Thiago Neves on 2011-10-23

Adjust 'spread' to be drawn at the right 'left' position.

767. By Thiago Neves on 2011-10-23

Adjust 'places' to be drawed at the right 'left' position.

766. By Thiago Neves on 2011-10-20

Set minimum width value of 32

765. By Thiago Neves on 2011-10-20

Make launcher width configurable

Preview Diff

1=== modified file 'data/com.canonical.Unity2d.gschema.xml'
2--- data/com.canonical.Unity2d.gschema.xml 2011-09-07 11:51:57 +0000
3+++ data/com.canonical.Unity2d.gschema.xml 2011-10-29 00:34:27 +0000
4@@ -25,6 +25,11 @@
5 <summary>Reserve launcher area</summary>
6 <description>Use EWMH standard to reserve an area of the desktop (struts) so that no window can use the launcher's area</description>
7 </key>
8+ <key name="maximum-width" type="i">
9+ <default>66</default>
10+ <summary>Maximum width</summary>
11+ <description>Launcher panel maximum width</description>
12+ </key>
13 </schema>
14 <schema path="/com/canonical/unity-2d/panel/" id="com.canonical.Unity2d.Panel" gettext-domain="unity-2d">
15 <key type="as" name="applets">
16
17=== modified file 'data/unity-2d.convert'
18--- data/unity-2d.convert 2011-08-02 10:15:06 +0000
19+++ data/unity-2d.convert 2011-10-29 00:34:27 +0000
20@@ -2,4 +2,5 @@
21 super-key-enable = /desktop/unity-2d/launcher/super_key_enable
22 hide-mode = /desktop/unity-2d/launcher/hide_mode
23 use-strut = /desktop/unity-2d/launcher/use_strut
24+maximum-width = /desktop/unity-2d/launcher/maximum_width
25
26
27=== modified file 'launcher/LauncherItem.qml'
28--- launcher/LauncherItem.qml 2011-09-15 10:52:36 +0000
29+++ launcher/LauncherItem.qml 2011-10-29 00:34:27 +0000
30@@ -245,8 +245,8 @@
31 anchors.centerIn: parent
32 smooth: true
33
34- sourceSize.width: 48
35- sourceSize.height: 48
36+ sourceSize.width: item.tileSize - 6
37+ sourceSize.height: item.tileSize - 6
38
39 /* Whenever one of the parameters used in calculating the background color of
40 the icon changes, recalculate its value */
41
42=== modified file 'launcher/LauncherList.qml'
43--- launcher/LauncherList.qml 2011-09-29 07:54:12 +0000
44+++ launcher/LauncherList.qml 2011-10-29 00:34:27 +0000
45@@ -26,9 +26,9 @@
46 by adding some padding to the items because of
47 http://bugreports.qt.nokia.com/browse/QTBUG-17622. */
48 spacing: 0
49- property int itemPadding: 5
50+ property int itemPadding: 1
51
52- property int tileSize: 54
53+ property int tileSize: parent.width - 12
54
55 /* Keep a reference to the currently visible contextual menu */
56 property variant visibleMenu
57@@ -241,7 +241,7 @@
58
59 function setIconGeometry() {
60 if (running) {
61- item.setIconGeometry(x + panel.x, y + panel.y, width, height)
62+ //item.setIconGeometry(x + panel.x, y + panel.y, width, height)
63 }
64 }
65
66
67=== modified file 'launcher/app/launcher.cpp'
68--- launcher/app/launcher.cpp 2011-09-20 08:40:43 +0000
69+++ launcher/app/launcher.cpp 2011-10-29 00:34:27 +0000
70@@ -91,6 +91,12 @@
71 /* Panel containing the QML declarative view */
72 Unity2dPanel panel(true);
73
74+ /* Synchronise panel's "useStrut" property with its corresponding DConf key */
75+ QConf dconfLauncher(LAUNCHER_DCONF_SCHEMA);
76+ LauncherClient::MaximumWidth = dconfLauncher.property("maximumWidth").toInt();
77+ if (LauncherClient::MaximumWidth < 32)
78+ LauncherClient::MaximumWidth = 32;
79+
80 panel.setEdge(Unity2dPanel::LeftEdge);
81 panel.setFixedWidth(LauncherClient::MaximumWidth);
82 panel.setAccessibleName("Launcher");
83@@ -123,7 +129,6 @@
84 launcherView->setSource(QUrl("./Launcher.qml"));
85
86 /* Synchronise panel's "useStrut" property with its corresponding DConf key */
87- QConf dconfLauncher(LAUNCHER_DCONF_SCHEMA);
88 panel.setUseStrut(dconfLauncher.property("useStrut").toBool());
89 PropertyBinder useStrutBinder;
90 useStrutBinder.bind(&dconfLauncher, "useStrut", &panel, "useStrut");
91
92=== modified file 'libunity-2d-private/src/launcherclient.cpp'
93--- libunity-2d-private/src/launcherclient.cpp 2011-03-22 16:30:31 +0000
94+++ libunity-2d-private/src/launcherclient.cpp 2011-10-29 00:34:27 +0000
95@@ -30,7 +30,7 @@
96 static const char* LAUNCHER_DBUS_OBJECT_PATH = "/Launcher";
97 static const char* LAUNCHER_DBUS_INTERFACE = "com.canonical.Unity2d.Launcher";
98
99-const int LauncherClient::MaximumWidth = 66;
100+int LauncherClient::MaximumWidth = 66;
101
102 struct LauncherClientPrivate
103 {
104
105=== modified file 'libunity-2d-private/src/launcherclient.h'
106--- libunity-2d-private/src/launcherclient.h 2011-03-22 11:30:53 +0000
107+++ libunity-2d-private/src/launcherclient.h 2011-10-29 00:34:27 +0000
108@@ -36,7 +36,7 @@
109 public:
110 // The amount of pixels used by the launcher on the left edge when it is
111 // fully visible.
112- static const int MaximumWidth;
113+ static int MaximumWidth;
114
115 LauncherClient(QObject* parent = 0);
116 ~LauncherClient();
117
118=== modified file 'places/app/CMakeLists.txt'
119--- places/app/CMakeLists.txt 2011-07-18 14:36:34 +0000
120+++ places/app/CMakeLists.txt 2011-10-29 00:34:27 +0000
121@@ -27,6 +27,7 @@
122 ${CMAKE_CURRENT_BINARY_DIR}
123 ${QTGCONF_INCLUDE_DIRS}
124 ${GTK_INCLUDE_DIRS}
125+ ${DCONFQT_INCLUDE_DIRS}
126 ${libunity-2d-private_SOURCE_DIR}/src
127 )
128
129@@ -36,6 +37,7 @@
130 ${QT_QTDBUS_LIBRARIES}
131 ${QT_QTDECLARATIVE_LIBRARIES}
132 ${QTGCONF_LDFLAGS}
133+ ${DCONFQT_LDFLAGS}
134 ${GTK_LDFLAGS}
135 ${X11_Xext_LIB}
136 ${X11_X11_LIB}
137
138=== modified file 'places/app/places.cpp'
139--- places/app/places.cpp 2011-09-20 08:40:43 +0000
140+++ places/app/places.cpp 2011-10-29 00:34:27 +0000
141@@ -34,10 +34,16 @@
142 // unity-2d
143 #include <unity2dapplication.h>
144 #include <unity2ddebug.h>
145+#include <launcherclient.h>
146
147 #include "dashdeclarativeview.h"
148 #include "config.h"
149
150+// libdconf-qt
151+#include "qconf.h"
152+
153+static const char* LAUNCHER_DCONF_SCHEMA = "com.canonical.Unity2d.Launcher";
154+
155 int main(int argc, char *argv[])
156 {
157 Unity2dApplication::earlySetup(argc, argv);
158@@ -45,6 +51,11 @@
159 application.setApplicationName("Unity 2D Dash");
160 QSet<QString> arguments = QSet<QString>::fromList(QCoreApplication::arguments());
161
162+ QConf dconfLauncher(LAUNCHER_DCONF_SCHEMA);
163+ LauncherClient::MaximumWidth = dconfLauncher.property("maximumWidth").toInt();
164+ if (LauncherClient::MaximumWidth < 32)
165+ LauncherClient::MaximumWidth = 32;
166+
167 qmlRegisterType<DashDeclarativeView>("Unity2d", 1, 0, "DashDeclarativeView");
168 DashDeclarativeView view;
169 view.setAccessibleName("Dash");
170
171=== modified file 'spread/app/CMakeLists.txt'
172--- spread/app/CMakeLists.txt 2011-07-18 14:36:34 +0000
173+++ spread/app/CMakeLists.txt 2011-10-29 00:34:27 +0000
174@@ -25,6 +25,7 @@
175 ${CMAKE_CURRENT_SOURCE_DIR}
176 ${CMAKE_CURRENT_BINARY_DIR}
177 ${GTK_INCLUDE_DIRS}
178+ ${DCONFQT_INCLUDE_DIRS}
179 ${libunity-2d-private_SOURCE_DIR}/src
180 )
181
182@@ -33,6 +34,7 @@
183 ${QT_QTGUI_LIBRARIES}
184 ${QT_QTDBUS_LIBRARIES}
185 ${QT_QTDECLARATIVE_LIBRARIES}
186+ ${DCONFQT_INCLUDE_DIRS}
187 ${GTK_LDFLAGS}
188 ${X11_Xext_LIB}
189 ${X11_X11_LIB}
190
191=== modified file 'spread/app/spread.cpp'
192--- spread/app/spread.cpp 2011-08-09 17:06:35 +0000
193+++ spread/app/spread.cpp 2011-10-29 00:34:27 +0000
194@@ -31,12 +31,22 @@
195
196 #include "config.h"
197
198+// libdconf-qt
199+#include "qconf.h"
200+
201+static const char* LAUNCHER_DCONF_SCHEMA = "com.canonical.Unity2d.Launcher";
202+
203 int main(int argc, char *argv[])
204 {
205 Unity2dApplication::earlySetup(argc, argv);
206 Unity2dApplication application(argc, argv);
207 application.setApplicationName("Unity 2D Workspace Switcher");
208 QSet<QString> arguments = QSet<QString>::fromList(QCoreApplication::arguments());
209+
210+ QConf dconfLauncher(LAUNCHER_DCONF_SCHEMA);
211+ LauncherClient::MaximumWidth = dconfLauncher.property("maximumWidth").toInt();
212+ if (LauncherClient::MaximumWidth < 32)
213+ LauncherClient::MaximumWidth = 32;
214
215 SpreadView view;
216 view.setUseOpenGL(arguments.contains("-opengl"));

Subscribers

People subscribed via source and target branches