Re: Wine keyboard driver+XKB. What am I to do?

2007-05-20 Thread Shachar Shemesh
Oleh R. Nykyforchyn wrote:
 Hello,
 I need an advice on what to do with some piece of code that I have written for
 about 3 years. I started to make changes in Wine keyboard driver because I was
 not able to use MS Office under it on my Linux box (3 or 4 XKB groups, 2 
 overlay
 groups used, English, Russian, Ukrainian, Belarusian, German and Polish
 layouts). First I submitted a patch that adds koi8-u encoding to Wine, and it 
 was
 happily introduced. But my changes to X11DRV (now winex11) keyboard driver 
 were
 large and I understand Wine people who didn't want to risk.  Meanwhile I
 continued to polish my implementation to correct bugs and improve performance.
 I am heavily indebted to people that tested my patches, wrote me about 
 problems
 with them and suggested possible solutions. I thank to L.Rahyen that supported
 me in my efforts.

 Now patched keyboard driver allows user to:

 1) have up to 4 XKB groups working (current code uses 1 or 2);
 2) detect and use XKB overlays to put 2 or 3 close layouts (e.g. Russian,
 Ukrainian and Belarusian) into one XKB group;
 3) freely combine available XKB layouts in any order, e.g. en,ru,fr;
 4) list all layouts available at the system, and implement
 ActivateKeyboardLayout();
 5) notify an application (e.g. MS Word) about change of layout, and make
 GetKeyboardLayout() work really, not just return LCID for current locale
 of Unix box;
 6) input characters that do not fit into current Unix locale, e.g. German and
 French accented letters at a system with Cyrillic (not UTF) locale;
 7) override any of detected layouts via registry, if user is not satisfied
 with Wine driver choice.

 In fact I made cosmetic changes to 3 files in winex11.drv directory: x11drv.h,
 x11drv_main.c, event.c, but the fourth file keyboard.c was changed much more.
 About half of functions in it were rewritten, and it now easier to read new
 keyboard.c than diff output to understand the changes. 

 I got very reasonable advice from L.Rahyen to broke the patch into smaller
 parts. The problem is that I preserved the driver logic, but changed
 data structures, so any such patch (even very small one) will touch hundreds 
 if
 lines across the whole file, probably introducing new bugs and being difficult
 to read and understand. Also, there is no reason to change code by a patch
 if we can benefit of it only after next patch.

 Now I will be grateful to any help. How can such big changes be introduced in
 Wine tree? I also attach a patch against wine-0.9.37 and copies of original 
 and
 changed files. Perhaps somebody, who is interested in multilingual keyboard 
 input,
 can test it and write me about results.

 Oleh
   
Also, have a look at http://bugs.winehq.org/show_bug.cgi?id=735, which
is an independent attempt to solve the same problem. It's a pity I
didn't know about your effort.

Shachar




Re: Wine keyboard driver+XKB. What am I to do?

2007-05-17 Thread Vitaliy Margolen
Oleh R. Nykyforchyn wrote:
 Hello,
 I need an advice on what to do with some piece of code that I have written for
 about 3 years. I started to make changes in Wine keyboard driver because I was
If you would have cleaned it up and sent 3 years ago, then it would have
been a part of the Wine for that long...

What you should do is:
1. Remove all the useless changes (white space, debugging code, extra
hacks all over the place).
2. Pick an indentation style either use the code's style or 4 spaces for
indentation (not 3, not 5, and no tabs please).
3. Separate independent changes from everything else (additional
layouts, layout table, structures, header files, different debug
channel(s), noop changes) and send them separately to wine-patches.
4. Clean up the logic changing code and brake it into separate
independent functions or logical parts that can be applied one after
another (detect layouts, init keyboard, helper functions, etc).

 In fact I made cosmetic changes to 3 files in winex11.drv directory: x11drv.h,
 x11drv_main.c, event.c, but the fourth file keyboard.c was changed much more.
 About half of functions in it were rewritten, and it now easier to read new
 keyboard.c than diff output to understand the changes. 
Don't do cosmetic changes. This makes it impossible to see what the real
changes are. Please remove all this type of changes from your patch.
This will account for about 80% of all the changes.

 I got very reasonable advice from L.Rahyen to broke the patch into smaller
 parts. The problem is that I preserved the driver logic, but changed
 data structures, so any such patch (even very small one) will touch hundreds 
 if
I didn't see all these hundreds of lines of code. I've seen 10-20 lines
for each data structure change (not counting main_key_tab). Btw some of
your structure changes are totally unnecessary and should be reverted
(changes to event.c file).

 lines across the whole file, probably introducing new bugs and being difficult
 to read and understand. Also, there is no reason to change code by a patch
Actually no. That would be the easiest thing to read - structure changes
for each line. Nothing else.


Thanks for working on this for such a long time and for all your hard
work! I'm sure there are lots of people who really like to see this sort
of functionality in Wine.

Vitaliy.