Indicator keyboard

Merge lp://staging/~attente/indicator-keyboard/fcitx-transition into lp://staging/indicator-keyboard

  1. fcitx-transition
  2. Merge into trunk
Proposed by William Hua
Status: Merged
Approved by: Sebastien Bacher
Approved revision: 434
Merged at revision: 670
Proposed branch: lp://staging/~attente/indicator-keyboard/fcitx-transition
Merge into: lp://staging/indicator-keyboard
Diff against target: 1817 lines (+450/-796)
17 files modified
.bzrignore (+11/-8)
configure.ac (+1/-32)
data/Makefile.am (+4/-4)
debian/control (+2/-0)
deps/Fcitx-1.0.metadata (+7/-0)
deps/GnomeDesktop-3.0.metadata (+1/-0)
deps/accountsservice.vapi (+0/-151)
deps/fontconfig.vapi (+0/-13)
deps/freetype2.vapi (+0/-20)
deps/gnome-desktop-3.0.vapi (+0/-320)
deps/pangoft2.vapi (+0/-33)
lib/Makefile.am (+5/-2)
lib/ibus-menu.vala (+13/-17)
lib/indicator-menu.vala (+50/-43)
lib/main.vala (+249/-94)
lib/source.vala (+106/-59)
tests/indicator-keyboard-test.in (+1/-0)
To merge this branch: bzr merge lp://staging/~attente/indicator-keyboard/fcitx-transition
Reviewer Review Type Date Requested Status
desrt (community) Approve
Sebastien Bacher Approve
PS Jenkins bot (community) continuous-integration Approve
Ted Gould (community) Approve
csslayer Pending
Review via email: mp+229737@code.staging.launchpad.net

Commit message

Basic support for Fcitx input sources.

Description of the change

Basic support for Fcitx input sources.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:409
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/69/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-amd64-ci/2/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/2
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/2/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/69/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:411
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/70/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/3/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/70/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:412
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/71/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/4/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/71/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:413
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/72/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-amd64-ci/5
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/5
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/5/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/72/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:420
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/73/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-amd64-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-utopic-armhf-ci/6/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/73/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

So I read through this and I don't see any issues, but I don't really know how fcitx works, so any of the interactions are not known by me. Not sure if there's someone better to review this or not.

Revision history for this message
Ted Gould (ted) wrote :

I'll approve, but not top-approve in hoping there's someone else who can review better.

review: Approve
Revision history for this message
Ted Gould (ted) wrote :

Talking with Will this is going to have to be a 15.04 feature. I'm marking the review as a WIP as it'll need to be resubmitted to the 15.04 branch anyway.

Revision history for this message
William Hua (attente) wrote :

Hi Ted, could we please reconsider this for approval now that the Fcitx MIR is complete? I'll see if I can find another reviewer for it, maybe from the Kylin team.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:422
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/74/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/1
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/1/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/74/rebuild

review: Approve (continuous-integration)
Revision history for this message
desrt (desrt) wrote :

Without really appreciating what's going on with the input method side of this patch, see below about some code comments.

review: Needs Fixing
lp://staging/~attente/indicator-keyboard/fcitx-transition updated
423. By William Hua

Remove unnecessary AC_SUBST.

424. By William Hua

Use shift operator.

425. By William Hua

Check flags better.

426. By William Hua

Only try initializing Fcitx once.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:426
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/75/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/2
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/2/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/75/rebuild

review: Approve (continuous-integration)
lp://staging/~attente/indicator-keyboard/fcitx-transition updated
427. By William Hua

Remove (!) when possible.

428. By William Hua

Use foreach when possible.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:428
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/76/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/3/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/76/rebuild

review: Approve (continuous-integration)
lp://staging/~attente/indicator-keyboard/fcitx-transition updated
429. By William Hua

Remove vapi.

430. By William Hua

Remove unnecessary flag.

We don't really need this flag any more since we decided to hide indicator-keyboard in the session when Fcitx is running since it has its own indicator.

Revision history for this message
William Hua (attente) wrote :

Thanks, I think that's all of the necessary changes, including removing the generated vapi files.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:430
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/77/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/4/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/77/rebuild

review: Needs Fixing (continuous-integration)
lp://staging/~attente/indicator-keyboard/fcitx-transition updated
431. By William Hua

Build depend on gir1.2-fcitx-1.0.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:431
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/78/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-amd64-ci/5
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/5
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/5/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/78/rebuild

review: Approve (continuous-integration)
Revision history for this message
desrt (desrt) wrote :

There's nothing wrong here, really -- just a few nitpicks. I wouldn't let it block the merge, or anything...

Revision history for this message
William Hua (attente) wrote :

I filed https://bugzilla.gnome.org/show_bug.cgi?id=744797 for the foreach+(!) issue.

Revision history for this message
William Hua (attente) wrote :

I don't think we ever scroll input sources by more than 1 at a time, but it's exposed via org.gtk.Actions over D-Bus, so I did that mod just in case. I'll re-write it though to explain why, and fix it so we only need one loop.

lp://staging/~attente/indicator-keyboard/fcitx-transition updated
432. By William Hua

Don't handle activate signal when the default behaviour will do.

433. By William Hua

Simplify input source cycling.

434. By William Hua

Explain input source cycling logic.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:434
http://jenkins.qa.ubuntu.com/job/indicator-keyboard-ci/79/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-amd64-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/indicator-keyboard-vivid-armhf-ci/6/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-keyboard-ci/79/rebuild

review: Approve (continuous-integration)
Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks, that looks fine, let's get that in a silo on monday, with u-c-c and u-s-d

review: Approve
Revision history for this message
desrt (desrt) wrote :

No further comments.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: