TDE
/
kvkbd
6
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity
Labels Milestones

Fix displayed characters #16

Merged
MicheleC merged 4 commits from fix/key-display into master 3 years ago
Conversation 5 Commits 4 Files Changed 3
Ray-V commented 3 years ago
Collaborator

Blank the key for the VoidSymbol, .notdef, character which is usually an upright outline rectangle.
For example, Turkish keyboard, AltGr+'D'.

Add combining character for key display.

Toggle the case of the displayed AltGr alpha characters when Caps is on.

For example on the Italian keyboard, the sequence AltGr+ Caps+AltGr+ AltGr+Shift+ Caps+AltGr+Shift+ on the 'D' key prints as ðÐÐð.

Pre-patch this shows on the kvkbd key as ððÐÐ, and post-patch shows as printed.

Blank the key for the VoidSymbol, .notdef, character which is usually an upright outline rectangle. For example, Turkish keyboard, AltGr+'D'. --- Add combining character for key display. --- Toggle the case of the displayed AltGr alpha characters when Caps is on. For example on the Italian keyboard, the sequence AltGr+ Caps+AltGr+ AltGr+Shift+ Caps+AltGr+Shift+ on the 'D' key prints as ðÐÐð. Pre-patch this shows on the kvkbd key as ððÐÐ, and post-patch shows as printed.
Ray-V added 3 commits 3 years ago
450074d274 Blank VoidSymbol for key display
3b35caf849 Add character representation for key display
MicheleC requested changes 3 years ago
MicheleC left a comment
Owner

  1. VoidSymbol: need to discuss whether it is right or wrong.

  2. caps+altGr text: PR is good but need some rework to align to the rest of the code (will result in more performing code)

