Merge ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/bionic-sru3 into ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/bionic

Proposed by Marco Trevisan (Treviño)
Status: Merged
Merged at revision: 638809248ddac6ee03bfeed977739425f97465b6
Proposed branch: ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/bionic-sru3
Merge into: ~ubuntu-desktop/ubuntu/+source/gnome-shell:ubuntu/bionic
Prerequisite: ~3v1n0/ubuntu/+source/gnome-shell:ubuntu/bionic-touch-two-clicks-fix
Diff against target: 408 lines (+76/-46)
5 files modified
debian/changelog (+3/-6)
debian/patches/series (+2/-4)
debian/patches/st-scroll-view-Handle-the-case-where-scrollbars-are-NULL.patch (+38/-0)
debian/patches/st-scroll-view-Remove-scrollbars-references-on-dispose.patch (+33/-0)
dev/null (+0/-36)
Reviewer Review Type Date Requested Status
Iain Lane Approve
Daniel van Vugt (community) Approve
Review via email: mp+358010@code.launchpad.net

Description of the change

Reverting XUbuntuCancel patches, we will do them soon after this

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

The green stuff looks good to me.

I have no opinion on the red parts.

review: Approve
Revision history for this message
Iain Lane (laney) wrote :

thanks!

please keep working to get those fixes upstream

I guess the revert should be reverted - can you propose an MP for that please?

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index c6ad925..c43929d 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,16 +1,13 @@
1gnome-shell (3.28.3-0ubuntu0.18.04.3) UNRELEASED; urgency=medium1gnome-shell (3.28.3-0ubuntu0.18.04.3) UNRELEASED; urgency=medium
22
3 * d/p/search-Cancel-search-provider-operations-on-clear.patch,
4 d/p/search-Ignore-search-provider-results-metas-if-search-is-.patch,
5 d/p/viewSelector-Cancel-search-on-overview-hidden.patch,
6 d/p/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch:
7 - Add support for cancelling remote search providers when the overlay
8 is closed (and actually stop searches when requested from UI, LP: #1756826)
9 * debian/ubuntu.css:3 * debian/ubuntu.css:
10 - use defined color for menu separators (LP: #1739931)4 - use defined color for menu separators (LP: #1739931)
11 - set StEntry minimun height to work properly with Ubuntu font (LP: #1743058)5 - set StEntry minimun height to work properly with Ubuntu font (LP: #1743058)
12 * debian/patches/st-button-Ignore-pointer-emulated-touch-events.patch:6 * debian/patches/st-button-Ignore-pointer-emulated-touch-events.patch:
13 - Don't emit two click events on touch under X11 (LP: #1745888)7 - Don't emit two click events on touch under X11 (LP: #1745888)
8 * d/p/st-scroll-view-Handle-the-case-where-scrollbars-are-NULL.patch,
9 d/p/st-scroll-view-Remove-scrollbars-references-on-dispose.patch:
10 - Handle NULL scroll bars in st-scroll-view (LP: #1725312)
1411
15 -- Marco Trevisan (Treviño) <marco@ubuntu.com> Wed, 03 Oct 2018 00:50:42 +020012 -- Marco Trevisan (Treviño) <marco@ubuntu.com> Wed, 03 Oct 2018 00:50:42 +0200
1613
diff --git a/debian/patches/search-Cancel-search-provider-operations-on-clear.patch b/debian/patches/search-Cancel-search-provider-operations-on-clear.patch
17deleted file mode 10064414deleted file mode 100644
index 052ee8a..0000000
--- a/debian/patches/search-Cancel-search-provider-operations-on-clear.patch
+++ /dev/null
@@ -1,29 +0,0 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Thu, 23 Aug 2018 18:14:38 +0200
3Subject: search: Cancel search provider operations on clear
4
5Ensure that the search provider operations (just getResultMetas requests in the
6current implementation) in progress are properly cancelled when we clear the UI,
7otherwise returned results might still be added when not needed.
8
9This is triggered for each provider by the SearchResults reset.
10
11Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
12Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
13Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
14---
15 js/ui/search.js | 1 +
16 1 file changed, 1 insertion(+)
17
18diff --git a/js/ui/search.js b/js/ui/search.js
19index 1fb54b4..804be95 100644
20--- a/js/ui/search.js
21+++ b/js/ui/search.js
22@@ -192,6 +192,7 @@ var SearchResultsBase = new Lang.Class({
23 },
24
25 clear() {
26+ this._cancellable.cancel();
27 for (let resultId in this._resultDisplays)
28 this._resultDisplays[resultId].actor.destroy();
29 this._resultDisplays = {};
diff --git a/debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch b/debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch
30deleted file mode 1006440deleted file mode 100644
index 27645a5..0000000
--- a/debian/patches/search-Ignore-search-provider-results-metas-if-search-is-.patch
+++ /dev/null
@@ -1,30 +0,0 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Thu, 30 Aug 2018 07:11:24 +0200
3Subject: search: Ignore search provider results metas if search is cancelled
4
5Call updateSearch callback with no results when the search provider has been
6cancelled, without doing any logging.
7
8Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
9Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
10Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
11---
12 js/ui/search.js | 5 +++--
13 1 file changed, 3 insertions(+), 2 deletions(-)
14
15diff --git a/js/ui/search.js b/js/ui/search.js
16index 804be95..dd4bfad 100644
17--- a/js/ui/search.js
18+++ b/js/ui/search.js
19@@ -227,8 +227,9 @@ var SearchResultsBase = new Lang.Class({
20
21 this.provider.getResultMetas(metasNeeded, metas => {
22 if (metas.length != metasNeeded.length) {
23- log('Wrong number of result metas returned by search provider ' + this.provider.id +
24- ': expected ' + metasNeeded.length + ' but got ' + metas.length);
25+ if (!this._cancellable.is_cancelled())
26+ log(`Wrong number of result metas returned by search provider ${this.provider.id}` +
27+ `: expected ${metasNeeded.length} but got ${metas.length}`);
28 callback(false);
29 return;
30 }
diff --git a/debian/patches/series b/debian/patches/series
index 217740f..6a12345 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -22,8 +22,6 @@ authPrompt-Unset-preemptiveAnswer-on-reset.patch
22authPrompt-Do-not-enable-sensitivity-if-retries-are-disal.patch22authPrompt-Do-not-enable-sensitivity-if-retries-are-disal.patch
23gdm-util-Always-allow-to-retry-login-in-unlock-mode.patch23gdm-util-Always-allow-to-retry-login-in-unlock-mode.patch
24popupMenu-Don-t-handle-key-presses-directly-if-there-are-.patch24popupMenu-Don-t-handle-key-presses-directly-if-there-are-.patch
25viewSelector-Cancel-search-on-overview-hidden.patch
26search-Cancel-search-provider-operations-on-clear.patch
27search-Ignore-search-provider-results-metas-if-search-is-.patch
28st-button-Ignore-pointer-emulated-touch-events.patch25st-button-Ignore-pointer-emulated-touch-events.patch
29ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch26st-scroll-view-Handle-the-case-where-scrollbars-are-NULL.patch
27st-scroll-view-Remove-scrollbars-references-on-dispose.patch
diff --git a/debian/patches/st-scroll-view-Handle-the-case-where-scrollbars-are-NULL.patch b/debian/patches/st-scroll-view-Handle-the-case-where-scrollbars-are-NULL.patch
30new file mode 10064428new file mode 100644
index 0000000..0c5e93d
--- /dev/null
+++ b/debian/patches/st-scroll-view-Handle-the-case-where-scrollbars-are-NULL.patch
@@ -0,0 +1,38 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Fri, 3 Aug 2018 18:51:24 +0200
3Subject: st/scroll-view: Handle the case where scrollbars are NULL
4
5The scrollbars actors in a scroll view can be NULL, in case they get removed
6with a call to `clutter_actor_remove_child` on a scroll-view (and this is
7implemented in st_scroll_view_remove).
8
9So, we should support the case where `priv->{h,v}scroll` are NULL, not to crash
10in `st_widget_style_changed`.
11
12Fixes #467
13
14Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/467
15BUG-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1725312
16Forwarded: yes, https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/190
17---
18 src/st/st-scroll-view.c | 7 +++++--
19 1 file changed, 5 insertions(+), 2 deletions(-)
20
21diff --git a/src/st/st-scroll-view.c b/src/st/st-scroll-view.c
22index fc0db1c..a8b6d2e 100644
23--- a/src/st/st-scroll-view.c
24+++ b/src/st/st-scroll-view.c
25@@ -741,8 +741,11 @@ st_scroll_view_style_changed (StWidget *widget)
26 gdouble hfade_offset = st_theme_node_get_length (theme_node, "-st-hfade-offset");
27 st_scroll_view_update_fade_effect (self, vfade_offset, hfade_offset);
28
29- st_widget_style_changed (ST_WIDGET (priv->hscroll));
30- st_widget_style_changed (ST_WIDGET (priv->vscroll));
31+ if (priv->hscroll)
32+ st_widget_style_changed (ST_WIDGET (priv->hscroll));
33+
34+ if (priv->vscroll)
35+ st_widget_style_changed (ST_WIDGET (priv->vscroll));
36
37 ST_WIDGET_CLASS (st_scroll_view_parent_class)->style_changed (widget);
38 }
diff --git a/debian/patches/st-scroll-view-Remove-scrollbars-references-on-dispose.patch b/debian/patches/st-scroll-view-Remove-scrollbars-references-on-dispose.patch
0new file mode 10064439new file mode 100644
index 0000000..f5f49be
--- /dev/null
+++ b/debian/patches/st-scroll-view-Remove-scrollbars-references-on-dispose.patch
@@ -0,0 +1,33 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Fri, 3 Aug 2018 19:06:47 +0200
3Subject: st/scroll-view: Remove scrollbars references on dispose
4
5As we're destroying the scrollbars on destruction, we should remove any
6reference of it, not to cause multiple-calls to disposal to unreference them
7again.
8
9Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/467
10BUG-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1725312
11Forwarded: yes, https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/190
12---
13 src/st/st-scroll-view.c | 7 ++-----
14 1 file changed, 2 insertions(+), 5 deletions(-)
15
16diff --git a/src/st/st-scroll-view.c b/src/st/st-scroll-view.c
17index a8b6d2e..cb38aef 100644
18--- a/src/st/st-scroll-view.c
19+++ b/src/st/st-scroll-view.c
20@@ -244,11 +244,8 @@ st_scroll_view_dispose (GObject *object)
21 priv->fade_effect = NULL;
22 }
23
24- if (priv->vscroll)
25- clutter_actor_destroy (priv->vscroll);
26-
27- if (priv->hscroll)
28- clutter_actor_destroy (priv->hscroll);
29+ g_clear_pointer (&priv->vscroll, clutter_actor_destroy);
30+ g_clear_pointer (&priv->hscroll, clutter_actor_destroy);
31
32 /* For most reliable freeing of memory, an object with signals
33 * like StAdjustment should be explicitly disposed. Since we own
diff --git a/debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch b/debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch
0deleted file mode 10064434deleted file mode 100644
index 94a1d42..0000000
--- a/debian/patches/ubuntu/search-call-XUbuntuCancel-method-on-providers-when-no-dat.patch
+++ /dev/null
@@ -1,167 +0,0 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Thu, 23 Aug 2018 20:00:57 +0200
3Subject: search: call XUbuntuCancel method on providers when no data is
4 needed
5
6Add XUbuntuCancel method to search providers and call it when a search provider
7is still doing operations.
8Ignore the result when the method does not exist or is cancelled.
9
10This will allow to stop operations on providers.
11
12Fixes LP: #1756826
13
14Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
15Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
16Forwarded: not-needed
17---
18 data/org.gnome.ShellSearchProvider.xml | 6 ++++++
19 data/org.gnome.ShellSearchProvider2.xml | 6 ++++++
20 js/ui/remoteSearch.js | 15 +++++++++++++++
21 js/ui/search.js | 34 +++++++++++++++++++++++++++++++++
22 4 files changed, 61 insertions(+)
23
24diff --git a/data/org.gnome.ShellSearchProvider.xml b/data/org.gnome.ShellSearchProvider.xml
25index 78ad305..393cb01 100644
26--- a/data/org.gnome.ShellSearchProvider.xml
27+++ b/data/org.gnome.ShellSearchProvider.xml
28@@ -69,5 +69,11 @@
29 <method name="ActivateResult">
30 <arg type="s" name="identifier" direction="in" />
31 </method>
32+
33+ <!--
34+ XUbuntuCancel:
35+ Cancel the current search operation
36+ -->
37+ <method name="XUbuntuCancel" />
38 </interface>
39 </node>
40diff --git a/data/org.gnome.ShellSearchProvider2.xml b/data/org.gnome.ShellSearchProvider2.xml
41index 9502340..8141bc0 100644
42--- a/data/org.gnome.ShellSearchProvider2.xml
43+++ b/data/org.gnome.ShellSearchProvider2.xml
44@@ -83,5 +83,11 @@
45 <arg type="as" name="terms" direction="in" />
46 <arg type="u" name="timestamp" direction="in" />
47 </method>
48+
49+ <!--
50+ XUbuntuCancel:
51+ Cancel the current search operation
52+ -->
53+ <method name="XUbuntuCancel" />
54 </interface>
55 </node>
56diff --git a/js/ui/remoteSearch.js b/js/ui/remoteSearch.js
57index c6c5a29..8de6148 100644
58--- a/js/ui/remoteSearch.js
59+++ b/js/ui/remoteSearch.js
60@@ -30,6 +30,7 @@ const SearchProviderIface = '<node> \
61 <method name="ActivateResult"> \
62 <arg type="s" direction="in" /> \
63 </method> \
64+<method name="XUbuntuCancel" /> \
65 </interface> \
66 </node>';
67
68@@ -57,6 +58,7 @@ const SearchProvider2Iface = '<node> \
69 <arg type="as" direction="in" /> \
70 <arg type="u" direction="in" /> \
71 </method> \
72+<method name="XUbuntuCancel" /> \
73 </interface> \
74 </node>';
75
76@@ -310,6 +312,19 @@ var RemoteSearchProvider = new Lang.Class({
77 cancellable);
78 },
79
80+ XUbuntuCancel(cancellable, callback) {
81+ this.proxy.XUbuntuCancelRemote((results, error) => {
82+ if (error &&
83+ !error.matches(Gio.DBusError, Gio.DBusError.UNKNOWN_METHOD) &&
84+ !error.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)) {
85+ log('Received error from DBus search provider %s during XUbuntuCancel: %s'.format(this.id, String(error)));
86+ } else if (callback && !error) {
87+ callback();
88+ }
89+ },
90+ cancellable);
91+ },
92+
93 activateResult(id) {
94 this.proxy.ActivateResultRemote(id);
95 },
96diff --git a/js/ui/search.js b/js/ui/search.js
97index dd4bfad..629664e 100644
98--- a/js/ui/search.js
99+++ b/js/ui/search.js
100@@ -225,7 +225,9 @@ var SearchResultsBase = new Lang.Class({
101 this._cancellable.cancel();
102 this._cancellable.reset();
103
104+ this.provider.resultsMetasInProgress = true;
105 this.provider.getResultMetas(metasNeeded, metas => {
106+ this.provider.resultsMetasInProgress = this._cancellable.is_cancelled();
107 if (metas.length != metasNeeded.length) {
108 if (!this._cancellable.is_cancelled())
109 log(`Wrong number of result metas returned by search provider ${this.provider.id}` +
110@@ -450,6 +452,10 @@ var SearchResults = new Lang.Class({
111
112 this._searchTimeoutId = 0;
113 this._cancellable = new Gio.Cancellable();
114+ this._searchCancelCancellable = new Gio.Cancellable();
115+ this._cancellable.connect(() => {
116+ this._cancelSearchProviderRequest();
117+ });
118
119 this._registerProvider(new AppDisplay.AppSearchProvider());
120 this._reloadRemoteProviders();
121@@ -491,11 +497,32 @@ var SearchResults = new Lang.Class({
122 }
123 },
124
125+ _cancelSearchProviderRequest() {
126+ if (this._terms.length != 0 || this._searchCancelTimeoutId > 0)
127+ return;
128+
129+ this._searchCancelTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 100, () => {
130+ this._providers.forEach(provider => {
131+ if (provider.isRemoteProvider &&
132+ (provider.searchInProgress || provider.resultsMetasInProgress)) {
133+ provider.XUbuntuCancel(this._searchCancelCancellable, () => {
134+ provider.searchInProgress = false;
135+ provider.resultsMetasInProgress = false;
136+ });
137+ }
138+ });
139+
140+ delete this._searchCancelTimeoutId;
141+ return GLib.SOURCE_REMOVE;
142+ });
143+ },
144+
145 _reset() {
146 this._terms = [];
147 this._results = {};
148 this._clearDisplay();
149 this._clearSearchTimeout();
150+ this._cancelSearchProviderRequest();
151 this._defaultResult = null;
152 this._startingSearch = false;
153
154@@ -562,6 +589,13 @@ var SearchResults = new Lang.Class({
155 if (this._terms.length > 0)
156 isSubSearch = searchString.indexOf(previousSearchString) == 0;
157
158+ this._searchCancelCancellable.cancel();
159+ this._searchCancelCancellable.reset();
160+ if (this._searchCancelTimeoutId > 0) {
161+ GLib.source_remove(this._searchCancelTimeoutId);
162+ delete this._searchCancelTimeoutId;
163+ }
164+
165 this._terms = terms;
166 this._isSubSearch = isSubSearch;
167 this._updateSearchProgress();
diff --git a/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch b/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch
168deleted file mode 1006440deleted file mode 100644
index 88ae73e..0000000
--- a/debian/patches/viewSelector-Cancel-search-on-overview-hidden.patch
+++ /dev/null
@@ -1,36 +0,0 @@
1From: =?utf-8?b?Ik1hcmNvIFRyZXZpc2FuIChUcmV2acOxbyki?= <mail@3v1n0.net>
2Date: Thu, 23 Aug 2018 16:11:10 +0200
3Subject: viewSelector: Cancel search on overview hidden
4
5Currently when the overview is hidden, any pending search is kept alive, not only
6at remote search provider level (as per issue #183), but even the shell providers
7proxies continue to get and process data. This happens even if this is not needed
8anymore, while the UI reset is performed only next time that the overview is
9shown (causing some more computation presentation time).
10
11In order to stop this to happen, when the overview is hidden, we have to unset
12the search entry to an empty value as this would make SearchResults to have empty
13terms list and that would make the proxies cancellable to be triggered (without
14causing any further search to start).
15
16https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
17
18Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-shell/issues/183
19Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bionic/+source/gnome-shell/+bug/1756826
20Forwarded: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/205
21---
22 js/ui/viewSelector.js | 1 +
23 1 file changed, 1 insertion(+)
24
25diff --git a/js/ui/viewSelector.js b/js/ui/viewSelector.js
26index 91bc222..c46afc5 100644
27--- a/js/ui/viewSelector.js
28+++ b/js/ui/viewSelector.js
29@@ -311,6 +311,7 @@ var ViewSelector = new Lang.Class({
30 },
31
32 hide() {
33+ this.reset();
34 this._workspacesDisplay.hide();
35 },
36

Subscribers

People subscribed via source and target branches