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 Moldavian diacritics to ru_keyboard #5652

Merged
merged 4 commits into from Nov 30, 2019
Merged

Conversation

@SilverGreen93
Copy link
Contributor

SilverGreen93 commented Nov 28, 2019

This commit contains the following changes:
- Added Șș diacritics - I added those instead of $ symbol which was already duplicate on the main en layout popup.
- Added Ĭĭ, Ŭŭ, Ăă (on the en layout popup)
- Added comments for all Romanian characters with diacritics - also rearranges some of them for consistency.

  • Added Ӂӂ on the Russian layout popup for Moldavian.

This change is Reviewable

@Frenzie Frenzie added this to the 2019.12 milestone Nov 30, 2019
Copy link
Member

Frenzie left a comment

Thanks for the PR! 👍

},
_a_ = {
"a",
north = "ä",
northeast = "á",
northwest = "à",
east = "â",
west = "ã",

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

This change reduces consistency and overall language compatibility (e.g., with Portuguese). It might be better to add a Romanian keyboard with minor adaptations (cf. the French keyboard). But in any case, since the æ is quite rare it's a much better candidate for being moved to the non-swipe positions.

This comment has been minimized.

Copy link
@SilverGreen93

SilverGreen93 Nov 30, 2019

Author Contributor

Ok, I will switch æ with ă and leave ã in its place, if this is ok.
I am also working on a separate Romanian keyboard.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

Well, as the keyboard layout shows, characters useful to Romanian were not a priority since that's considered as probably something better left to a Romanian keyboard, hence why they ended up in the extra row that isn't reachable by swipe without popup. The idea isn't that the US International style QWERTY layout should be ideal for everything without modifications, although it should at least be usable for a lot of languages written using the Latin alphabet. It's easier to hold for a popup and go for the extra row than to switch keyboards. When I wrote the above I forgot for a second that the æ is a common character in the Scandinavian languages. It's in English & French that it's rare. This simply betrays my own particular familiarity with Dutch, English, French, and German.

Romanian is unfortunately not part of the traditional US International type layout, see below. In that sense moving the Polish/Lithuanian ą would be a better idea if anything is to be moved within the a. That particular character is there mainly by analogy with a more traditional character like the ç.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

But also note that as per the French & Spanish mods, a "Romanian keyboard" could literally just be the swap you're proposing here.

This comment has been minimized.

Copy link
@SilverGreen93

SilverGreen93 Nov 30, 2019

Author Contributor

As I will come with a PR for a separate Romanian keyboard soon, feel free to rearrange the Romanian ă with breve (which was missing, do not confuse with ã with tilde) whereever you consider to have the least impact here :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

You mean caron, not tilde (~). But indeed, I didn't realize the difference at a glance. :-)

In light of that misinterpretation it would probably make the most sense right next to the a with caron. That's the space for characters that aren't strictly speaking part of the keyboard layout but more of a bonus feature. However, like I said the ą is also a kind of bonus feature. Since you say a Romanian keyboard is coming, I'd be more inclined to add it rather than changing any current positions.

Btw @robert00s how does the English QWERTY keyboard work for Polish atm?

@SilverGreen93

This comment has been minimized.

Copy link
Contributor Author

SilverGreen93 commented Nov 30, 2019

Updated changes.

east = "Ŝ",
west = "Š",
south = "Ş",
southeast = "$",

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

Duplicate in which way?

This comment has been minimized.

Copy link
@SilverGreen93

SilverGreen93 Nov 30, 2019

Author Contributor

The $ sign is already in the en_keyboard.lua at line 83 (second row, column 3). So I see no reason to add it here again.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

Those old layers could stand a bit more attention. ;-)

It's a fairly standard practice, fwiw.

Copy link
Member

Frenzie left a comment

.

@SilverGreen93

This comment has been minimized.

Copy link
Contributor Author

SilverGreen93 commented Nov 30, 2019

@Frenzie I think it would be better to make the changes just to the ru popup in this PR and discard the changes to the en layout. I will open another PR to add a separate Romanian layout. What do you think?

I will open 2 seaparte PR for: ru_popup and ro_keyboard

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 30, 2019

@SilverGreen93 You can also leave the ŭ if you like, but sure, I can merge the Ӂӂ immediately. The a is a bit crowded I'm afraid.

@Frenzie Frenzie added the UX label Nov 30, 2019
@SilverGreen93

This comment has been minimized.

Copy link
Contributor Author

SilverGreen93 commented Nov 30, 2019

That would be perfect, not to crowd the en_keyboard anymore. Go ahead and merge only ru_popup & ru_keyboard.

@SilverGreen93 SilverGreen93 changed the title Add Romanian and Moldavian diacritics to en & ru keyboards Add Moldavian diacritics to ru_keyboard Nov 30, 2019
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 30, 2019

Alright, could you please remove them from the PR?

@Frenzie Frenzie merged commit 849bd95 into koreader:master Nov 30, 2019
0 of 2 checks passed
0 of 2 checks passed
ci/circleci: build CircleCI is running your tests
Details
code-review/reviewable 2 files, 3 discussions left (Frenzie, SilverGreen93)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.