Merge lp:~pete-woods/hud/tweak-search-parameters into lp:hud/14.04

Proposed by Pete Woods on 2014-02-25
Status: Merged
Approved by: Antti Kaijanmäki on 2014-03-14
Approved revision: 385
Merged at revision: 378
Proposed branch: lp:~pete-woods/hud/tweak-search-parameters
Merge into: lp:hud/14.04
Prerequisite: lp:~pete-woods/hud/no-suggestions-for-legacy-queries
Diff against target: 227 lines (+44/-30)
8 files modified
CMakeLists.txt (+1/-1)
data/com.canonical.indicator.appmenu.hud.search.gschema.xml (+4/-4)
debian/control (+1/-1)
service/HardCodedSearchSettings.h (+2/-2)
service/ItemStore.cpp (+28/-17)
service/QGSettingsSearchSettings.cpp (+2/-2)
tests/integration/TestHud.cpp (+2/-2)
tests/unit/service/TestItemStore.cpp (+4/-1)
To merge this branch: bzr merge lp:~pete-woods/hud/tweak-search-parameters
Reviewer Review Type Date Requested Status
Charles Kerr (community) Needs Information on 2014-03-15
Antti Kaijanmäki (community) Approve on 2014-03-14
PS Jenkins bot (community) continuous-integration Needs Fixing on 2014-03-14
Sebastien Bacher 2014-02-25 Needs Information on 2014-02-25
Review via email: mp+208087@code.launchpad.net

Commit message

Tweak search parameters following advice from Jussi

Description of the change

* Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)
  * Yes
 * Did you build your software in a clean sbuild/pbuilder chroot or ppa?
  * Yes
 * Did you build your software in a clean sbuild/pbuilder armhf chroot or ppa?
  * Yes
 * Has your component "TestPlan” been executed successfully on emulator, N4?
  * Yes
 * Has a 5 minute exploratory testing run been executed on N4?
  * Yes
 * If you changed the packaging (debian), did you subscribe a core-dev to this MP?
  * N/A
 * If you changed the UI, did you subscribe the design-reviewers to this MP?
  * No change
 * What components might get impacted by your changes?
  * Unity7
  * Unity8
 * Have you requested review by the teams of these owning components?
  * Yes

Check List:
https://wiki.ubuntu.com/Process/Merges/Checklists/hud

Test Plan:
https://wiki.ubuntu.com/Process/Merges/TestPlan/hud

Silo:
https://launchpad.net/~ci-train-ppa-service/+archive/landing-002/

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Sebastien Bacher (seb128) wrote :

Thanks for the work, could you explain what issues you are trying to fix with the parameters tweaks? Could you give an example of a query which was considered buggy and what are the differences in the results now?

review: Needs Information
Pete Woods (pete-woods) wrote :

> Thanks for the work, could you explain what issues you are trying to fix with
> the parameters tweaks? Could you give an example of a query which was
> considered buggy and what are the differences in the results now?

The branch is not ready for merge, but is part of an investigation into the (now) linked bug. I expect there will additionally be changes made to libcolumbus. I will link the relevant columbus branch to this bug, also.

The test application is essentially GIMP. If you search for, for example, "crop to selection" (assuming you have something selected), you will see that you don't exactly get the results you'd expect. The same applies to the other cases I have been testing "fit canvas to layers" and "fit canvas to selection".

Sebastien Bacher (seb128) wrote :

the new parameters have regressions. One example, open gedit in french and type "pref" -> empty results, it used to list "préférence", adding a char "prefe" and it's still empty, you need to type "prefer" to get a result.

Pete Woods (pete-woods) wrote :

> the new parameters have regressions. One example, open gedit in french and
> type "pref" -> empty results, it used to list "préférence", adding a char
> "prefe" and it's still empty, you need to type "prefer" to get a result.

Hmm, that's annoying, the unit tests (http://bazaar.launchpad.net/~pete-woods/hud/tweak-search-parameters/view/head:/tests/unit/service/TestItemStore.cpp#L181) are clearly not strong enough for this regression!

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
375. By Pete Woods on 2014-03-04

Change GTK documentation so that it builds with the new glib version Fixes: 1287580

376. By PS Jenkins bot on 2014-03-04

Releasing 13.10.1+14.04.20140304-0ubuntu1

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Antti Kaijanmäki (kaijanmaki) wrote :

LGTM. Waiting for silo.

Antti Kaijanmäki (kaijanmaki) wrote :

 * Are any changes against your component pending/needed to land the MP under review in a functional state and are those called out explicitly by the submitter?
Yes.

 * Did you do exploratory testing related to the component you own with the MP changeset included?
Yes.

 * Has the submitter requested review by all the relevant teams/reviewres?
Yes.

 * If you are the reviewer owning the component the MP is against, have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?
Yes.

review: Approve
Charles Kerr (charlesk) wrote :