1. VoidSymbol: need to discuss whether it is right or wrong. 2. caps+altGr text: PR is good but need some rework to align to the rest of the code (will result in more performing code)
src/VButton.cpp Outdated
{
if (shift)
{
TQPushButton::setText(altGrShiftText.lower());
MicheleC commented 3 years ago
Owner

@Ray-V
Could you rework the code so that it is similar to the other cases? What I mean is to introduce two variables (for example capsAltGrText and capsAltGrShiftText) and set them in setupTexts() and here we just call setText() with the corresponding variable?
The difference is setupText() is called much less frequently that shiftCapsAltGrPressed(), so we avoid continuous lower()/upper() calls and related memory usage.
I suggest we add a function setAltGrText() similar to the way we use setShiftText().

@Ray-V Could you rework the code so that it is similar to the other cases? What I mean is to introduce two variables (for example capsAltGrText and capsAltGrShiftText) and set them in setupTexts() and here we just call setText() with the corresponding variable? The difference is setupText() is called much less frequently that shiftCapsAltGrPressed(), so we avoid continuous lower()/upper() calls and related memory usage. I suggest we add a function setAltGrText() similar to the way we use setShiftText().
src/Xutils.cpp Outdated
{ 0x0000fe65L, 0x1ffe }, /* dead_abovereversedcomma ῾ Greek Dasia */
{ 0x0000fe68L, 0x02cd }, /* dead_belowmacron ˍ modifier letter low macron, eg actual=ṯ */
{ 0x0000fe6eL, 0x201a }, /* dead_belowcomma ‚ quotesinglbase, eg actual=ț */
{ 0x00ffffffL, 0x0020 }, /* VoidSymbol -> <- space */
MicheleC commented 3 years ago
Owner

Not sure what is the best here. VoidSymbol is supposed to be void, right? I understand the point that the empty rectangle character may look ugly, but inserting a blank (0x20) is different from inserting a void symbol.
I can't think of a real useful usage of a void symbol to be honest, but it seems wrong to me to replace it with a blank. @SlavekB what is your opinion on this as well?

Not sure what is the best here. VoidSymbol is supposed to be void, right? I understand the point that the empty rectangle character may look ugly, but inserting a blank (0x20) is different from inserting a void symbol. I can't think of a real _useful_ usage of a void symbol to be honest, but it seems wrong to me to replace it with a blank. @SlavekB what is your opinion on this as well?
MicheleC commented 3 years ago
Owner

This is ok, as per this commnet

This is ok, as per [this commnet](https://mirror.git.trinitydesktop.org/gitea/TDE/kvkbd/issues/16#issuecomment-12270)
MicheleC marked this conversation as resolved
Ray-V commented 3 years ago
Poster
Collaborator

Could you rework the code so that it is similar to the other cases?

I'll see what I can do. I did look at something similar, but didn't consider resource usage and decided it was overkill for what was needed.

VoidSymbol

I think that one of the benefits of kvkbd is that it shows on the keys what characters will be printed - an advantage over a physical keyboard for the level 3 & 4 characters where the user has to know what character will be printed by any particular key sequence.

The .notdef character is ugly and it also detracts from the display of the characters which any set of modifier keys enable.

This in no way affects the functioning of either type of keyboard, so it's not a replacement, it's just a representation on the kvkbd key. It seems reasonable to me that if a key prints nothing, it should show nothing.

>Could you rework the code so that it is similar to the other cases? I'll see what I can do. I did look at something similar, but didn't consider resource usage and decided it was overkill for what was needed. --- >VoidSymbol I think that one of the benefits of kvkbd is that it shows on the keys what characters will be printed - an advantage over a physical keyboard for the level 3 & 4 characters where the user has to know what character will be printed by any particular key sequence. The .notdef character ***is*** ugly and it also detracts from the display of the characters which any set of modifier keys enable. This in no way affects the functioning of either type of keyboard, so it's not a replacement, it's just a representation on the kvkbd key. It seems reasonable to me that if a key prints nothing, it should show nothing.
MicheleC commented 3 years ago
Owner

This in no way affects the functioning of either type of keyboard, so it's not a replacement, it's just a representation on the kvkbd key.

Oh yes 🤦 I totally forgot about that and thought about the character being printed and not shown on the key. In that case, I agree with you!

> This in no way affects the functioning of either type of keyboard, so it's not a replacement, it's just a representation on the kvkbd key. Oh yes 🤦 I totally forgot about that and thought about the character being printed and not shown on the key. In that case, I agree with you!
MicheleC force-pushed fix/key-display from 5f2cc19f6d to 907f3d61aa 3 years ago
MicheleC added 1 commit 3 years ago
f519fe9ea2
Minor code rework for better consistency.
MicheleC commented 3 years ago
Owner

Hi @Ray-V,
this had been pending for a while. I have rebased and adjusted the code as I had previously asked. Could you test everything is fine on your side before we proceed with merging?

Hi @Ray-V, this had been pending for a while. I have rebased and adjusted the code as I had previously asked. Could you test everything is fine on your side before we proceed with merging?
Ray-V commented 3 years ago
Poster
Collaborator

Works for me

Works for me
MicheleC merged commit f519fe9ea2 into master 3 years ago
MicheleC deleted branch fix/key-display 3 years ago
MicheleC approved these changes 3 years ago
MicheleC added this to the R14.0.11 release milestone 3 years ago
MicheleC commented 3 years ago
Owner

Thanks for testing and for the original PR work @Ray-V.

Thanks for testing and for the original PR work @Ray-V.

Reviewers

MicheleC approved these changes 3 years ago
The pull request has been merged as f519fe9ea2.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
MicheleC
Labels
Apply labels
Clear labels
GE/need-info
General - need additional info from contributor PR/keep-branch
Pull request - do not delete branch after merging PR/not-ok
Pull request - need fixing PR/rfc
Pull request - request for comments PR/update-trans
Pull request - update to translation files needed PR/wip
Pull request - work in progress RS/R14.0.x
Related to R14.0.x series RS/R14.1.x
Related to R14.1.x series SL/critical
Severity level - critical SL/major
Severity level - major SL/minor
Severity level - minor SL/normal
Severity level - normal SL/regression
Severity level - regression from previous version SL/trivial
Severity level - trivial SL/wishlist
Severity level - wishlist request ST/duplicate
Status - duplicate of another issue ST/invalid
Status - invalid report ST/notourproblem
Status - not our problem ST/rejected
Status - rejected ST/wontfix
Status - won't fix ST/worksforme
Status - works for me, unable to reproduce
No Label GE/need-info PR/keep-branch PR/not-ok PR/rfc PR/update-trans PR/wip RS/R14.0.x RS/R14.1.x SL/critical SL/major SL/minor SL/normal SL/regression SL/trivial SL/wishlist ST/duplicate ST/invalid ST/notourproblem ST/rejected ST/wontfix ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.0.11 release
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/kvkbd#16
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'fix/key-display'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes