legionus / kbd

Mirror of https://git.kernel.org/pub/scm/linux/kernel/git/legion/kbd.git
https://kbd-project.org
Other
84 stars 41 forks source link

openSUSE upstreaming, part 2 #112

Closed stanislav-brabec closed 3 months ago

stanislav-brabec commented 5 months ago

Here is a second set of openSUSE upstreaming effort.

With exception of 22b6de6 already discussed in legionus#111, all commits represent additions that come from the suse-add.tar.bz2. The tarball was carefully reviewed. I dropped files that are equal to the upstream, files that are worse or older than the upstream and files that have a near upstream replacement. The rest is set of additions that could still make sense.

Feel free to cherry pick.

legionus commented 4 months ago

I feel uncomfortable when unrelated changes are lumped together in one PR. There's no need to do that.

For example I agree with commit:

commit: Drop broken keymap de_alt_UTF-8.map

But you added scripts to contrib:

commit: contrib: Add sed script to convert mac keymaps to i386 commit: contrib: Add supplementary shell script to convert-kbd-mac.sh

but I don’t see you adding the conversion result with this script. A separate question is whether this should be done at all.

All other commits have very poor descriptions. They describe what is being done and not why it is being done. For example:

commit: Add Czech qwerty map

Add Czech/English querty keyboard. Changed by pause.

Changed by pause ? WAT ? And the link https://bugzilla.opensuse.org/show_bug.cgi?id=61829 and the link does not help at all because it is Bug Access Denied.

So far, this is more like an attempt to move legacy hacks from the vendor rpm package to the upstream with minimal effort. Sorry, but I will not accept changes that I do not understand why they are being made.

stanislav-brabec commented 3 months ago

I feel uncomfortable when unrelated changes are lumped together in one PR. There's no need to do that.

Me too. What is the optimal solution? More pull requests from different branches?

In the last set of patches, I deleted disapproved commits and did force push.

But you added scripts to contrib:

commit: contrib: Add sed script to convert mac keymaps to i386 commit: contrib: Add supplementary shell script to convert-kbd-mac.sh

but I don’t see you adding the conversion result with this script. A separate question is whether this should be done at all.

I am not adding the result of the script, as it changes all mac keymaps in place. The installation on a standard PC installs Mac keymaps, but as Mac hardware uses a different scancodes (at least on a legacy hardware that was used for these keymaps), these keymaps are not useful for PC. The script attempts to create a nearest match of Mac keymaps on a PC hardware.

I don't know which scancodes uses the recent Mac hardware, so I did not add the result of the conversion and kept the original mac files.

All other commits have very poor descriptions. They describe what is being done and not why it is being done. For example:

commit: Add Czech qwerty map

Add Czech/English querty keyboard. Changed by pause.

Changed by pause ? WAT ? And the link https://bugzilla.opensuse.org/show_bug.cgi?id=61829 and the link does not help at all because it is Bug Access Denied. This is a dual layout keymap. Layouts are switched by the Pause key.

I overseen that the bug is not public. Here is the original comment:

We need new map (querty) for console where czech layout will be the default and second will be english same as has cz-us-qwertz.map. Because current cz-lat2 has first english and second czech which is inconsistent to czech querty map in X11.

The english and czech layout is changed by pressing 'pause' key in console.

I prepared new map by modifying cz-lat2.

The author: @bruclik

So far, this is more like an attempt to move legacy hacks from the vendor rpm package to the upstream with minimal effort. Sorry, but I will not accept changes that I do not understand why they are being made.

I spent several weeks to decompose and analyze all the divergences of the package over the last 25 years. I dropped most of them, and created patches only from files that still look useful. I hope that we will get a consensus about usefulness of the particular commits, and keep only those that really make sense.

legionus commented 3 months ago

I feel uncomfortable when unrelated changes are lumped together in one PR. There's no need to do that.

Me too. What is the optimal solution? More pull requests from different branches?

