Skip to content

feat: redesign candidate comment layout with TOP/RIGHT/OVERLAY modes#1963

Merged
Bambooin merged 1 commit intoosfans:developfrom
ENOA-REIAH-UION:comment
Mar 30, 2026
Merged

feat: redesign candidate comment layout with TOP/RIGHT/OVERLAY modes#1963
Bambooin merged 1 commit intoosfans:developfrom
ENOA-REIAH-UION:comment

Conversation

@ENOA-REIAH-UION
Copy link
Copy Markdown
Contributor

Pull request

Issue tracker

Fixes will automatically close the related issues

Fixes #
Fixes #

Feature

Describe features of this pull request

Code of conduct

Code style

Build pass

  • make debug

Manually test

  • Done

Code Review

  1. No wildcards import
  2. Manual build and test pass
  3. GitHub Action CI pass
  4. At least one contributor review and approve
  5. Merged clean without conflicts
  6. PR will be merged by rebase upstream base

Daily build

Login and download artifact at https://github.com/osfans/trime/actions

Additional Info

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Redesigns how candidate comments are laid out in the IME candidate item UI by replacing the legacy boolean toggle with explicit positioning modes driven by theme config.

Changes:

  • Replaces comment_on_top with comment_position supporting RIGHT, TOP, and OVERLAY.
  • Adds candidate_text_vertical_bias and comment_vertical_bias to tune overlay positioning.
  • Updates shared and test theme YAMLs and adjusts the theme decode default / expectations.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
app/src/main/java/com/osfans/trime/ime/candidates/CandidateItemUi.kt Implements TOP/RIGHT/OVERLAY constraint layouts for candidate text + comment.
app/src/main/java/com/osfans/trime/data/theme/model/GeneralStyle.kt Updates theme model/decoder: removes commentOnTop, adds biases, changes commentPosition defaults.
app/src/main/assets/shared/trime.yaml Documents and sets new default style keys for comment positioning and overlay biases.
app/src/main/assets/shared/tongwenfeng.trime.yaml Updates preset theme to new comment positioning keys and defaults.
app/src/test/assets/trime.yaml Updates test theme asset to use comment_position: right and removes comment_on_top.
app/src/test/java/com/osfans/trime/data/theme/GeneralStyleTest.kt Updates default expectation for commentPosition to RIGHT.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Bambooin
Copy link
Copy Markdown
Collaborator

Found two small UI issues with this patch.

  • is truncated.
  • Switcher is covered

truncated

@ENOA-REIAH-UION
Copy link
Copy Markdown
Contributor Author

Found two small UI issues with this patch.

  • is truncated.
  • Switcher is covered

truncated

Thanks for the report.

These issues do not appear to be related to this patch. This PR only modifies the styling of candidate text and comments, and does not touch ToolButton or its layout.

The truncated and the covered switcher are likely caused by other layout constraints or pre-existing issues.

@Bambooin
Copy link
Copy Markdown
Collaborator

I double checked that the issue shows up in this patch.

The commit 75d6ace in main branch is fine.

You can rebase your branch and build it to verify it.

@ENOA-REIAH-UION
Copy link
Copy Markdown
Contributor Author

I double checked that the issue shows up in this patch.

The commit 75d6ace in main branch is fine.

You can rebase your branch and build it to verify it.

I double checked that the issue shows up in this patch.

The commit 75d6ace in main branch is fine.

You can rebase your branch and build it to verify it.

Perhaps we should re-test this patch itself rather than the main branch. As mentioned, this PR only modifies candidate text and comment styling, and does not involve ToolButton.

@Bambooin
Copy link
Copy Markdown
Collaborator

I had verified the main branch and the branch in this patch.

The main branch is fine, the issue shows up in this branch.

Could you reproduce it in this branch? If you couldn't reproduce it, maybe I missed something.

@ENOA-REIAH-UION
Copy link
Copy Markdown
Contributor Author

I had verified the main branch and the branch in this patch.

The main branch is fine, the issue shows up in this branch.

Could you reproduce it in this branch? If you couldn't reproduce it, maybe I missed something.

I tested on my device, emulator, and had a friend test as well — none of us could reproduce the issue.

Copy link
Copy Markdown
Collaborator

@Bambooin Bambooin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In LineageOS 23.2, the issue shows up, let's other people test it.

@Bambooin Bambooin merged commit 65a2073 into osfans:develop Mar 30, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants