Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WindowBase::setImePreEditPosition #2469

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Edgaru089
Copy link
Contributor

@Edgaru089 Edgaru089 commented Mar 24, 2023

Description

This PR adds a method to WindowBase, setImePreEditPosition(const Vector2i& pos), that hints to the OS/IME framework where to put the preedit window.

Tasks

  • Tested on Linux (Fcitx5)
  • Tested on Linux (IBus)
  • Tested on Windows
  • Tested on macOS (Not implemented)

I'm completely illiterate when it comes to macOS/Objective-C (or any other Apple stuff), and I need help on that part.

How to test this PR?

Install an IME if you don't have one, and run this example.

This is relatively straightforward on Windows 10: install a language of Chinese Simplified/Traditional or Japanese in the Settings app, and press Win+Space to switch to that input.

#include <SFML/Graphics.hpp>

int main() {
    sf::RenderWindow window(sf::VideoMode({ 400, 400 }), "IME position");
    window.setFramerateLimit(60);

    while (window.isOpen()) {
        for (sf::Event event; window.pollEvent(event);) {
            if (event.type == sf::Event::Closed)
                window.close();
            else if (event.type == sf::Event::MouseButtonPressed) {
                auto pos = sf::Mouse::getPosition(window);
                window.setImePreEditPosition({pos.x, pos.y});  // Try negating these values and see what happens    
            }
        }

        window.clear();
        window.display();
    }
}

Click around in the opened window, and try typing with the IME engaged.
图片

@Edgaru089
Copy link
Contributor Author

This patch also works with IBus on a fresh Ubuntu VM.
图片

src/SFML/Window/OSX/WindowImplCocoa.mm Outdated Show resolved Hide resolved
src/SFML/Window/Unix/WindowImplX11.cpp Outdated Show resolved Hide resolved
src/SFML/Window/Win32/WindowImplWin32.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Small note: we generally use PascalCase for abbreviations (Utf, UdpSocket, Http, AlResource).

This would mean IMEPreeditPosition should be ImePreeditPosition.

(Strictly speaking, WindowImplDRM is also not consistent, but that's out of scope for this PR).

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #2469 (5e9c13c) into master (9105148) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2469      +/-   ##
==========================================
- Coverage   38.54%   38.49%   -0.05%     
==========================================
  Files         229      229              
  Lines       19806    19834      +28     
  Branches     4733     4737       +4     
==========================================
+ Hits         7635     7636       +1     
- Misses      11147    11243      +96     
+ Partials     1024      955      -69     
Files Changed Coverage Δ
include/SFML/Window/WindowBase.hpp 0.00% <ø> (ø)
src/SFML/Window/Unix/WindowImplX11.cpp 32.62% <0.00%> (-0.14%) ⬇️
src/SFML/Window/Win32/WindowImplWin32.cpp 25.21% <0.00%> (-0.34%) ⬇️
src/SFML/Window/Win32/WindowImplWin32.hpp 83.33% <ø> (ø)
src/SFML/Window/WindowBase.cpp 62.32% <0.00%> (-1.49%) ⬇️
src/SFML/Window/WindowImpl.hpp 50.00% <ø> (ø)
src/SFML/Window/iOS/WindowImplUIKit.mm 0.00% <0.00%> (ø)
src/SFML/Window/macOS/WindowImplCocoa.mm 23.90% <0.00%> (-0.20%) ⬇️

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9105148...5e9c13c. Read the comment docs.

@Edgaru089
Copy link
Contributor Author

Issues fixed and rebased to latest master.

@eXpl0it3r eXpl0it3r changed the title Add WindowBase::setIMEPreeditPosition Add WindowBase::setImePreeditPosition Aug 29, 2023
@eXpl0it3r
Copy link
Member

eXpl0it3r commented Aug 29, 2023

Is preedit really the most commonly referred to name for that window?
I like the "Composition" term that the Windows API uses "nicer", mostly wonder though what term IME users generally use.

If we stick with preedit, I suggest to use PreEdit instead: setImePreeditPosition vs setImePreEditPosition

PR needs to be rebased

@Edgaru089 Edgaru089 changed the title Add WindowBase::setImePreeditPosition Add WindowBase::setImePreEditPosition Sep 19, 2023
@Edgaru089
Copy link
Contributor Author

Edgaru089 commented Sep 19, 2023

Rebased to latest master.

I won't prefer the word 'Composition'. Please correct me if I'm wrong, but in my understanding the word more describes the use of the Compose key to combine accent marks with Latin alphabet in some European languages. IMEs are something quite different.

People around me call that window something like 'input method window' or '(input method) selection pending box'. I'm from China mainland btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants