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

Remove popups not related to Turkish #909

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

dvrnynr
Copy link
Contributor

@dvrnynr dvrnynr commented May 18, 2021

  • Removed unnecessary letters that Turkish language doesn't have.
  • Edited domain extensions for correct local usage.
  • Still have no idea about how to assign lower case "i" to upper case "İ" and lower case "ı" to upper case "I"

Closes #882

@patrickgold
Copy link
Member

patrickgold commented May 18, 2021

Thanks for your contribution, really appretiate it!

Still have no idea about how to assign lower case "i" to upper case "İ" and lower case "ı" to upper case "I"

You can use a case selector for things the auto caps key can't automatically do correctly. For example (copied from Colemak):

{ "$": "case_selector",
  "lower": { "code":   59, "label": ";" },
  "upper": { "code":   58, "label": ":" }
}

When using the manual case selector, you have to define both the lower case and upper case popups separately in the tr.json mapping.

@Hayleia
Copy link
Contributor

Hayleia commented May 18, 2021

Still have no idea about how to assign lower case "i" to upper case "İ" and lower case "ı" to upper case "I"

I have something that solves this problem in my PR.
https://github.com/florisboard/florisboard/pull/866/files#diff-cfdfa4eed23e6dc024e7dbf860c34f8286791199ffd45fb90e2324f7223588fd

Used here for example.
https://github.com/florisboard/florisboard/pull/866/files#diff-34eb5065f71a55b180ff1a6068c032ec579a82361caa0a681879aa3d371f6a5dR9

I don't know if my PR is going to be merged as is or reworked or split or rejected, but at least it should be possible to do what you want using something similar.

edit I typed this at the same time as the other answer. I didn't know there was a way to do it already (or maybe it's relatively new?).

@patrickgold
Copy link
Member

patrickgold commented May 18, 2021

@dvrnynr Thanks for reworking the layout.

I have tested out and seen several issues though:

  1. There's a syntax error on line 4 of tr.json (the apostrophes are set incorrectly in the authors field)
  2. The codes of the keys for the is don't match up and show up empty or some other characters. If you use a case_selector, make sure to define the lowercase and uppercase key with their appropriate key codes and labels (they don't match in that case and a case selector does not do any automatic case mapping for you). Use https://unicode-table.com to find the correct keys for the letters wished to be added.
  3. Why do you remove so many popup keys? I understand that many are used very rarely and if they need reordering I fully support that, though just blanket removing them seems to restrict the usefulness of the TR layout severely if some more rarely used popup keys are needed anyways. I'd suggest evaluating which keys are at least a tiny bit relevant and placing them accordingly in the relevant field.

I don't know if my PR is going to be merged as is or reworked or split or rejected, but at least it should be possible to do what you want using something similar.

@Hayleia Your PR will definitely be merged at some point (because the composer implementation is pretty good as far as I've seen it on the quick overview). The up_low_text_key thing is basically a re-implementation of the case_selector, so this is the only thing which might need to be removed and re-implemented using the case_selectors. I'll get to that in detail though in your PR.

EDIT: @ Hanleia if you ever read this comment because I accidentally pinged you instead of @ Hayleia, please ignore this comment :)

@dvrnynr
Copy link
Contributor Author

dvrnynr commented May 19, 2021

@patrickgold Sorry for those stupid mistakes. That's my first time actually 😅

  1. Fixed.
  2. Fixed.
  3. I removed all those popup keys because they have no use in Turkish at all. I mean not very rarely. We don't have them in our alphabet. Total strangers to us 😄

@patrickgold
Copy link
Member

Thanks for fixing the codes. I just tried it out and found that there is a bug in the popup logic, which prevents the case selector from being evaluated within a popup. I will try to fix that later and then merge your changes in!

},
"main": { "$": "case_selector", "lower": { "code": 305, "label": "ı" },
"upper": { "code": 73, "label": "I" }
}},
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like here is a bracket too much.

Suggested change
}},
},

},
"main": { "$": "case_selector", "lower": { "code": 105, "label": "i" },
"upper": { "code": 304, "label": "İ" }
}},
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like here is a bracket too much.

Suggested change
}},
},

@patrickgold
Copy link
Member

@dvrnynr Sorry for the long wait on bug fixing. The bug itself which prevented me from merging has been fixed quite some time ago, although the Turkish layout and its popup also got some fixes (mainly the dotless i thing). Could you review the new Turkish popups in the latest beta v0.3.13-beta08 and base your improvements on the current master branch? This would be very helpful, thanks in advance!

@dvrnynr
Copy link
Contributor Author

dvrnynr commented Aug 13, 2021

@dvrnynr Sorry for the long wait on bug fixing. The bug itself which prevented me from merging has been fixed quite some time ago, although the Turkish layout and its popup also got some fixes (mainly the dotless i thing). Could you review the new Turkish popups in the latest beta v0.3.13-beta08 and base your improvements on the current master branch? This would be very helpful, thanks in advance!

I have just installed beta08 and dotless i thing works fine. But useless accents are still there. Sorry but what do you mean by "base your improvements on the current master branch". I don't know what to do as this is my very first pull request.

@patrickgold
Copy link
Member

But useless accents are still there. Sorry but what do you mean by "base your improvements on the current master branch". I don't know what to do as this is my very first pull request.

No worries, as you have little experience with Git yet I've rebased the branch and squashed the commits for you. Could you still have a final look at the json file (https://github.com/florisboard/florisboard/blob/762afdeca872a294b168bba893847817d1c6164d/app/src/main/assets/ime/text/characters/extended_popups/tr.json) if I didn't mess up somewhere? Thanks!

@patrickgold patrickgold changed the title Update tr.json Remove popups not related to Turkish Aug 13, 2021
@dvrnynr
Copy link
Contributor Author

dvrnynr commented Aug 13, 2021

But useless accents are still there. Sorry but what do you mean by "base your improvements on the current master branch". I don't know what to do as this is my very first pull request.

No worries, as you have little experience with Git yet I've rebased the branch and squashed the commits for you. Could you still have a final look at the json file (https://github.com/florisboard/florisboard/blob/762afdeca872a294b168bba893847817d1c6164d/app/src/main/assets/ime/text/characters/extended_popups/tr.json) if I didn't mess up somewhere? Thanks!

Thanks for your understanding. I've checked .json file and it's all good. Thank you.

@patrickgold
Copy link
Member

Perfect, thanks for checking. I will merge your changes in then, will be available in beta09 either tomorrow or in two days.

@patrickgold patrickgold merged commit 689881f into florisboard:master Aug 13, 2021
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.

Turkish QWERTY layout is wrong
4 participants