I notice this approved and waiting for silo even though the only changes since seb's "préférence" regression are docs/packaging changes. Are we releasing with that regression? Is it fixed upstream in libcolumbus?

review: Needs Information
Pete Woods (pete-woods) wrote :

> I notice this approved and waiting for silo even though the only changes since
> seb's "préférence" regression are docs/packaging changes. Are we releasing
> with that regression? Is it fixed upstream in libcolumbus?

Don't worry! The history just looks funny because I rebased.

I specifically strengthen the test for what Seb was talking about in revision 384. I also installed the French language pack and checked the search manually.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2014-02-10 12:13:59 +0000
3+++ CMakeLists.txt 2014-03-14 14:07:40 +0000
4@@ -163,9 +163,9 @@
5 check
6 ${CMAKE_CTEST_COMMAND} --force-new-ctest-process --output-on-failure
7 )
8+ add_subdirectory(tests)
9 endif()
10
11-add_subdirectory(tests)
12
13 add_subdirectory(tools-vala)
14 if(${ENABLE_DOCUMENTATION})
15
16=== modified file 'data/com.canonical.indicator.appmenu.hud.search.gschema.xml'
17--- data/com.canonical.indicator.appmenu.hud.search.gschema.xml 2012-04-02 15:57:16 +0000
18+++ data/com.canonical.indicator.appmenu.hud.search.gschema.xml 2014-03-14 14:07:40 +0000
19@@ -15,7 +15,7 @@
20 </key>
21
22 <key type='u' name='add-penalty'>
23- <default>10</default>
24+ <default>100</default>
25 <summary>Penalty for extra characters added to the search</summary>
26 <description>
27 The penalty for each extra character in the search string that does not appear in the text of a menu
28@@ -27,7 +27,7 @@
29 </key>
30
31 <key type='u' name='drop-penalty'>
32- <default>10</default>
33+ <default>100</default>
34 <summary>Penalty applied if a character is dropped</summary>
35 <description>
36 The penalty for each character dropped from the search string, as compared with the text of a menu item.
37@@ -39,7 +39,7 @@
38 </key>
39
40 <key type='u' name='end-drop-penalty'>
41- <default>1</default>
42+ <default>20</default>
43 <summary>Penalty applied if a character is dropped from the end</summary>
44 <description>
45 The penalty for each missing character at the end of a search term.
46@@ -50,7 +50,7 @@
47 </key>
48
49 <key type='u' name='swap-penalty'>
50- <default>15</default>
51+ <default>150</default>
52 <summary>Penalty applied when the characters are not the same</summary>
53 <description>
54 The penalty for each substituted character in the search term.
55
56=== modified file 'debian/control'
57--- debian/control 2014-02-10 12:13:59 +0000
58+++ debian/control 2014-03-14 14:07:40 +0000
59@@ -9,7 +9,7 @@
60 google-mock (>= 1.6.0+svn437),
61 gtk-doc-tools,
62 intltool,
63- libcolumbus1-dev,
64+ libcolumbus1-dev (>= 1.1.0),
65 libdbusmenu-glib-dev (>= 0.5.90),
66 libdbusmenu-gtk3-dev (>= 0.5.90),
67 libdbusmenu-jsonloader-dev (>= 0.5.90),
68
69=== modified file 'service/HardCodedSearchSettings.h'
70--- service/HardCodedSearchSettings.h 2014-02-17 09:58:27 +0000
71+++ service/HardCodedSearchSettings.h 2014-03-14 14:07:40 +0000
72@@ -51,9 +51,9 @@
73
74 uint m_dropPenalty = 100;
75
76- uint m_endDropPenalty = 1;
77+ uint m_endDropPenalty = 20;
78
79- uint m_swapPenalty = 5;
80+ uint m_swapPenalty = 150;
81 };
82
83 }
84
85=== modified file 'service/ItemStore.cpp'
86--- service/ItemStore.cpp 2014-03-14 14:07:39 +0000
87+++ service/ItemStore.cpp 2014-03-14 14:07:40 +0000
88@@ -29,6 +29,7 @@
89 using namespace Columbus;
90
91 static const QRegularExpression SINGLE_AMPERSAND("(?<![&])[&](?![&])");
92+static const QRegularExpression BAD_CHARACTERS("\\.\\.\\.|…");
93 static const QRegularExpression WHITESPACE("\\s+");
94 static const QRegularExpression WHITESPACE_OR_SEMICOLON("[;\\s+]");
95
96@@ -39,8 +40,9 @@
97 ErrorValues &errorValues(m_matcher.getErrorValues());
98 errorValues.addStandardErrors();
99
100- connect(m_settings.data(), SIGNAL(changed()), this,
101- SLOT(settingChanged()));
102+ m_matcher.getIndexWeights().setWeight(Word("context"), 0.5);
103+
104+ connect(m_settings.data(), SIGNAL(changed()), this, SLOT(settingChanged()));
105 settingChanged();
106 }
107
108@@ -72,7 +74,9 @@
109 continue;
110 }
111
112- QStringList text(convertActionText(action).split(WHITESPACE));
113+ QStringList text(
114+ convertActionText(action).remove(BAD_CHARACTERS).split(
115+ WHITESPACE));
116
117 bool isParameterized(action->property("isParameterized").toBool());
118
119@@ -89,7 +93,7 @@
120
121 WordList command;
122 for (const QString &word : text) {
123- command.addWord(Word(word.toStdString()));
124+ command.addWord(Word(word.toUtf8().constData()));
125 }
126 document.addText(Word("command"), command);
127
128@@ -102,7 +106,7 @@
129 context = stack;
130 }
131 for (const QString &word : context) {
132- wordList.addWord(Word(word.toStdString()));
133+ wordList.addWord(Word(word.toUtf8().constData()));
134 }
135 document.addText(Word("context"), wordList);
136
137@@ -177,21 +181,28 @@
138 }
139
140 } else {
141+ QString cleanQuery(query);
142+ cleanQuery.remove(BAD_CHARACTERS);
143+
144 WordList queryList;
145- for (const QString &word : query.split(WHITESPACE)) {
146- queryList.addWord(word.toStdString());
147+ for (const QString &word : cleanQuery.split(WHITESPACE)) {
148+ queryList.addWord(Word(word.toUtf8().constData()));
149 }
150
151- MatchResults matchResults(m_matcher.match(queryList));
152-
153- int queryLength(query.length());
154-
155- size_t maxResults = std::min(matchResults.size(), size_t(20));
156-
157- for (size_t i(0); i < maxResults; ++i) {
158- DocumentID id(matchResults.getDocumentID(i));
159- double relevancy(matchResults.getRelevancy(i));
160- addResult(id, stringMatcher, queryLength, relevancy, results);
161+ try {
162+ MatchResults matchResults(
163+ m_matcher.onlineMatch(queryList, Word("command")));
164+
165+ int queryLength(query.length());
166+
167+ size_t maxResults = std::min(matchResults.size(), size_t(20));
168+
169+ for (size_t i(0); i < maxResults; ++i) {
170+ DocumentID id(matchResults.getDocumentID(i));
171+ double relevancy(matchResults.getRelevancy(i));
172+ addResult(id, stringMatcher, queryLength, relevancy, results);
173+ }
174+ } catch (std::invalid_argument &e) {
175 }
176 }
177
178
179=== modified file 'service/QGSettingsSearchSettings.cpp'
180--- service/QGSettingsSearchSettings.cpp 2014-02-15 10:23:45 +0000
181+++ service/QGSettingsSearchSettings.cpp 2014-03-14 14:07:40 +0000
182@@ -34,11 +34,11 @@
183 }
184
185 uint QGSettingsSearchSettings::addPenalty() const {
186- return m_settings.get("addPenalty").toUInt() * 10;
187+ return m_settings.get("addPenalty").toUInt();
188 }
189
190 uint QGSettingsSearchSettings::dropPenalty() const {
191- return m_settings.get("dropPenalty").toUInt() * 10;
192+ return m_settings.get("dropPenalty").toUInt();
193 }
194
195 uint QGSettingsSearchSettings::endDropPenalty() const {
196
197=== modified file 'tests/integration/TestHud.cpp'
198--- tests/integration/TestHud.cpp 2014-01-24 14:25:22 +0000
199+++ tests/integration/TestHud.cpp 2014-03-14 14:07:40 +0000
200@@ -187,8 +187,8 @@
201 countChangedSpy.wait();
202 }
203 ASSERT_EQ(3, results.rowCount());
204- EXPECT_EQ(ResultPair("swift sad", "piece hook"), result(results, 0));
205- EXPECT_EQ(ResultPair("stray slash", "piece hook"), result(results, 1));
206+ EXPECT_EQ(ResultPair("stray slash", "piece hook"), result(results, 0));
207+ EXPECT_EQ(ResultPair("swift sad", "piece hook"), result(results, 1));
208 EXPECT_EQ(ResultPair("bowl", "link"), result(results, 2));
209 }
210
211
212=== modified file 'tests/unit/service/TestItemStore.cpp'
213--- tests/unit/service/TestItemStore.cpp 2014-03-14 14:07:39 +0000
214+++ tests/unit/service/TestItemStore.cpp 2014-03-14 14:07:40 +0000
215@@ -222,8 +222,11 @@
216 QMenu root;
217
218 QMenu file("File");
219+ file.addAction("Banana");
220+ file.addAction("Save All");
221+ file.addAction("Save");
222 file.addAction("Save As...");
223- file.addAction("Save");
224+ file.addAction("Apple");
225 root.addMenu(&file);
226
227 store->indexMenu(&root);

Subscribers

People subscribed via source and target branches