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

[cs_pinyin] CS-Pinyin keyboard initial public version #64

Merged
merged 22 commits into from
Aug 1, 2024

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Jul 27, 2017

This is a public version of the CS-Pinyin keyboard (from keyman-keyboards-internal). It contains all the files necessary to build the keyboard and the DLLs that go with it.

I will be checking the build agent to ensure that it can build the DLLs. We have checked in builds of the DLLs so that we don't introduce a Windows dependency for building the keyboards repo.

@DavidLRowe DavidLRowe self-assigned this Aug 1, 2017
Copy link
Contributor

@DavidLRowe DavidLRowe left a comment

Choose a reason for hiding this comment

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

README.md: Replace "DESCRIPTION" with description.

HISTORY.md: "Template" in first line should be changed to project name (or deleted).

\source\images*.bmp: Names of some files contain upper case letters. Not sure if this is a potential problem or not.

\source\docs: Are "About the Four Corner Method.pdf" and "The Four Corner Index Lookup Method.pdf" distinct documents?

\source\docs: "Using Simplified Chinese.pdf" contains references to Keyman version 6, and to tavultesoft.com

\source\docs: Are "Using Simplified Chinese.pdf" and "Using the CS-Pinyin IMX.pdf" distinct documents?

\source\images\CS-Pinyin Install.bmp: claims "© 2011 Tavultesoft"

"Readme" tab of installation package dialog references "copyright 2003-2011 by Tavultesoft", and tavultesoft.com (In contrast, welcome.htm has "copyright 2017 SIL International".)

Running build.cmd failed until a build folder was created manually.

Installing with Keyman Configuration seems to add "Uninstall Simplified Chinese" shortcut to start menu.

@mcdurdin
Copy link
Member Author

mcdurdin commented Aug 7, 2017

  • .cfx file has the wrong name (should match keyboard base name).
  • Even when .cfx is named correctly, the keyboard isn't working.

@LornaSIL
Copy link
Contributor

When I try to build I get this error:

cs_pinyin.kps: Warning: Warning: File ...\keyboards\cs_pinyin\source\..\build\KeymnIMX.dll does not exist.
cs_pinyin.kps: Warning: Warning: File ...\keyboards\cs_pinyin\source\..\build\imxconfig.exe does not exist.
cs_pinyin.kps: Warning: Warning: File ...\keyboards\cs_pinyin\source\..\build\KeymnIMX.x64.dll does not exist.

I think the .kps is looking in the wrong place for the .dlls?

A lot of the documentation still talks about Tavultesoft and Keyman 6.

@mcdurdin
Copy link
Member Author

I've just added the work-in-progress label to clarify that this keyboard is not yet really ready for review. The content was put online to give another developer access to the source for their own purposes and I will continue to prepare this for release as time permits.

Do you think we should close the PR and reopen it later? Or just leave it open as a hint to yours truly that something should be done about it?

@LornaSIL
Copy link
Contributor

LornaSIL commented Oct 2, 2017

The work in progress label is helpful.

@MakaraSok
Copy link
Collaborator

Missing files in the package have been copied from the legacy .kmp and put in the /build folder. Those files are: imxconfig.exe, KeymnIMX.dll and KeymnIMX.x64.dll.

The tweaks fix the errors, but the keyboard doesn't function well yet. It needs further investigation.

Also, the content of the README.md and CS-Pinyin ReadMe.html have been updated.

@mcdurdin
Copy link
Member Author

mcdurdin commented Oct 26, 2017 via email

@MakaraSok MakaraSok changed the title CS-Pinyin keyboard initial public version [cs_pinyin] CS-Pinyin keyboard initial public version Feb 13, 2018
@mcdurdin mcdurdin added the new label Feb 25, 2018
@DavidLRowe
Copy link
Contributor

@mcdurdin Two years ago you wrote: "Do you think we should close the PR and reopen it later? Or just leave it open as a hint to yours truly that something should be done about it?" How well has that hint been working? ;)

@LornaSIL
Copy link
Contributor

ask me again in two years. Currently my to do list is probably about 5 years long and getting longer.

Four years ago on June 18, 2020 you asked us to ask you in 2 years. Is there any likelihood this will be done @mcdurdin?

@rc-swag
Copy link
Contributor

rc-swag commented Jun 19, 2024

Four years ago on June 18, 2020 you asked us to ask you in 2 years. Is there any likelihood this will be done @mcdurdin?

Well, I have fixed a problem with it when you open more than one application at a time. However, the main issue the first point of my TODO above is the main sticking point it needs to build the dll. I think it will be done as we get the single repo keyboards?

@mcdurdin
Copy link
Member Author

mcdurdin commented Jul 2, 2024

@rc-swag and I will be discussing cs-pinyin this week with the aim of getting something published, even if it is not optimal. (A full IME/picker rework is on the future roadmap)

Build the relevant .dll and .exe files, include them in source/ so that
we don't need to rebuild them every time, and reorganize files ready for
Keyman 17.
All the global variables should be thread local. This probably needs
more review. The `EnumThreadWindows` call in `FindGlyph` should be
eliminated, as we can just look at the `hwnd` variable.
@mcdurdin
Copy link
Member Author

mcdurdin commented Jul 5, 2024

Is there any likelihood this will be done @mcdurdin?

It's nearly done now... we should be able to get it over the line soon I hope!

@DavidLRowe
Copy link
Contributor

