After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 756661 - [PATCH] Sync vala completion
[PATCH] Sync vala completion
Status: RESOLVED FIXED
Product: gnome-builder
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Builder Maintainers
GNOME Builder Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-10-15 18:41 UTC by Yannick Inizan
Modified: 2016-02-21 23:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] Sync vala completion (3.10 KB, patch)
2015-10-15 18:41 UTC, Yannick Inizan
none Details | Review
[PATCH] Sync vala completion (update) (3.11 KB, patch)
2015-10-16 04:12 UTC, Yannick Inizan
none Details | Review
[PATCH] Sync vala completion (update #2) (3.09 KB, patch)
2015-10-16 08:22 UTC, Yannick Inizan
none Details | Review
[PATCH] Sync vala completion (update #3) (4.69 KB, patch)
2015-10-16 08:33 UTC, Yannick Inizan
committed Details | Review

Description Yannick Inizan 2015-10-15 18:41:59 UTC
Created attachment 313393 [details] [review]
[PATCH] Sync vala completion

Sync vala completion because async management is deprecated & doesn't work.
Comment 1 Christian Hergert 2015-10-15 18:45:10 UTC
Review of attachment 313393 [details] [review]:

This is not okay to do synchronously, we can't block the UI thread. What is the preferred way to do an async call from a synchronous function?
Comment 2 Yannick Inizan 2015-10-15 19:12:35 UTC
OK. I test GLib.Thread with signal when results are ready
Comment 3 Christian Hergert 2015-10-15 19:18:52 UTC
Inside of Builder you should not use GLib.Thread directly. Manually spawning threads is slow and prevents the ability to throttle the number of outstanding requests we have in flight.

Also, it is paramount that the signal is emitted from the main thread via a GLib.Idle.add or equivalent. We got this for free before because the callback was guaranteed to be on the main thread.

It's still unclear to me what is wrong with the original code. The async stuff might generate code using deprecated GLib API (GSimpleAsyncResult, which is okay to use, just deprecated), but I don't see warnings from vala...
Comment 4 Christian Hergert 2015-10-15 19:19:31 UTC
Oops, forgot the important part:

 Use Ide.ThreadPool like the previous code instead of GLib.Thread.
Comment 5 Yannick Inizan 2015-10-16 04:12:58 UTC
Created attachment 313412 [details] [review]
[PATCH] Sync vala completion (update)
Comment 6 Christian Hergert 2015-10-16 05:28:26 UTC
Review of attachment 313412 [details] [review]:

couple things to fix. once these are done, it would be good to update indentations to match

::: plugins/vala-pack/ide-vala-completion-provider.vala
@@ -97,3 +96,3 @@
 
-				if (!cancellable.is_cancelled ()) {
-					this.results.present (this, context);
+				Idle.add (() => {
+					if (!cancellable.is_cancelled () && results.replay (query))

results.replay() should not be necessary here

@@ +97,3 @@
+				Idle.add (() => {
+					if (!cancellable.is_cancelled () && results.replay (query))
+						results.present (this, context);

We use this. to be clear about where the scope came from. (so this.results). It's verbose, but handy during code reviews.
Comment 7 Yannick Inizan 2015-10-16 08:22:23 UTC
Created attachment 313419 [details] [review]
[PATCH] Sync vala completion (update #2)

[PATCH] Sync vala completion (update #2)
Comment 8 Yannick Inizan 2015-10-16 08:33:26 UTC
Created attachment 313423 [details] [review]
[PATCH] Sync vala completion (update #3)
Comment 9 Yannick Inizan 2015-10-16 08:34:36 UTC
patch updated with completion inside lambda expressions
Comment 10 Christian Hergert 2015-10-18 16:25:28 UTC
Pushed with an additional commit for whitespace cleanup.
Comment 11 Christian Hergert 2015-10-18 16:25:54 UTC
Thanks for working on this!
Comment 12 Ben 2015-10-18 17:13:20 UTC
FYI neither vala's async functions, nor .begin, nor .end, are deprecated
Comment 13 Christian Hergert 2015-10-18 19:08:38 UTC
If that is the case, I would like to keep everything async in IdeValaIndex.

Yannick, where there any other reasons we needed to switch that code?