search

inmbolmie / 5250_usb_converter

Converter to plug an IBM 5251 terminal to a Linux PC via USB emulating a VT52 terminal
GNU General Public License v3.0
34 stars 6 forks source link

Blackbit/fixes #1

Closed blackbit42 closed 4 years ago

blackbit42 commented 4 years ago

Some cleanups. No significant changes to business logic.

inmbolmie commented 4 years ago

Thanks for taking time to improve the code. Not very familiarized to this "pull request" dynamics but will try to merge the changes. The only thing i'm feeling is not right is ordering the dictionaries by scancode. The reason is that the are already more or less ordered by the "natural key position", that is from left to right and from top to bottom. That way it is easier for me to translate a mapping from one keyboard type to other, and find a mapping to modify it without knowing the scancode.

blackbit42 commented 4 years ago

Fair enough. I understand the desire to have some kind of 'natural' order of the scancodes. Still, i found it a bit difficult to work with the structure that I found. It might be worthwhile structuring the order in a way so the intent is more obvious. Maybe by grouping them by row and commenting accordingly? I also created a mapping for a 122 key US keyboard that I can provide in an additional pull request once this one is merged if you feel so inclined.

blackbit42 commented 4 years ago

Again on the ordering of keyboard scancodes. Do you have a suggestion how the lists for individual keyboards can be organized in way so overview is provided which scancodes are assigned and which ones aren't? That was the foremost intent to order them ascending.

inmbolmie commented 4 years ago

Again on the ordering of keyboard scancodes. Do you have a suggestion how the lists for individual keyboards can be organized in way so overview is provided which scancodes are assigned and which ones aren't? That was the foremost intent to order them ascending.

Do you want to know "which scancodes are not assigned" or "which keys are not assigned" ? For me a scancode has no particular meaning so its ordering also has no meaning, as I suppose concrete keys are assigned concrete scancodes for historical reasons or whatever. Knowing instead unassigned keys is more interesting, for that I suppose that can be organized by key blocks (function block, alpha block, numpad block etc) and then by natural order (left to right and top to bottom", leaving unassigned keys commented out.

blackbit42 commented 4 years ago

I'd say both aspects are of some interest, but I agree, unsigned keys are probably more relevant. Organizing the scancodes by key blocks makes sense to me.

inmbolmie commented 4 years ago

Hi, I've tried to merge the commits (first time ever for me with git) except for the "scancode ordering" commit. I had a bad day because the indentation commit depends somehow on the previous commits. Then edited manually the mappings as commented to organize them by function block and key position. Hope it is more useful that way.

Some empty key mappings should still be added anyway to complete the layouts, but I don't have now resources for that.

UNFORTUNATELY I don't have access to the converter this week so I only can test that the application runs, hope I didn't break anything with my changes, because there were some weird conflicts and indentation problems due to not applying (I suppose) one of the commits.

Thanks and best regards

blackbit42 commented 4 years ago

Awesome, thanks.

blackbit42 commented 4 years ago

Hmm, it seems like some changes went missing when you merged. E.g. the formatting fixes didn't happen. Do you want me to produce a new patch that doesn't depend on the scancodes reordering?

inmbolmie commented 4 years ago

Mmm wait I'll see what happened, everything looked OK when I pushed the changes...

inmbolmie commented 4 years ago

Well, the important changes seem to be there, maybe it is that I messed up only the whitespace changes? Because other changes like line breaks are still there. So feel free to send another patch over what is now in master (to keep my changes over the key mapping comments) and I'll apply it.