GNOME Bugzilla – Bug 756661
[PATCH] Sync vala completion
Last modified: 2016-02-21 23:01:12 UTC
Created attachment 313393 [details] [review] [PATCH] Sync vala completion Sync vala completion because async management is deprecated & doesn't work.
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?
OK. I test GLib.Thread with signal when results are ready
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...
Oops, forgot the important part: Use Ide.ThreadPool like the previous code instead of GLib.Thread.
Created attachment 313412 [details] [review] [PATCH] Sync vala completion (update)
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.
Created attachment 313419 [details] [review] [PATCH] Sync vala completion (update #2) [PATCH] Sync vala completion (update #2)
Created attachment 313423 [details] [review] [PATCH] Sync vala completion (update #3)
patch updated with completion inside lambda expressions
Pushed with an additional commit for whitespace cleanup.
Thanks for working on this!
FYI neither vala's async functions, nor .begin, nor .end, are deprecated
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?