Unity

Merge lp://qastaging/~mhr3/unity/background-icon-loading into lp://qastaging/unity

background-icon-loading
Merge into trunk

Proposed by Michal Hruby on 2012-01-19

Status:

Merged

Approved by:

Gord Allott on 2012-01-25

Approved revision:

no longer in the source branch.

Merged at revision:

1860

Proposed branch:

lp://qastaging/~mhr3/unity/background-icon-loading

Merge into:

lp://qastaging/unity

Diff against target:

501 lines (+285/-89)

3 files modified

plugins/unityshell/src/IconLoader.cpp (+89/-89)
tests/CMakeLists.txt (+5/-0)
tests/test_icon_loader.cpp (+191/-0)

To merge this branch:

bzr merge lp://qastaging/~mhr3/unity/background-icon-loading

High

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Gord Allott (community)		Approve on 2012-01-25
Sam Spilsbury (community)	2012-01-19	Approve on 2012-01-21
Review via email: mp+89233@code.qastaging.launchpad.net

Description of the change

Move all icon loading into background gio threads. (there was mainly issue with gtk_icon_info_load_icon which is doing synchronous IO and noticeably blocks the UI when using some themes)

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-01-21:

All ok on the code and testing.

161 + // careful here this is running in non-main thread
162 + if (task->icon_info)
163 + {

Maybe here you might want to use mutexes to synchronize the threads ? I'm not sure about the exact semantics of g_io_scheduler_push_job - if it processes jobs in serial or parallel.

review: Approve

Revision history for this message

Michal Hruby (mhr3) wrote on 2012-01-23:

> All ok on the code and testing.
>
> 161 + // careful here this is running in non-main thread
> 162 + if (task->icon_info)
> 163 + {
>
> Maybe here you might want to use mutexes to synchronize the threads ? I'm not
> sure about the exact semantics of g_io_scheduler_push_job - if it processes
> jobs in serial or parallel.

There is no synchronization needed, once this method starts icon_info is either already set to valid GtkIconInfo instance (now owned by the task), or NULL in case of URI request. This is merely a reminder for anyone wanting to touch that code.

As for serial vs parallel - it's both, since GIOScheduler uses a thread pool.

Revision history for this message

Gord Allott (gordallott) on 2012-01-25:

review: Approve