Merge lp:~azzar1/unity/fix-868452-825037 into lp:unity

Proposed by Andrea Azzarone on 2011-11-28
Status: Merged
Approved by: Andrea Azzarone on 2011-12-06
Approved revision: 1747
Merged at revision: 1764
Proposed branch: lp:~azzar1/unity/fix-868452-825037
Merge into: lp:unity
Diff against target: 168 lines (+80/-13)
3 files modified
plugins/unityshell/src/BFBLauncherIcon.cpp (+66/-8)
plugins/unityshell/src/BFBLauncherIcon.h (+11/-2)
plugins/unityshell/src/DashView.cpp (+3/-3)
To merge this branch: bzr merge lp:~azzar1/unity/fix-868452-825037
Reviewer Review Type Date Requested Status
Mirco Müller (community) 2011-11-28 Approve on 2011-12-06
Marco Trevisan (Treviño) Approve on 2011-12-05
Review via email: mp+83652@code.launchpad.net

Description of the change

Display a list of the lenses in the bfb quicklist. It tries to fix bug #825037 too.

To post a comment you must log in.
Marco Trevisan (Treviño) (3v1n0) wrote :

It works as advertised, I guess this should be tested when autopilot is ready.

However, works well (fixes both the linked bugs) and the code looks good to me.

Just one thing, I guess that should be fixed at upper levels:
+ // "Files & folders", you are the culprit!
+ std::string name = lens->name();
+ boost::replace_all(name, "&", "&");

Plus, if you open the dash, and you right-click on the BFB, all the quicklist items works, except the "Dash Home" item.

review: Needs Fixing
Andrea Azzarone (azzar1) wrote :

> It works as advertised, I guess this should be tested when autopilot is ready.
>
> However, works well (fixes both the linked bugs) and the code looks good to
> me.
>
> Just one thing, I guess that should be fixed at upper levels:
> + // "Files & folders", you are the culprit!
> + std::string name = lens->name();
> + boost::replace_all(name, "&", "&");

In the quicklist drawing functions?

>
> Plus, if you open the dash, and you right-click on the BFB, all the quicklist
> items works, except the "Dash Home" item.

I'll talk to Neil about this ;)

Marco Trevisan (Treviño) (3v1n0) wrote :

> > Just one thing, I guess that should be fixed at upper levels:
> > + // "Files & folders", you are the culprit!
> > + std::string name = lens->name();
> > + boost::replace_all(name, "&", "&");
>
> In the quicklist drawing functions?

Sorry, I meant "lower" level. So, I guess on the Unity-core lens wrapper or even at the libunity level.
I guess that libunity replaces the & with & to send these values via dbus, so we just need to check that the lens->name() property returns the right value for that.

Andrea Azzarone (azzar1) wrote :

> > > Just one thing, I guess that should be fixed at upper levels:
> > > + // "Files & folders", you are the culprit!
> > > + std::string name = lens->name();
> > > + boost::replace_all(name, "&", "&");
> >
> > In the quicklist drawing functions?
>
> Sorry, I meant "lower" level. So, I guess on the Unity-core lens wrapper or
> even at the libunity level.
> I guess that libunity replaces the & with & to send these values via dbus,
> so we just need to check that the lens->name() property returns the right
> value for that.

I don't understand. The problem is that lens->name() returns "Files & folders" and not "Files & Folder". I think that we're using pango in the quicklist drawing functions and pango doesn't like the & char :)

Marco Trevisan (Treviño) (3v1n0) wrote :

Ah, ok... Sorry I misunderstood that then.
So, the issue is on the quicklist drawing function, and the issue is happening also when a quicklist item has a "<" inside.

This should be definitely fixed at that level, otherwise also the application quicklist will suffer for it.
I'll open another bug for that and I'll work on it, so please, remove the workaround from your code :)

Andrea Azzarone (azzar1) wrote :

Ok... Use something like this:

       boost::replace_all(value, "&", "&amp;");
       boost::replace_all(value, "<", "&lt;");
       boost::replace_all(value, ">", "&gt;");

Marco Trevisan (Treviño) (3v1n0) wrote :

Andrea, to fix both the & and < issue, you can just use g_markup_escape_text(str, -1), then when I'll fix properly the bug #899677 (easy to fix, but as discussed, I'd like to define something with design before), I'll see if this should centralized.

Andrea Azzarone (azzar1) wrote :

Done.

Marco Trevisan (Treviño) (3v1n0) wrote :

Code looks good now, and works as expected.

However I guess you need at least a manual test for it.

review: Approve
Mirco Müller (macslow) wrote :

Ok, looking good to me.

review: Approve
Unity Merger (unity-merger) wrote :

Attempt to merge into lp:unity failed due to conflicts:

text conflict in plugins/unityshell/src/BFBLauncherIcon.cpp

Andrea Azzarone (azzar1) wrote :

All conflicts resolved.

Unity Merger (unity-merger) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/35/console reported an error when processing this lp:~andyrock/unity/fix-868452-825037 branch.
Not merging it.

lp:~azzar1/unity/fix-868452-825037 updated on 2011-12-06
1747. By Andrea Azzarone on 2011-12-06

Ops :/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/unityshell/src/BFBLauncherIcon.cpp'
2--- plugins/unityshell/src/BFBLauncherIcon.cpp 2011-11-22 17:47:59 +0000
3+++ plugins/unityshell/src/BFBLauncherIcon.cpp 2011-12-06 16:36:28 +0000
4@@ -15,6 +15,7 @@
5 * along with this program. If not, see <http://www.gnu.org/licenses/>.
6 *
7 * Authored by: Jason Smith <jason.smith@canonical.com>
8+ * Andrea Azzarone <azzaronea@gmail.com>
9 */
10
11 #include "BFBLauncherIcon.h"
12@@ -28,6 +29,8 @@
13 {
14 namespace launcher
15 {
16+
17+UBusManager BFBLauncherIcon::ubus_manager_;
18
19 BFBLauncherIcon::BFBLauncherIcon(Launcher* IconManager)
20 : SimpleLauncherIcon(IconManager)
21@@ -37,29 +40,84 @@
22 SetQuirk(QUIRK_VISIBLE, true);
23 SetQuirk(QUIRK_RUNNING, false);
24 SetIconType(TYPE_HOME);
25-
26- _background_color = nux::color::White;
27-
28- mouse_enter.connect([&] () { _ubus_manager.SendMessage(UBUS_DASH_ABOUT_TO_SHOW, NULL); });
29+
30+ background_color_ = nux::color::White;
31+
32+ mouse_enter.connect([&]() { ubus_manager_.SendMessage(UBUS_DASH_ABOUT_TO_SHOW, NULL); });
33 }
34
35 nux::Color BFBLauncherIcon::BackgroundColor()
36 {
37- return _background_color;
38+ return background_color_;
39 }
40
41 nux::Color BFBLauncherIcon::GlowColor()
42 {
43- return _background_color;
44+ return background_color_;
45 }
46
47 void BFBLauncherIcon::ActivateLauncherIcon(ActionArg arg)
48 {
49- if (arg.button == 1)
50- _ubus_manager.SendMessage (UBUS_PLACE_ENTRY_ACTIVATE_REQUEST, g_variant_new("(sus)", "home.lens", 0, ""));
51+ ubus_manager_.SendMessage(UBUS_PLACE_ENTRY_ACTIVATE_REQUEST, g_variant_new("(sus)", "home.lens", 0, ""));
52
53 // dont chain down to avoid random dash close events
54 }
55
56+void BFBLauncherIcon::OnMenuitemActivated(DbusmenuMenuitem* item,
57+ int time,
58+ gchar* lens)
59+{
60+ if (lens != NULL)
61+ {
62+ ubus_manager_.SendMessage(UBUS_PLACE_ENTRY_ACTIVATE_REQUEST, g_variant_new("(sus)", lens, dash::GOTO_DASH_URI, ""));
63+ g_free(lens);
64+ }
65+}
66+
67+std::list<DbusmenuMenuitem*> BFBLauncherIcon::GetMenus()
68+{
69+ std::list<DbusmenuMenuitem*> result;
70+ DbusmenuMenuitem* menu_item;
71+
72+ // Home dash
73+ menu_item = dbusmenu_menuitem_new();
74+
75+ dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_LABEL, _("Dash Home"));
76+ dbusmenu_menuitem_property_set_bool(menu_item, DBUSMENU_MENUITEM_PROP_ENABLED, true);
77+ dbusmenu_menuitem_property_set_bool(menu_item, DBUSMENU_MENUITEM_PROP_VISIBLE, true);
78+
79+ g_signal_connect(menu_item,
80+ DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED,
81+ (GCallback)&BFBLauncherIcon::OnMenuitemActivated,
82+ g_strdup("home.lens"));
83+
84+ result.push_back(menu_item);
85+
86+ // Other lenses..
87+ for (auto lens : lenses_.GetLenses())
88+ {
89+ if (!lens->visible())
90+ continue;
91+
92+ glib::String name(g_markup_escape_text(lens->name().c_str(), -1));
93+
94+ menu_item = dbusmenu_menuitem_new();
95+
96+ dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_LABEL, name.Value());
97+ dbusmenu_menuitem_property_set_bool(menu_item, DBUSMENU_MENUITEM_PROP_ENABLED, true);
98+ dbusmenu_menuitem_property_set_bool(menu_item, DBUSMENU_MENUITEM_PROP_VISIBLE, true);
99+
100+ g_signal_connect(menu_item,
101+ DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED,
102+ (GCallback)&BFBLauncherIcon::OnMenuitemActivated,
103+ g_strdup(lens->id().c_str()));
104+
105+ result.push_back(menu_item);
106+ }
107+
108+ return result;
109+}
110+
111 } // namespace launcher
112 } // namespace unity
113+
114
115=== modified file 'plugins/unityshell/src/BFBLauncherIcon.h'
116--- plugins/unityshell/src/BFBLauncherIcon.h 2011-10-06 04:18:36 +0000
117+++ plugins/unityshell/src/BFBLauncherIcon.h 2011-12-06 16:36:28 +0000
118@@ -22,6 +22,8 @@
119
120 #include "SimpleLauncherIcon.h"
121
122+#include <UnityCore/FilesystemLenses.h>
123+
124 #include "UBusWrapper.h"
125
126 namespace unity
127@@ -39,9 +41,16 @@
128 virtual nux::Color GlowColor();
129
130 void ActivateLauncherIcon(ActionArg arg);
131+
132+protected:
133+ std::list<DbusmenuMenuitem*> GetMenus();
134+
135 private:
136- unity::UBusManager _ubus_manager;
137- nux::Color _background_color;
138+ static void OnMenuitemActivated(DbusmenuMenuitem* item, int time, gchar* lens);
139+
140+ static unity::UBusManager ubus_manager_;
141+ nux::Color background_color_;
142+ dash::FilesystemLenses lenses_;
143 };
144
145 }
146
147=== modified file 'plugins/unityshell/src/DashView.cpp'
148--- plugins/unityshell/src/DashView.cpp 2011-12-06 00:21:09 +0000
149+++ plugins/unityshell/src/DashView.cpp 2011-12-06 16:36:28 +0000
150@@ -542,16 +542,16 @@
151 {
152 glib::String uri;
153 glib::String search_string;
154+ dash::HandledType handled_type;
155
156- g_variant_get(args, "(sus)", &uri, NULL, &search_string);
157+ g_variant_get(args, "(sus)", &uri, &handled_type, &search_string);
158
159 std::string id = AnalyseLensURI(uri.Str());
160
161 home_view_->search_string = "";
162 lens_bar_->Activate(id);
163
164-
165- if (id == "home.lens" || !visible_)
166+ if ((id == "home.lens" && handled_type != GOTO_DASH_URI ) || !visible_)
167 ubus_manager_.SendMessage(UBUS_DASH_EXTERNAL_ACTIVATION);
168 }
169