Skip to content

Commit

Permalink
xapp status applet: Store icons using a unique key based on the icon
Browse files Browse the repository at this point in the history
name and object path.

The name is not necessarily enough now due to changes here:
linuxmint/xapp@f6db3f7
  • Loading branch information
mtwebster committed Aug 28, 2020
1 parent 42960e3 commit fa11b3f
Showing 1 changed file with 30 additions and 24 deletions.
54 changes: 30 additions & 24 deletions files/usr/share/cinnamon/applets/xapp-status@cinnamon.org/applet.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,7 @@ class XAppStatusIcon {
this.iconSize = this.applet.getPanelIconSize(type);
this.proxy.icon_size = this.iconSize;

// for now, assume symbolic icons would always be square/suitable for an StIcon.
// TODO: Need to handle symbolic filenames also.

// Assume symbolic icons would always be square/suitable for an StIcon.
if (iconName.includes("/") && type != St.IconType.SYMBOLIC) {
St.TextureCache.get_default().load_image_from_file_async(iconName,
/* If top/bottom panel, allow the image to expand horizontally,
Expand Down Expand Up @@ -319,10 +317,17 @@ class CinnamonXAppStatusApplet extends Applet.Applet {
this.signalManager.connect(this.panel, "icon-size-changed", this.icon_size_changed, this);
}

onMonitorIconAdded(monitor, icon_proxy) {
getKey(icon_proxy) {
let proxy_name = icon_proxy.get_name();
let proxy_path = icon_proxy.get_object_path()

return proxy_name + proxy_path;
}

onMonitorIconAdded(monitor, icon_proxy) {
let key = this.getKey(icon_proxy);

if (this.statusIcons[proxy_name]) {
if (this.statusIcons[key]) {
return;
}

Expand All @@ -333,22 +338,22 @@ class CinnamonXAppStatusApplet extends Applet.Applet {
return;
}

global.log(`Adding XAppStatusIcon: ${icon_proxy.name} (${proxy_name})`);
global.log(`Adding XAppStatusIcon: ${icon_proxy.name} (${key})`);
this.addStatusIcon(icon_proxy);
}

onMonitorIconRemoved(monitor, icon_proxy) {
let proxy_name = icon_proxy.get_name();
let key = this.getKey(icon_proxy);

if (!this.statusIcons[proxy_name]) {
if (this.ignoredProxies[proxy_name]) {
delete this.ignoredProxies[proxy_name];
if (!this.statusIcons[key]) {
if (this.ignoredProxies[key]) {
delete this.ignoredProxies[key];
}

return;
}

global.log(`Removing XAppStatusIcon: ${icon_proxy.name} (${proxy_name})`);
global.log(`Removing XAppStatusIcon: ${icon_proxy.name} (${key})`);
this.removeStatusIcon(icon_proxy);
}

Expand Down Expand Up @@ -378,44 +383,44 @@ class CinnamonXAppStatusApplet extends Applet.Applet {
}

addStatusIcon(icon_proxy) {
let proxy_name = icon_proxy.get_name();

let key = this.getKey(icon_proxy);
let statusIcon = new XAppStatusIcon(this, icon_proxy);

this.manager_container.insert_child_at_index(statusIcon.actor, 0);
this.statusIcons[proxy_name] = statusIcon;
this.statusIcons[key] = statusIcon;

this.sortIcons();
}

removeStatusIcon(icon_proxy) {
let proxy_name = icon_proxy.get_name();
let key = this.getKey(icon_proxy);

if (!this.statusIcons[proxy_name]) {
if (!this.statusIcons[key]) {
return;
}

this.manager_container.remove_child(this.statusIcons[proxy_name].actor);
this.statusIcons[proxy_name].destroy();
delete this.statusIcons[proxy_name];
this.manager_container.remove_child(this.statusIcons[key].actor);
this.statusIcons[key].destroy();
delete this.statusIcons[key];

this.sortIcons();
}

ignoreStatusIcon(icon_proxy) {
let proxy_name = icon_proxy.get_name();
let key = this.getKey(icon_proxy);

if (this.ignoredProxies[proxy_name]) {
if (this.ignoredProxies[key]) {
return;
}

this.ignoredProxies[proxy_name] = icon_proxy;
this.ignoredProxies[key] = icon_proxy;
}

shouldIgnoreStatusIcon(icon_proxy) {
let hiddenIcons = Main.systrayManager.getRoles();

if (hiddenIcons.indexOf(icon_proxy.name) != -1 ) {
let key = this.getKey(icon_proxy);
if (hiddenIcons.indexOf(key) != -1 ) {
return true;
}

Expand All @@ -434,7 +439,8 @@ class CinnamonXAppStatusApplet extends Applet.Applet {
return -1;
}

return GLib.utf8_collate(a.proxy.name, b.proxy.name);
return GLib.utf8_collate(a.proxy.name.replace("org.x.StatusIcon.", "").toLowerCase(),
b.proxy.name.replace("org.x.StatusIcon.", "").toLowerCase());
}

sortIcons() {
Expand Down

11 comments on commit fa11b3f

@janos-r
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there,
after the update yesterday, the recently missing fcitx (input method) icon got a new one.
image
But it is not really the right icon. Do I assume correctly that the appearance of an icon is due to these changes? If yes, than thank you ^^ good step forward, but please would it be possible to have a look at this specific fcitx icon... It should resemble a keyboard, but it looks just like a white square with a "@" in the corner.
Thank you.

@mtwebster
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the icon it's requesting - "input-keyboard":

This is the information fcitx is providing to the applet:
image

In Mint-Y, unfortunately, uses that ugly white box (maybe we should improve that in the theme) but if I switch to other icon themes, it looks better. I tried this with the normal system tray applet (by removing the xapp applet temporarily) and it gave the same icon as well.

@janos-r
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quick reply and the check.
You are right, this is the icon from /usr/share/icons/Mint-Y/devices/22/input-keyboard.png
I think the previous icon was "input-keyboard-symbolic" which I found in /usr/share/icons/Adwaita/scalable/devices/input-keyboard-symbolic.svg
I tried to copy it to /usr/share/icons/Mint-Y/devices/symbolic both as input-keyboard-symbolic.svg or input-keyboard.svg but both were not recognized.
It always reads just the icon from /usr/share/icons/Mint-Y/devices/22/input-keyboard.png
I even tried to rename the .png with the .svg present in ../symbolic, but it remained unrecognized.
It is a little surprising, since the icon was fine on mint19.3. And even prior to this update, every time when shutting down the system, the expected icon appeared in the blank space for a split second.
My totally uneducated guess is that previously, it was matching also names with *-symbolic and was able to display .svg?
Is the "xapp status applet" able to recognize .svg? If yes, how can I make it recognize "input-keyboard-symbolic.svg" ?
Thank you for your time in advance.

@janos-r
Copy link

@janos-r janos-r commented on fa11b3f Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my company pc, Im on mint19.3. Here the icon looks still like this:
image
Sorry, upon closer look, I see it's not the one from Adwaita. This one has a small cable on top. But input-keyboard.png (the white box) is still the same here in mint-y on 19.3, so I don't have a clue where this "original" icon comes from on 19.3 and why it's not found on 20.
Please have a look into it... fcitx is probably the most common alternative input method on linux. And the mint team did a great job in 19.1 to make input methods more accessible! Would be sad to have such a great input method wizard and suddenly a broken icon.
Thank you.

@mtwebster
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I did a bit of digging in the fcitx source.

A bit of background on status icons:

There are two types of 'status icons' that are relevant here - a traditional x-embed style (which uses the x server to provide the status icon) - these types end up in our old systray applet. There's a newer type called StatusNotifier which uses dbus to communicate between the application (or service, like fcitx) and the desktop and tray applets.

The first kind is on its way out - it isn't supported under wayland, it's no longer supported by Gtk, and is generally a pain because it can be tricky to render the icon in modern systems and desktops. The second kind can work anywhere. QT supports both still, though I'm not sure how long that may be, I don't follow their development.

Now, for the specific problem here:
In mint 19.x, we only supported the x-embed type icon. In fcitx, this works correctly, the icon pasted above is used, and the icon updates when/if the input language is changed.

In mint 20, we now use the newer style, and end up a different icon, and it never gets updated (I can look at exactly what icon name fcitx is specifying.)

So, it seems like problem here is that fcitx needs to try and make its StatusNotifier icon match the x-embed icon, and right now, it's never updating. I'm going to file a bug report with fcitx about this, but in the meantime, you can disable StatusNotifier support entirely by running this in a terminal:

gsettings set org.x.apps.statusicon status-notifier-enabled-desktops []

I'll update with a bug link once I file it.

@janos-r
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the dig!
I'm just not sure what you mean with "never updating". If you mean loading the right icon, ok...
Let me just mention (I didn't before) while using fcitx with mozc (japanese), when switching to mozc, to write in Japanese (you have to stand in a text field), the icon correctly switches to the correct orange mozc icon. This "update" of the icon is fine. Just the default icon remains that white box.
Ok, I assume that fcitx has to update their install, to add their specific icon to the icon path... so that the StatusNotifier can find the correct one, if I understand correctly. I always thought that fcitx is the most modern and most updated of the input methods, or in comparison to ibus. I'm quite disappointed. Thank you for your effort and I hope this gets fixed soon. Basically whole asia is using fcitx or ibus on linux. That would be embarrassing if not resolved.

@mtwebster
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It updated for you? Strange.. it would never switch here. I'm not sure I have everything configured correctly, but I could get the icon to change when using the traditional (Xembed) icon, but not the StatusNotifier.

The way I tested was, I added Japanese (utf-8) to system languages and added "Keyboard - Japanese" to the fcitx "Input Methods" tab. When I'd switch to the new language in the status icon menu, the icon would never update. But using the same setup, but disabling StatusNotifier like I described above, the icon would change to (I think) a blue JP or something along those lines.

The "input-keyboard" icon is only used when StatusNotifier is active. The xembed version uses different ones.

Here's the report for reference:
fcitx/fcitx#465

I wouldn't place much blame on fcitx - most desktop environments don't enable StatusNotifier support by default, so it's possible no one ever noticed this issue before, or at least not to anyone who decided to report it - it's not due to any recent changes as far as I can tell.

@janos-r
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a whole guide on the setup with details back when 19.0 came out:
https://youtu.be/UIKm67gDGEk
You don't add anything to "keyboards", you just add "mozc" to the "input method configuration".
The mint team even added this short setup description to the cinnamon input method wizard:
image
Notice the mentioning on Mozc there.
So... when setup correctly, it switches either with the shortcut, or with left-click on the icon. There you get the correct mozc icon.
The problem remains with the fcitx icon that goes back to being a white brick.
If you would simply just take out fcitx from being part of the xapp panel and having it on the side like in 19.3, that could also work. Wouldn't it work and look like before? Without requiring fcitx to change their icon setup?

@mtwebster
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something they need to fix on their side. And regardless of how different my configuration was from yours, the fact remains it was updating with one type of tray and not the other. So I feel there is at least one other issue there.

I can't 'block' fcitx without blocking other things, due to how StatusNotifier works, though I've already mentioned how to disable all of this for now to restore the keyboard icon.

Really, the incorrect icon is more of a theme issue than anything else. Our Mint-Y theme icon for this is not a very good one.

@janos-r
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just worried you were switching the keyboard layout and not the input method. But ok, I trust you, you are the expert. My only issue here was the fcitx icon. With fcitx now added in the xapp addon, it is loading the mint-y icon, that was not really ever there. I'm not sure if this is more a mint or fcitx responsibility, but I will be silently waiting for a fix in the future.
Thank you once more for all your time and effort. Bye

@mtwebster
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I installed mozc and can confirm the icon does change, so I won't worry about that particular issue I guess. I'm definitely no expert on this, and would rather have too much info than not enough, so thanks for checking my configuration.

The reason it's picking a Mint-Y icon is that it's actually asking for a different icon name than before. It asks for input-keyboard now. It should (I think) be requesting fcitx-kbd (those are in /usr/share/icons/hicolor - this is where program-supplied icons should go. The theme checks here if it can't find the icon name in the current theme).

Please sign in to comment.