I get it, @mcdurdin : You're holding out for a few more weeks to make it a seven-year PR!

@LornaSIL
Copy link
Contributor

LornaSIL commented Jul 5, 2024

  • Could you delete keyboard_info?
  • In LICENSE.md please extend the copyright date 2017-2024 - actually it should be 2003-2024.
  • In HISTORY.md are you interested in bumping the date to 2024 and our goal could be a release on July 27 :)
  • README.md could have copyright and version removed.
  • Could you change the encoding for .kmn from ANSI to UTF-8?
    • Please remove copyright year in .kmn
  • In the .kps, Details you could remove the version and tick "Package version follows keyboard version"
    • Please remove copyright date or extend it to 2024.
  • CS-Pinyin Install.bmp needs updating to remove the "(c) 2011 Tavultesoft"
  • welcome.htm could have the copyright information removed
  • CS-Pinyin ReadMe.html it would be wise to rename this to readme.htm since it has spaces and casing. Then it needs updating in the .kps.
    • This file could also have the copyright information removed

- Since this is exactly the same name as the keyboard in legacy, there should not be a deprecation, the folder in legacy should be deleted.

@LornaSIL
Copy link
Contributor

LornaSIL commented Jul 5, 2024

Since this is exactly the same name as the keyboard in legacy, there should not be a deprecation, the folder in legacy should be deleted.

Scratch that. It has a different folder name. So, the .kps file needs to be updated to deprecate the legacy\c\cs-pinyin keyboard.

@LornaSIL
Copy link
Contributor

LornaSIL commented Jul 5, 2024

I've done minor testing of the keyboard in Word 2019. It seems to be working, even in Github comments (特命). I'm not sure how else to test.

I wondered if you were wanting to address this issue? #219

Also, all the documentation (pdfs) are outdated, but I suspect that is out of scope for this current fix.

This change has several fixes.
1) Multiple monitor support. If the primary monitor is not the left most
   monitor then 'screencordinates' can be negative. The IMX windows now
   displays on all monitors. The IMX window can now moved (click drag)
   on all monitors.
2) Remove the EnumThreadWindows as the windowhandle was already known.
3) UpdateWindow is has correct calculation (sign was wrong) and also incorrectly
   set -ve x values to zero
The imxconfig.cpp and KeymnIMX.cpp files had a different location
defined for accessing the configuration keys. This resulted in the
configuration being updated but not read correctly by the IMX dll.
Ideally the read and write functions should also be harmonised.
However, refactoring this code should soon become a yak shave.
@mcdurdin
Copy link
Member Author

I wondered if you were wanting to address this issue? #219

This should definitely be fixed in a separate PR!

Also, all the documentation (pdfs) are outdated, but I suspect that is out of scope for this current fix.

Yes, I think we'll aim to update documentation separately as well.

For remaining items, @rc-swag are you able to cover those? (What else are we waiting on for publishing this?)

@rc-swag
Copy link
Contributor

rc-swag commented Jul 31, 2024

For remaining items, @rc-swag are you able to cover those? (What else are we waiting on for publishing this?)

Yes I can address those items @LornaSIL raised. The last two commits address the functional issues with the window ids, multi-monitor support and finally the registry settings.

@rc-swag
Copy link
Contributor

rc-swag commented Jul 31, 2024

  • - Could you delete keyboard_info?
  • - In LICENSE.md please extend the copyright date 2017-2024 - actually it should be 2003-2024.
  • - In HISTORY.md are you interested in bumping the date to 2024 and our goal could be a release on July 27 :)
  • - README.md could have copyright and version removed.
  • - Could you change the encoding for .kmn from ANSI to UTF-8?
  • - Please remove copyright year in .kmn
  • - In the .kps, Details you could remove the version and tick "Package version follows keyboard version"
  • - Please remove copyright date or extend it to 2024.
  • - CS-Pinyin Install.bmp needs updating to remove the "(c) 2011 Tavultesoft"
  • [ ] - welcome.htm could have the copyright information removed
  • - CS-Pinyin ReadMe.html it would be wise to rename this to readme.htm since it has spaces and casing. Then it needs updating in the .kps.
  • [ ] - This file could also have the copyright information removed
  • Create issue for updating pdf documentation chore: CS Pinyin documentation (pdfs) require updating #3006

Leaving the copyright info in welcome.htm and readme.htm as they also acknowledge Linguasoft

Bring the copyright and format of the keyboard upto the current
Keyman Developer 17 layout.
@rc-swag
Copy link
Contributor

rc-swag commented Jul 31, 2024

@LornaSIL and @mcdurdin I believe I have addressed those remaining items.

@LornaSIL
Copy link
Contributor

Just one more thing:

The .kps file needs to be updated to deprecate the legacy\c\cs-pinyin keyboard.

That is in Details / Related Packages

Click on Add and type in cs-pinyin for the Related package ID. Select Deprecated. Apparently it finds the path by itself.

image

@rc-swag
Copy link
Contributor

rc-swag commented Jul 31, 2024

Ok added the related package

Copy link
Contributor

@LornaSIL LornaSIL left a comment

Choose a reason for hiding this comment

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

LGTM!!!!

@DavidLRowe DavidLRowe merged commit baa25b6 into keymanapp:master Aug 1, 2024
2 checks passed
@mcdurdin mcdurdin deleted the cs-pinyin branch September 17, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants