Created attachment 229382 [details] [review]
key-model: Also hide subkeys on key-released

This allows users to insert subkeys with a single button press/release
pair by pressing on the main key, then moving and releasing while
hovering the subkey.

Review of attachment 229382 [details] [review]:

Looks good to me.

::: libcaribou/key-model.vala
@@ +125,2 @@
         internal void add_subkey (KeyModel key) {
             key.key_clicked.connect(on_subkey_clicked);

Perhaps icbw, is on_subkey_clicked still needed?  While it emits "key-clicked" on the main key, I think it is not so useful.

Created attachment 229493 [details] [review]
key-model: Don't emit key-clicked both on the main key and the subkey

As it is the subkey that's being clicked we shouldn't emit key-clicked
on the main key too.
--

(In reply to comment #2)
> Perhaps icbw, is on_subkey_clicked still needed?  While it emits "key-clicked"
> on the main key, I think it is not so useful.

I also think it's not needed but had decided to go the safest way as
I'm not 100% sure if this breaks something elsewhere. It certainly
makes more sense and doesn't break anything on my own testing
though. So here it is.

Created attachment 229495 [details] [review]
key-model: Also hide subkeys on key-released

--

If we go with the previous patch then the original isn't needed and
instead we should go with this.

> (In reply to comment #2)
> > Perhaps icbw, is on_subkey_clicked still needed?  While it emits "key-clicked"
> > on the main key, I think it is not so useful.
> 
> I also think it's not needed but had decided to go the safest way as
> I'm not 100% sure if this breaks something elsewhere. It certainly
> makes more sense and doesn't break anything on my own testing
> though. So here it is.

I thought that those events (key-pressed, key-released, key-clicked) are analogues to GtkButton's (i.e. button-press-event, button-release-event, and clicked (activate)).  If so, only key-clicked is meaningless and the main key should still emit key-pressed/key-released event, just IMHO.

(In reply to comment #5)
> I thought that those events (key-pressed, key-released, key-clicked) are
> analogues to GtkButton's (i.e. button-press-event, button-release-event, and
> clicked (activate)).  If so, only key-clicked is meaningless and the main key
> should still emit key-pressed/key-released event, just IMHO.

I'm puzzled now. Do you agree with the patch?

Created attachment 229725 [details] [review]
key-model: Use key-released instead of key-clicked to hide subkeys

This allows users to insert subkeys with a single button press/release
pair by pressing on the main key, then moving and releasing while
hovering the subkey.

Also, emit key-released instead of key-clicked on the main key since
that is a better model what is really happening.
--

Ok, if I understand you correctly, this also fixes things nicely.

Review of attachment 229725 [details] [review]:

Looks good to me.

Attachment 229725 [details] pushed as 134f3c3 - key-model: Use key-released instead of key-clicked to hide subkeys