Fix backward compatibility for candidate window display in mozc.el #1427
Conversation
Ensure compatibility with both old and new versions of mozc_emacs_helper by supporting both candidate window field names. Prioritize `candidate-window` and fall back to `candidates` if not present. Add debug logging output. Fixes google#1424
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
I have signed the CLA. Could you please re-run the CLA check? |
hiroyuki-komatsu
left a comment
hiroyuki-komatsu
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
src/unix/emacs/mozc.el
Outdated
| (cands (mozc-protobuf-get output 'candidates)) | ||
| (candidates (progn | ||
| (when (or cand-window cands) | ||
| (message "mozc.el: Using '%s' field for candidates" |
There was a problem hiding this comment.
Does this show a message every key event?
How about simplifying as follows?
(candidates (or (mozc-protobuf-get output 'candidate-window)
(mozc-protobuf-get output 'candidates)))There was a problem hiding this comment.
You're right, it was displaying a message on every key event. I've removed the message and simplified the code as you suggested.
src/unix/emacs/mozc.el
Outdated
| (preedit (mozc-protobuf-get output 'preedit)) | ||
| (cand-window (mozc-protobuf-get output 'candidate-window)) | ||
| (cands (mozc-protobuf-get output 'candidates)) | ||
| (candidates (progn |
There was a problem hiding this comment.
Please add a comment why candidates is also checked here (e.g. ; candiates is also checked for backward compatibility (see: #1424)).
- Simplify candidate field handling using (or ...) syntax - Remove verbose debug message - Add comment explaining backward compatibility
|
Updated per your suggestions. Tested with both old and new mozc_emacs_helper versions. |
hiroyuki-komatsu
left a comment
hiroyuki-komatsu
left a comment
There was a problem hiding this comment.
Thank you for update!
src/unix/emacs/mozc.el
Outdated
| (let ((result (mozc-protobuf-get output 'result)) | ||
| (preedit (mozc-protobuf-get output 'preedit)) | ||
| (candidates (mozc-protobuf-get output 'candidate-window))) | ||
| (let* ((result (mozc-protobuf-get output 'result)) |
There was a problem hiding this comment.
Please use let here, since let* is no longer necessary.
There was a problem hiding this comment.
You're right, I overlooked that. Fixed now.
Use let instead of let* since sequential binding is no longer needed.
2 participants
Description
This PR fixes a backward compatibility issue where mozc.el cannot display the candidate window when used with older versions of mozc_emacs_helper.
After investigating, I confirmed with the Mozc developers that the protocol change in commit ec4ff27 was simply a field rename from
candidatestocandidate-windowwith no functional changes.The fix modifies mozc.el to support both field names, ensuring it works with both old and new versions of mozc_emacs_helper without breaking existing setups.
Issue IDs
#1424
Steps to test new behaviors
Additional context
This change ensures backward compatibility by checking for
candidate-windowfirst (new protocol) and falling back tocandidates(old protocol) if not present. Debug logging has been added to help identify which protocol version is being used.