tests/ibus-engine-switch: Don't try to remove non-existing GSource

tests/ibus-engine-switch: Don't try to remove non-existing GSource

ibus-engine-switch schedules an idle doing an engine switch, and
also schedules a timeout after 1 microsecond which exits the main
loop. Both of these callbacks will return FALSE, which detaches
the associated GSource from the mainloop and destroys it.

As the only way to exit the mainloop is when the timeout callback runs,
we should not try to run g_source_remove(timeout_id) as it will already
be gone, and this causes a warning.

This commit reorders a bit to make the ordering between the idle and
timeout more obvious, and it gets rid of the calls to g_source_remove()
as they are needed.

BUG=https://github.com/ibus/ibus/issues/1868

Committed: 20068d9d0731b580d6d8218b97c482c0558149dd

[Peng, 2014-06-05 14:22:25]

https://codereview.appspot.com/103100044/diff/1/src/tests/ibus-engine-switch.c
File src/tests/ibus-engine-switch.c (right):

https://codereview.appspot.com/103100044/diff/1/src/tests/ibus-engine-switch....
src/tests/ibus-engine-switch.c:85: g_idle_add ((GSourceFunc)
change_global_engine_cb, NULL);
I think the timeout_cb is for verifying global_engine_changed_cb will happen in
1 second.

If we don't have the timeout_cb, and the global-engine-changed signal does not
happen, the test will run forever.

[fujiwara, 2014-06-06 08:13:33]

I noticed you created the updated patch of
v4-0001-tests-ibus-engine-switch-Don-t-try-to-remove-non-.patch.
Would you create a new code review with git-cl?
I will drop this CL later.

[fujiwara, 2014-06-09 07:09:50]

https://codereview.appspot.com/103100044/diff/1/src/tests/ibus-engine-switch.c
File src/tests/ibus-engine-switch.c (right):

https://codereview.appspot.com/103100044/diff/1/src/tests/ibus-engine-switch....
src/tests/ibus-engine-switch.c:85: g_idle_add ((GSourceFunc)
change_global_engine_cb, NULL);
On 2014/06/05 14:22:25, Peng wrote:
> I think the timeout_cb is for verifying global_engine_changed_cb will happen
in
> 1 second.
> 
> If we don't have the timeout_cb, and the global-engine-changed signal does not
> happen, the test will run forever.

Updated the patch regarding to
http://code.google.com/p/ibus/issues/detail?id=1712#c9

[Peng, 2014-06-09 11:47:13]

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
File src/tests/ibus-engine-switch.c (right):

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
src/tests/ibus-engine-switch.c:62: ibus_quit ();
Should we fail the test if timeout happens?

[teuf, 2014-06-10 07:03:06]

On 2014/06/09 07:09:50, fujiwara wrote:
> Updated the patch regarding to
> http://code.google.com/p/ibus/issues/detail?id=1712#c9

Thanks for updating the patch!

[teuf, 2014-06-10 07:03:13]

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
File src/tests/ibus-engine-switch.c (right):

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
src/tests/ibus-engine-switch.c:62: ibus_quit ();
On 2014/06/09 11:47:13, Peng wrote:
> Should we fail the test if timeout happens?

At the moment, this timeout is the only way to exit the mainloop started in
test_global_engine().
We could have a g_assert_not_reached() in timeout_cb if we add an ibus_quit()
call to the end of change_global_engine_cb(). However, maybe the mainloop needs
to run for a bit after we try to change the engine?

[Peng, 2014-06-10 14:32:10]

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
File src/tests/ibus-engine-switch.c (right):

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
src/tests/ibus-engine-switch.c:62: ibus_quit ();
On 2014/06/10 07:03:13, teuf wrote:
> On 2014/06/09 11:47:13, Peng wrote:
> > Should we fail the test if timeout happens?
> 
> At the moment, this timeout is the only way to exit the mainloop started in
> test_global_engine().
> We could have a g_assert_not_reached() in timeout_cb if we add an ibus_quit()
> call to the end of change_global_engine_cb(). However, maybe the mainloop
needs
> to run for a bit after we try to change the engine?

At least, we need check data->count to make sure global_engine_changed_cb has
been called several times which we expect.

[teuf, 2014-06-16 11:21:46]

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
File src/tests/ibus-engine-switch.c (left):

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
src/tests/ibus-engine-switch.c:90: g_assert_cmpint (data.count, ==, G_N_ELEMENTS
(engine_names));
The data.count check mentioned above is here.

[fujiwara, 2016-07-22 05:12:25]

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
File src/tests/ibus-engine-switch.c (right):

https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
src/tests/ibus-engine-switch.c:62: ibus_quit ();
On 2014/06/10 14:32:10, Peng wrote:
> On 2014/06/10 07:03:13, teuf wrote:
> > On 2014/06/09 11:47:13, Peng wrote:
> > > Should we fail the test if timeout happens?
> > 
> > At the moment, this timeout is the only way to exit the mainloop started in
> > test_global_engine().
> > We could have a g_assert_not_reached() in timeout_cb if we add an
ibus_quit()
> > call to the end of change_global_engine_cb(). However, maybe the mainloop
> needs
> > to run for a bit after we try to change the engine?
> 
> At least, we need check data->count to make sure global_engine_changed_cb has
> been called several times which we expect.

Done.

[Peng, 2016-07-22 11:19:03]

On 2016/07/22 05:12:25, fujiwara wrote:
>
https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
> File src/tests/ibus-engine-switch.c (right):
> 
>
https://codereview.appspot.com/103100044/diff/20001/src/tests/ibus-engine-swi...
> src/tests/ibus-engine-switch.c:62: ibus_quit ();
> On 2014/06/10 14:32:10, Peng wrote:
> > On 2014/06/10 07:03:13, teuf wrote:
> > > On 2014/06/09 11:47:13, Peng wrote:
> > > > Should we fail the test if timeout happens?
> > > 
> > > At the moment, this timeout is the only way to exit the mainloop started
in
> > > test_global_engine().
> > > We could have a g_assert_not_reached() in timeout_cb if we add an
> ibus_quit()
> > > call to the end of change_global_engine_cb(). However, maybe the mainloop
> > needs
> > > to run for a bit after we try to change the engine?
> > 
> > At least, we need check data->count to make sure global_engine_changed_cb
has
> > been called several times which we expect.
> 
> Done.

lgtm

[fujiwara, 2016-07-25 02:42:51]

Committed patchset #3 (id:40001) manually as
20068d9d0731b580d6d8218b97c482c0558149dd (presubmit successful).