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

[korean_rr] remove non-functional Windows target #2345

Merged
merged 5 commits into from Aug 30, 2023

Conversation

DavidLRowe
Copy link
Contributor

No description provided.

@DavidLRowe
Copy link
Contributor Author

Addresses the immediate issue raised on #1703 by removing Windows as a target.

Does not address Keyman's lack of IMX support.

@mcdurdin
Copy link
Member

Does not address Keyman's lack of IMX support.

Keyman for Windows has IMX support. So I think the big issue here is that I haven't had space to look into this problem, because there's a bunch of history here around this keyboard, and I don't remember it. I understand the Windows target is not working but would like to try and fix that rather than simply disable it and then forget again :)

@DavidLRowe
Copy link
Contributor Author

Does not address Keyman's lack of IMX support.

Keyman for Windows has IMX support. So I think the big issue here is that I haven't had space to look into this problem, because there's a bunch of history here around this keyboard, and I don't remember it. I understand the Windows target is not working but would like to try and fix that rather than simply disable it and then forget again :)

Sorry for the sloppy comment. Perhaps "Does not address this keyboard's IMX issues on Windows."? (Instead this PR removes Windows as a target.)

Issue #1703 will remain open.

@DavidLRowe DavidLRowe requested review from LornaSIL and rc-swag and removed request for LornaSIL August 18, 2023 20:27
@DavidLRowe
Copy link
Contributor Author

@rc-swag
When I changed the target to web only, the .kmx file wasn't generated, which caused an error.
I removed the .kmx from the .kps list, and got an error saying it needed either .kmx or .js.
I added the .js and it built.
But the question is whether this package has everything needed to work on the web. Is it possible to build a web-only keyboard?

@LornaSIL LornaSIL added this to In progress in Keyboards Aug 28, 2023
@LornaSIL
Copy link
Contributor

@darcywong00 I don't have enough knowledge to know if this should be approved or not.

@@ -9,7 +9,6 @@
<ReadMeFile>readme.htm</ReadMeFile>
<MSIFileName></MSIFileName>
<MSIOptions></MSIOptions>
<FollowKeyboardVersion/>
Copy link
Contributor

Choose a reason for hiding this comment

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

@DavidLRowe - is it possible to keep this checked?

Copy link

Choose a reason for hiding this comment

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

@LornaSIL @DavidLRowe I think we need to see if we can keep this checked then I am ok to approve

@darcywong00
Copy link
Contributor

@darcywong00 I don't have enough knowledge to know if this should be approved or not.

I think this one is fine to approve. @DavidLRowe is keeping #1703 open and I also created a related issue keymanapp/keyman#9516 for refactoring Keyman for Windows to support korean_rr.

@LornaSIL LornaSIL self-assigned this Aug 29, 2023
@LornaSIL
Copy link
Contributor

It won't let me check it, so I think @rc-swag needs to approve it now.

Add `<FollowKeyboardVersion/>` back in.
Copy link

@rc-swag rc-swag left a comment

Choose a reason for hiding this comment

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

lgtm

@LornaSIL LornaSIL merged commit 6b62562 into keymanapp:master Aug 30, 2023
2 checks passed
Keyboards automation moved this from In progress to Done Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Keyboards
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants