Kxkb (mostly aesthetic) improvements #202

Changes:

Three modes (label and flag {default}, flag only, label only);
Fully customizable layout label (font, colors, shadow);
Definitions and flags for several existing locales (Braille, Esperanto, Javanese, Maori);
Some flags were slightly adjusted (removed/filled empty space, optimization etc.)
Minor improvements (pixmap cache usage, cleanups).

Ideas:

Probably replace old low-quality pixmaps with vector flags? Better quality, but might take up more disk space.

Rebased to current HEAD.

Great, thanks Philippe. Will have a look at this this week, maybe tomorrow.

Hi Phillipe,
I finally got around to test this. Here is some feedback for you.

great idea, I like the new config options avaiable 👍
on my system the label shows up very big at the first run. While it is easy to fix it changing the config, I think it would be advisable to keep the default settings the same as the existing ones (label colors, size, shadow)
when changing the options, the selected keyboard is reset to the default one. For example I have US-IT keyboards configured. If I click on the icon and selected IT and then change some settings, the keyboard is automatically switched back to US. No big deal, but definitely a small annoying bug
using QtStyle theme, the "Sticky Switching" groupbox becomes a bit "lost" if we make the config page very wide. This happens because the text/color fields in "Use custom colors" group box expand to the right, but the spinbox of "Number of layouts to rotate" expands differently. It seems to me the internal layout of the two group boxes is a bit different, with the "Use custom colors" having a left column and expandable right column, while "Sticky Switching" have two columns that expands together. I think it would look better if the "Number of layouts to rotate" spinbox expands in the same way as the text/color fields above that do.
the layout page looks good too, without the label on the flag that re found. There are a couple of languages (Arabic, Spanish (Latin America) ) who do not have a flag (at least in Debian) and show up with an empty flag with the label. The new label is too big while the original label was fitting nicely. Here it would make sense to have the same default settings as also explained in point 2.
the flags in the layout page seems a bit stretched vertically. Overall it looks better than before, just wondering if a bit less stretch would look better or not.

Comparison between original flags and new flags.
Btw, notice Chinese and Croatian flags probably need some fix up (not visible in the screenshot though)

compare.png

76 KiB

For debian like systems, to be built with TDE/tde-packaging#80.

Hello Michele, thank you for the feedback.

Apropos 2,5: Indeed the font size was 12 instead of 10.
Apropos 3: I suppose it's due to Kxkb applying the options. Will look into the problem.

Update: It's due to Kxkb restarting itself. What would the best way to solve this? Maybe pass a command line switch to set initial layout?

Apropos 4: Will look into this, too. If only TQt Designer didn't segfault on saving it would've been easier.
Apropos 6: They appear so because the actual pictures (in tdebase/l10n/...) come in quite different sizes. Some of them even had transparent space on bottom and right side. The solution here would probably be adapting those for one and the same size, of 1:1 proportions (a box).
Also, I've just noticed that in flags that are in the tray icon's context menu, the white colour becomes transparent for some reason.

So, seems that I have quite a bit of polishing to do to Kxkb ;)

Update: It's due to Kxkb restarting itself. What would the best way to solve this? Maybe pass a command line switch to set initial layout?

Ok, in this case I guess this was an existing problem, so perhaps we can address this separately after merging this PR. Adding a CLI switch to specify the initial layout could be a good way to go, if not there already.

Also, I've just noticed that in flags that are in the tray icon's context menu, the white colour becomes transparent for some reason.

Yeah, noticed similar problems. The italian flag is partly transparent, with the green left side not visible in the tray bar.

So, seems that I have quite a bit of polishing to do to Kxkb ;)

good work so far 👍

Let me know when you want me to have another round of testing at this.

Also, I've just noticed that in flags that are in the tray icon's context menu, the white colour becomes transparent for some reason.

I'm beginning to suspect that this might be a TQt bug, as changeItem( int id, TQPixmap& pixmap) inserts the flag pixmap correctly (but removes the text label), probably in insertAny(...) function.

Also, I've just noticed that in flags that are in the tray icon's context menu, the white colour becomes transparent for some reason.

I'm beginning to suspect that this might be a TQt bug, as changeItem( int id, TQPixmap& pixmap) inserts the flag pixmap correctly (but removes the text label), probably in insertAny(...) function.

uhm... I haven't look at the code so I could be wrong, but if it was a TQt bug I would expect this to show up in a lot of places, not just on kxkb systray, if you get what I mean. I would first look into kxkb as the source of the issue. Happy to be proven wrong, if that is the case 😄

if it was a TQt bug I would expect this to show up in a lot of places, not just on kxkb systray,

I know, but then, most apps make use of TDEActions and standard icons in popup menus, not TQPixmap's and Kxkb code looks correct; that's why I made this assumption. I was trying to solve this bug yesterday, but maybe I have missed something someplace.

Update: it must be a problem in the LayoutIcon class after all.

@MicheleC I'd like to hear your opinion on 3d81351942. It's somewhat of a hack, adding a little border to flags, which though eliminates the problem with the "lost colours". It works for me. Does it work for you? Also, does it look good or ugly?

The new version doesn't show any problem with the italian flag, I can see all the colors. I don't see any black border anyway.
One thing I noted is that when the label is shown, the colors of the flag are darker, not sure if this was something already done by the original code or something you added in.

Also, with reference to point 2. here, the default colors of the label is still different from the original one. Default color in the new version is black, which makes the label difficult to read on the US flag for example. I think the default color in the original code was white.

One thing I noted is that when the label is shown, the colors of the flag are darker, not sure if this was something already done by the original code or something you added in.

It is part of the original code. I suppose it's done so that the label can be seen easier.

Default color in the new version is black, which makes the label difficult to read on the US flag for example. I think the default color in the original code was white.

The default colors are still white for the text and gray for the background, with a black shadow. Still, though, the default is to use the theme highlight colors, maybe this is to blame. So, either in the case of both flag and label being shown the label should always be white or the "Use custom colors" should be on by default to let use the original colors of Kxkb.

It is part of the original code. I suppose it's done so that the label can be seen easier.

Ok, in that case we can address this separately if we decide to change this.

The default colors are still white for the text and gray for the background, with a black shadow. Still, though, the default is to use the theme highlight colors, maybe this is to blame. So, either in the case of both flag and label being shown the label should always be white or the "Use custom colors" should be on by default to let use the original colors of Kxkb.

Uhm... I will do a new test with and without the PR. Currently with the PR I see a black label. Will let you know after I test.

Currently with the PR I see a black label.

What I mean is, maybe in your colorsceme "Selected text" is black, and by default "Use custom colors" is off, so colors from the colorscheme are used (resulting in the unreadable label). So, maybe forcing the label to be white when the flag is also shown could solve this problem.

Point 3 solved with commit 9c3cca1664.

Point 3 solved with commit 9c3cca1664.

Excellent solution, works like a charm.
Just need to tide up the points I mentioned above.

We can probably consider merging this commit separately since it fixes a different issue. What do you think?

I think you misunderstood the code a little bit; the border is drawn regardless of whether the indicator is shown. The check is here so that the indicator does not make use of the border, because there there is no need for it. The problem's only with the context menu.

Yes, I probably did misunderstood, I had just a quick look and I confused the indicator with the label.

Regarding colors, I did a new comparison test. See attached screenshot.
Top part is with the code from tdebase master. Label color is very small and the color is white.
Bottom part is the code from PR. The label is black and wider. As you can see they are different. The wider label is good, IMO.

No other settings were changed in between the two tests. My "Selected text" color scheme is black, as you pointed out. If I change it to a different color and restart kxkb, I can see the new color there, so it confirms your assumption.

Now the question is to whether go for "Selected text" or "white" as the default color (I am assuming here that many users will have a default text of black...).
What do you think?
@Slavek: question open to you too.

screenshot.png

124 KiB

So, what do you think? If we enable the border for the indicator, it looks a little IMO out of place:

As you can see from my screenshot, I don't see any border (maybe we have different icon size??).
But looking at your screenshot, I agree it looks a bit out of place and I could see users complaining about it.
So I guess we need to look for a different solution for that transparency issue.

Let's try to go step by step. Whatever was an original problem (transparenct, darker flag with label, ...) we can address separately.
On this PR let's focus on the new functionality you are adding in 👍

Now the question is to whether go for "Selected text" or "white" as the default color (I am assuming here that many users will have a default text of black...).
What do you think?
@Slavek: question open to you too.

It seems that makes sense to use white as default because flags are not dependent on setting the users color theme and use the color Selected text can lead to the text will not be readable.

Is there a question here if it could help use another color for the shadow when the text is black?

So, to sum up:

The border hack (3d81351942 Added hack for non-indicator pixmaps so that they are displayed correctly) should be undone and will be addressed in separate PR;
We should use the Kxkb defaults, not the colorscheme colors by default;
76e6f163cd (Re-set previous layout after changing layout settings) will be a separate commit.

Is there a question here if it could help use another color for the shadow when the text is black?

There could be an additional option to set the shadow color. What do you think?

darker flag with label

So this something we do not want? I mean, there was a reason for this, and the reason was so that the label is easier to read.

I did find a better way to solve the transparency issue in 0234685780 — by not resizing them to box size.

So, to sum up:

The border hack (3d81351942 Added hack for non-indicator pixmaps so that they are displayed correctly) should be undone and will be addressed in separate PR;

We should use the Kxkb defaults, not the colorscheme colors by default;

76e6f163cd (Re-set previous layout after changing layout settings) will be a separate commit.

Is there a question here if it could help use another color for the shadow when the text is black?

There could be an additional option to set the shadow color. What do you think?

darker flag with label

So this something we do not want? I mean, there was a reason for this, and the reason was so that the label is easier to read.

That is a good sum up. will test the new code tomorrow and let you know how it goes.
Regarding shadow, yes it would be good to be able to choose the shadow color as well.

So this something we do not want? I mean, there was a reason for this, and the reason was so that the label is easier to read

Sorry, missed this. Let's see how the new PR is first, then we can discuss/address separately since it was something done in the original code already

Hi Philippe,
I tested the latest version. It seems we are almost there 👍
Here is some feedback.

nice to have the ability to choose the color of the shadow. Small thing to fix here. When we switch "Enable shadow" to off, it correctly disables the color chooser component. But when we open the config dialog from the popup menu in kxkb tray icon, the color chooser is enabled even if "Enable shadow" is off.
"Use theme color" as implemented now is very counter intuitive for the user. But I now see where you were coming from in first place!! Your original solution makes a lot more sense now!
It would probably be good to have a radio button group with the choices:

use theme colors
use custom colors (follow by the color selection fields).
By default, "custom colors" should be enable if no config exists in kxkbrc and the original (pre PR) default colors be the chosen one.
I guess this is what you were trying to do from the beginning, it just needed the extra radio button I think.

the "what's this" tip of the "Label style" part shows an empty line in the middle of the text between "even when" and "labels are"
the transparency problem doesn't show up anymore... not sure why but I can't reproduce it at the moment
please check the intendation of your code, you can see in TGW that it does not line up with the rest of the code (I guess you indent by 2 spaces).
don't forget to fix up this

the transparency problem doesn't show up anymore... not sure why but I can't reproduce it at the moment

I think I solved this with 0234685780.

I think the issues are solved now.

Great, thanks Philippe. Will run a test tomorrow and update as usual. Looking forward to complete this and have it in the master branch 😄

Functionality wise it is good now, all issues solved AFAICT.
Need to fix a type and adjust indentation in a few spots that were missed.
After that it is good to go 👍

Done with the changes, will squash the commits now.

Done, do the commits look okay? Do I proceed to rebasing and merging into master etc.?

Looks good!

Done, do the commits look okay? Do I proceed to rebasing and merging into master etc.?

Good job Philippe.
Yes, go ahead with rebasing and merging.
After that we need to backport to R14.0.x.

Merged and backported

Merged and backported

Great, congratulation in merging your first PR!!

Small note: for backporting, we use git cherry-pick command. This adds a "(cherry picked from commit <hash>)" to the commit message, making clear where the commit was coming from.
For future backports, please take note of this. If you need some example, just have a look at other commits backported to R14.0.x.

Sorry my bad for lack of information. We use

git cherry-pick -x -S <hash to cherry pick>

Without the -x, the line I was mentioning above is not added.

Didn't know about needing the -x switch, thank you for the info (even though it's a little late), will take note for future backports.

         p.drawPixmap(TQRect(0, 0, FLAG_MAX_DIM, FLAG_MAX_DIM), flag);
       } else {
         // HACK: this solves strange colour loss in flags in context menu
         pm->fill( TQt::black ); // so that this looks like a nice border

                                     <string>Label Style</string>
                                 </property>
                                 <property name="whatsThis" stdset="0">
                                     <string>Here you can choose the way the label of your keyboard layout indicator will be displayed. These options are relevant even when flabels are disabled, for locales where the flag is missing.</string>

 	bool m_useKxkb;
 	bool m_showSingle;
 	bool m_showFlag;
   bool m_showFlag;

       kdDebug() << "Warning: invalid reply from Kxkb!" << endl;
   TQString currentLayout;
   reply.get(currentLayout);

   DCOPReply successReply = kxkbref.call( "setLayout", currentLayout );
   if ( !reply.isValid() )
       kdDebug() << "Warning: invalid reply from Kxkb!" << endl;

 	m_showSingle = config->readBoolEntry("ShowSingle", false);
 	m_showFlag = config->readBoolEntry("ShowFlag", true);
   m_showFlag = config->readBoolEntry("ShowFlag", true);

 	config->writeEntry("ShowSingle", m_showSingle);
 	config->writeEntry("ShowFlag", m_showFlag);
   config->writeEntry("ShowFlag", m_showFlag);

				`int m_stickySwitchingDepth;`


				`bool m_useThemeColors;`

Kxkb (mostly aesthetic) improvements #202

Reviewers