fcitx / fcitx

A Flexible Input Method Framework
http://fcitx-im.org
GNU General Public License v2.0
853 stars 174 forks source link

StatusNotifier uses the wrong icon name #465

Open mtwebster opened 4 years ago

mtwebster commented 4 years ago

Hi, we recently began supporting the StatusNotifierItem spec in Cinnamon/Mint, and it seems as though it uses the incorrect icon when no im is selected.

See: https://user-images.githubusercontent.com/262776/92781618-48b71200-f372-11ea-8498-c9dd3f6222c9.png

This is an icon from the active icon theme. I think it needs to use the fcitx-specific name (which the xembed tray icon appears to use)

edit: I updated this to really the only pertinent thing, the no-im-active icon being a generic theme name, rather than an fcitx icon name.

wengxt commented 4 years ago

I think the behavior matches the intention of the code.

Try to use Pinyin to see if it shows a "拼" icon.

The keyboard layout always uses that icon, unfortunately. The actual thing I'd like to show is some text (like "us" "ru"), but I don't think StatusNotifierItem supports something like show a text string.

There's a extension property XAyatanaLabel, I'm not sure if you guys support that.

wengxt commented 4 years ago

So here's the thing, XAyatanaLabel was only supported on ubuntu initially and it's never supported on KDE, so I never add such support to fcitx 5 (mainly because ubuntu uses gnome-shell now, but on gnome-shell we recommend to use https://extensions.gnome.org/extension/261/kimpanel/). But I think fcitx 4 supports it, but only emit it under unity for now, unfortunately https://github.com/fcitx/fcitx/blob/3f87c5acd129d4c85410e1064e0eb6c662133b81/src/module/notificationitem/notificationitem.c#L530

wengxt commented 4 years ago

another reason is that, this property is undocumented, and I never get what's the point of XAyatanaLabelGuide because no one use it nowadays.

ubuntu's appindicator extension seems to show only the label string without touching XAyatanaLabelGuide. https://github.com/ubuntu/gnome-shell-extension-appindicator/blob/c71f76e793d3371fbd5bdecbaa9736b4a8e623cc/indicatorStatusIcon.js#L71

wengxt commented 4 years ago

图片

This is what fcitx 5 it looks like with https://extensions.gnome.org/extension/615/appindicator-support/

janos-r commented 4 years ago

I thought the issue is the requested name... When not using statusNotifier, it asks for /usr/share/icons/hicolor/22x22/apps/fcitx-kbd.png - this is the usual icon. But with statusNotifier it asks for "input-keyboard" and it doesn't find much. What if it also asked for fcitx-kbd when under statusNotifier? Would we have the expected behavior (the previous fcitx-kbd.png icon)?

wengxt commented 4 years ago

ah, I see. This is actually an intended behavior (hardcoded). Because input-keyboard is provided by most icon theme so this specific icon can looks consistent with the system icon theme.

Fcitx's own xembed tray icon doesn't use xdg icon theme, it only uses /usr/share/fcitx/imicon/.... . Fcitx support xdg icon theme, but still, input-keyboard would be used.

mtwebster commented 4 years ago

Yeah I think the main complaint was that moving from xembed to notifier is the (unexpected) change to the icons.

We support labels, but don't actually see them used anywhere, which is fine with me. The label guide was used to tell the panel "this is the maximum amount of space a label might take" so it could be pre-allocated, and adjacent items on the panel wouldn't be bumped around as the label changed. But I definitely would want to avoid any properties outside of the official StatusNotifier spec.

A couple of things to maybe keep in mind going forward:

Anyhow, we're going to change our themes' input-keyboards icon to something less awful which should help with the original issue a bit.

Thanks a lot

clefebvre commented 3 years ago

Hi @wengxt,

The code is OK in my opinion. There are things which could be improved (using symbolics, hidpi support etc..) but these are different topics.

The best way to load the icon is by icon name (i.e. using the icon theme), and that's perfectly OK as well. But the problem is the name of the icon you're using... it's a very generic one. It's also used by an application in MATE (the Keyboard preferences) and by an application in Cinnamon (the Virtual Keyboard app).

The icon theme can make input-keyboard look either like an app icon, or a status icon, but not both.. these are two different styles of icons but there's only one name here.

If you use fcitx-kbd instead of input-keyboard, then it will look in the theme for it and won't find it, and it will fall back to the icon you provided:

image

We can also make it look different by adding it to our icon theme, and this time it won't conflict with any other apps that use input-keyboard, since only fcitx uses fcitx-kbd.

clefebvre commented 3 years ago

In terms of code I'm only talking about changing the default/kbd icon name:

image

wengxt commented 3 years ago

So, first of all, fcitx 4 reaches its end of life and only in maintenance mode for certain bug fixing. So after all it is not gonna be changed. fcitx 4 xembed icon doesn't support xdg icon theme so I have to add a "kbd" for it anyway. That's basically why there's a fcitx-kbd there, but never be used for SNI.

Secondly, even on fcitx 5, we still gonna use input-keyboard ( or "-symbolic". based on xdg icon fallback rule it will fallback to input-keyboard if symbolic version is not available, but on certain desktop input-keyboard works better so that's where we prefer input-keyboard as name). I think it looks ok on major desktop (KDE/GNOME). If we gonna use a keyboard icon anyway, I would prefer the icon comes from desktop icon theme because it IS a generic icon considering the fact that fcitx manages keyboard layout. I think using input-keyboard is the right choice instead of adding an inconsistent icon of ourselves.

clefebvre commented 3 years ago

OK, it's a pity. We could have made fcitx status icons to make Mozc and the keyboard status consistent with each other. We can't theme this properly unless it uses its own icon name.

clefebvre commented 3 years ago

@wengxt in case it helps, we talk about this at https://github.com/linuxmint/mint20.1-beta/issues/44.

On our side we'll stop using input-keyboard in Cinnamon and remove it from our icon theme. This fixes the issue for the original bug report (it makes input-keyboard used by fcitx look like Adwaita icon basically so it no longer looks like an application).

We'll also consider making a Cinnamon applet for it. Neither input-keyboard (from GNOME) nor fcitx-kbd (the provided one) look great. Switching to a symbolic and having the layout beside it ("fr", "de", "us"..etc..) would be much better.

wengxt commented 3 years ago

Yeah, if you check above fcitx5's screenshot, it's using input-keyboard-symbolic now for non-KDE (in kde input-keyboard looks better in panel).

Different DE uses icon in a different way. Another thing FYI, gnome-shell used to replace all icon with -symbolic suffix when being displayed by gnome-shell, but now they just don't do that so you see the ugly colorful "input-keyboard". I'd suggest you try to patch it to input-keybord-symbolic to see if works better for mint.

BTW here's what it looks like in KDE,

image

Also, as a side note for "virtual keyboard" in your issue, KDE's breeze has "input-keyboard-virtual", which basically fallback to input-keyboard when "input-keyboard-virtual" is not available.

While some people may think that fcitx should have its own branding, but I think Fcitx is a system component and should be as generic as possible. Especially fcitx when managing keyboard layout, which is traditionally put under system's keyboard section.

wengxt commented 3 years ago

https://github.com/fcitx/fcitx5/blob/03c9771bdf65bff6f07505b9d2861f97d6a388b5/src/modules/notificationitem/notificationitem.cpp#L66

Relevant code in fcitx5