Create separate PRs for each feature you offer. With a clear description of the proposed changes and clear commit messages. And please, no PRs with "looks useful" changes. If you don’t know exactly why it needs to be upstreamed, then it’s better to leave it in your package.

legionus commented 3 months ago

But you added scripts to contrib: commit: contrib: Add sed script to convert mac keymaps to i386 commit: contrib: Add supplementary shell script to convert-kbd-mac.sh but I don’t see you adding the conversion result with this script. A separate question is whether this should be done at all.

I am not adding the result of the script, as it changes all mac keymaps in place. The installation on a standard PC installs Mac keymaps, but as Mac hardware uses a different scancodes (at least on a legacy hardware that was used for these keymaps), these keymaps are not useful for PC. The script attempts to create a nearest match of Mac keymaps on a PC hardware.

I don't know which scancodes uses the recent Mac hardware, so I did not add the result of the conversion and kept the original mac files.

Then let's keep these scripts in your rpm package. I don’t want to have contrib scripts that are unclear what they do and are used by whom.

legionus commented 3 months ago

I overseen that the bug is not public. Here is the original comment:

We need new map (querty) for console where czech layout will be the default and second will be english same as has cz-us-qwertz.map. Because current cz-lat2 has first english and second czech which is inconsistent to czech querty map in X11.

Sorry, but the "not like X11" argument is a bad argument. Keymaps for the kernel and X11 are completely different keymaps and they have completely different restrictions. I don't think it's a good idea to add keymaps with all language combinations. The keymaps directory will turn into hell.

I know that debian converts the entire xkb database into kbd keymaps to comply with x11, but it keeps it for itself and does not try to upstream it.

If you want a similar solution, then I suggest testing the patch that adds xkb to kbd.

legionus commented 3 months ago

I'm closing this big PR in favor of new PRs.

stanislav-brabec commented 3 months ago

I overseen that the bug is not public. Here is the original comment: We need new map (querty) for console where czech layout will be the default and second will be english same as has cz-us-qwertz.map. Because current cz-lat2 has first english and second czech which is inconsistent to czech querty map in X11.

Sorry, but the "not like X11" argument is a bad argument. Keymaps for the kernel and X11 are completely different keymaps and they have completely different restrictions. I don't think it's a good idea to add keymaps with all language combinations. The keymaps directory will turn into hell.

That is not "all language combinations". Some languages have a keymap that combines the national keyboard with another (mostly English one) with a switch hotkey. The national part cannot type latin characters or numbers, the English part fulfills the task.

Czech keymaps is a problematic area. Czech language has many accented letters. There is an official Czech keymap. But it is widely disliked by programmers, as it does not have a numeric row. It reserves upper row to accented characters. (And as the standard comes from the pre-computer era, it does not support even basic programmer characters.) That is why there are so many Czech keymaps:

I know that debian converts the entire xkb database into kbd keymaps to comply with x11, but it keeps it for itself and does not try to upstream it.

If you want a similar solution, then I suggest testing the patch that adds xkb to kbd.

SUSE does the same nowadays. We use a converter tool based on Fedora. However some languages use the upper mentioned multi-keymap layout in X11 (with a switch hotkey), and those keymaps cannot be converted yet. (For example Russian, Greek.) That is why we use the dedicated kbd keymaps for those languages.

dmage commented 3 months ago

@stanislav-brabec if that tool doesn't support multi-language keymaps, you should give @legionus's patch a try - https://github.com/legionus/kbd/tree/xkbcommon-v1. It adds --xkb-model, --xkb-layout, etc to loadkeys.

Regarding new keymaps, the project has accepted too many keymaps, making it impossible to maintain. It should be more restrictive. I believe it would be fair to say that new layouts should only use Unicode. Additionally, they should either reflect a physical keyboard (ideally, new keymaps should be submitted with a keyboard photo so we can later verify their correctness) or mimic another operating system (Windows, macOS). Custom keymaps that people created for their own convenience probably shouldn't belong to the project that is installed on every Linux machine.