Mir

Merge lp://qastaging/~vanvugt/mir/modifier-groups into lp://qastaging/mir

modifier-groups
Merge into development-branch

Proposed by Daniel van Vugt on 2015-01-16

Status:	Work in progress
Proposed branch:	lp://qastaging/~vanvugt/mir/modifier-groups
Merge into:	lp://qastaging/mir
Diff against target:	122 lines (+39/-8) 6 files modified client-ABI-sha1sums (+1/-1) common-ABI-sha1sums (+1/-1) include/common/mir_toolkit/events/input/input_event.h (+12/-4) platform-ABI-sha1sums (+1/-1) server-ABI-sha1sums (+1/-1) tests/unit-tests/input/test_input_event.cpp (+23/-0)
To merge this branch:	bzr merge lp://qastaging/~vanvugt/mir/modifier-groups
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Robert Carr (community)			Abstain on 2015-01-20
Daniel van Vugt			Needs Fixing on 2015-01-20
Alan Griffiths			Needs Information on 2015-01-19
Alexandros Frantzis (community)			Needs Information on 2015-01-19
PS Jenkins bot (community)	continuous-integration		Approve on 2015-01-19
Cemil Azizoglu (community)		2015-01-16	Approve on 2015-01-16
Review via email: mp+246676@code.qastaging.launchpad.net

Commit message

Fix wasteful allocation of modifier bit flags that allowed for impossible combinations.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2677/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-01-16:

Wouldn't

mir_input_event_modifier_alt = mir_input_event_modifier_alt_left | mir_input_event_modifier_alt_right,

have been better?

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-19:

I thought the same thing afterwards...

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-19:

Just did a quick audit. This stuff appeared in r2129 so first released in Mir 0.10.0. I think that makes it new enough that we can safely change the API without anyone complaining.

lp://qastaging/~vanvugt/mir/modifier-groups updated on 2015-01-19

2233. By Daniel van Vugt on 2015-01-19: Merge latest trunk
2234. By Daniel van Vugt on 2015-01-19: Changed syntax per Cemil's comments.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-19:

Changed to the syntax Cemil mentioned. It's a little bit more ugly but also more verbose. Don't know if that's better or worse.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2690/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-01-19:

OK with this as long as there is no good reason for having separate alt/shift/ctrl values in the first place. I vaguely remember some discussions about this, but not the conclusion. Robert?

Also, since this is a breaking change anyway, why not reorganize the enum to avoid holes?

> This stuff appeared in r2129 so first released in Mir 0.10.0. I think that makes it
> new enough that we can safely change the API without anyone complaining.

That's only true as long as no one uses 0.10 and then moves to a later version. I don't think we have the luxury to skip bumps in the client ABI as we have been doing on some occasions on the server side.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-01-19:

*Needs Discussion*

I know this example is a different enum, but do we plan to support the equivalent of:

            switch (event.modifiers & modifier_mask)
            {
            case mir_key_modifier_alt:
                wm->toggle(mir_surface_state_maximized);
                return true;

(from examples/server_example_window_management.cpp)

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-20:

Alexandros: Yeah we could fill the holes but I felt that would increase the risk of ABI breaks if all the bits moved instead of just four of them. Same kind of risk, but more limited.

Alan: Your example is certainly unusual and does expect mir_key_modifier_alt to be a single bit (or for the user to hold down both Alts at once :). I don't think that's common usage. More commonly mir_key_modifier_alt will be used as a mask. So in the long run I suspect the masks proposed here are going to be more useful. That said, this is not a critical change either.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-01-20:

Alan's example code needs modification if this branch is to land. But I'm undecided now.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-01-20:

I'm not sure. I suspect both series of flags are inadequate and eventually we will have to use a dynamic modifier system similar to xkb_modmask_t (this can of course be backwards compatible with either this branch or the current devel branches).

I would say its not worth the churn but I didn't think very hard.

review: Abstain

Unmerged revisions

2234. By Daniel van Vugt on 2015-01-19: Changed syntax per Cemil's comments.
2233. By Daniel van Vugt on 2015-01-19: Merge latest trunk
2232. By Daniel van Vugt on 2015-01-16: Fix search/replace error, and improve formatting
2231. By Daniel van Vugt on 2015-01-16: Ensure each modifier with separate left/right keys doesn't get three
separate flag bits. So that one may test for:
mods & mir_input_event_modifier_shift
without needing to know which shift key it is.