fyne-io / systray

a cross platfrom Go library to place an icon and menu in the notification area
Apache License 2.0
227 stars 42 forks source link

`SetIcon()` does nothing on Linux after the first call #3

Closed DeedleFake closed 2 years ago

DeedleFake commented 2 years ago

It sets the icon the first time, but then appears to do absolutely nothing after that.

andydotxyz commented 2 years ago

Can you please provide more information about the linux window manager / desktop you are using as there are many implementations each differing subtly.

DeedleFake commented 2 years ago

Sorry. I'm on Manjaro using GNOME 41 with the AppIndicator and KStatusNotifierItem Support extension. My DBus version is 1.14.0.

Also worth noting that the original github.com/getlantern/systray does correctly change the icon.

precisionpete commented 2 years ago

I can confirm that it does the same nothing on Linux Mint 20.3 Cinnamon with go 1.18. I get the icon and nothing else. It also did the same nothing on Ubuntu 20.04 and nothing at all on Fedora 35.

Seems fine on Windows 10 and macOS.

And yes. The getlantern version works fine on the same system. Except when they release new bugs into that library. I'm looking for something more reliable.

dhaavi commented 2 years ago

I can confirm the behavior on Ubuntu/Pop 21.10 using GNOME Shell 40.5.

The following two icons are created by the same program (https://github.com/safing/portmaster-ui).

image

Thanks a lot for creating an alternative that uses DBUS directly! This is really great!

andydotxyz commented 2 years ago
  • Icon itself is rendered nicely. (512x512 px)

Hmm, I guess that ayatana is doing a different scaling algorithm... Feel free to open a separate issue and we can look into it.

Thanks a lot for creating an alternative that uses DBUS directly! This is really great!

Not a problem, happy to help.

andydotxyz commented 2 years ago

I cannot replicate the issue, can you please try a different window manager, such as XFCE, to check if it's the code?

andydotxyz commented 2 years ago

And yes. The getlantern version works fine on the same system. Except when they release new bugs into that library. I'm looking for something more reliable.

Yes, in the legacy system they are literally drawing that space. Using DBUS we rely on notifications which may not all be implemented the same way.

andydotxyz commented 2 years ago

I can confirm that it does the same nothing on Linux Mint 20.3 Cinnamon with go 1.18.

Are you using additional plugins? When I run cinnamon I get no DBUS registered system tray host

precisionpete commented 2 years ago

Nothing other than vanilla installs. In the debug testing, I created clean VMs (VMware 16) using Mint 20.3 (Cinnamon) and Ubuntu 20.04. Nothing special was installed other than the recommended OS updates. The behaviour matched my Development PC's Mint 20.3. I've been really busy so have not had time to go back to test it.

andydotxyz commented 2 years ago

I created clean VMs (VMware 16) using Mint 20.3 (Cinnamon)

I have just done the same and SetIcon works. Can you please share your code as I can't track down how to replicate the problem.

precisionpete commented 2 years ago

Using the example code... And go 1.18.1 on Mint 20.3 Cinnamon. This not a VM but an Intel PC. Also using VScode.

The icon shows up. Hovering over the icon says "Awesome App". Right-click the icon and a tall skinny blank rectangle sometimes pops up (.5" w x 2" h). No text.

go.mod:

module fsystray

go 1.18

require fyne.io/systray v1.9.0

require (
    github.com/godbus/dbus/v5 v5.0.4 // indirect
    golang.org/x/sys v0.0.0-20200515095857-1151b9dac4a9 // indirect
)

main.go:

package main

import (
    "fmt"
    "io/ioutil"
    "time"

    "fyne.io/systray"
    "fyne.io/systray/example/icon"
)

func main() {
    onExit := func() {
        now := time.Now()
        ioutil.WriteFile(fmt.Sprintf(`on_exit_%d.txt`, now.UnixNano()), []byte(now.String()), 0644)
    }

    systray.Run(onReady, onExit)
}

func onReady() {
    systray.SetTemplateIcon(icon.Data, icon.Data)
    systray.SetTitle("Awesome App")
    systray.SetTooltip("Lantern")
    mQuit := systray.AddMenuItem("Quit", "Quit the whole app")
    go func() {
        <-mQuit.ClickedCh
        fmt.Println("Requesting quit")
        systray.Quit()
        fmt.Println("Finished quitting")
    }()

    // We can manipulate the systray in other goroutines
    go func() {
        systray.SetTemplateIcon(icon.Data, icon.Data)
        systray.SetTitle("Awesome App")
        systray.SetTooltip("Pretty awesome棒棒嗒")
        mChange := systray.AddMenuItem("Change Me", "Change Me")
        mChecked := systray.AddMenuItemCheckbox("Checked", "Check Me", true)
        mEnabled := systray.AddMenuItem("Enabled", "Enabled")
        // Sets the icon of a menu item. Only available on Mac.
        mEnabled.SetTemplateIcon(icon.Data, icon.Data)

        systray.AddMenuItem("Ignored", "Ignored")

        subMenuTop := systray.AddMenuItem("SubMenuTop", "SubMenu Test (top)")
        subMenuMiddle := subMenuTop.AddSubMenuItem("SubMenuMiddle", "SubMenu Test (middle)")
        subMenuBottom := subMenuMiddle.AddSubMenuItemCheckbox("SubMenuBottom - Toggle Panic!", "SubMenu Test (bottom) - Hide/Show Panic!", false)
        subMenuBottom2 := subMenuMiddle.AddSubMenuItem("SubMenuBottom - Panic!", "SubMenu Test (bottom)")

        systray.AddSeparator()
        mToggle := systray.AddMenuItem("Toggle", "Toggle some menu items")
        shown := true
        toggle := func() {
            if shown {
                subMenuBottom.Check()
                subMenuBottom2.Hide()
                mEnabled.Hide()
                shown = false
            } else {
                subMenuBottom.Uncheck()
                subMenuBottom2.Show()
                mEnabled.Show()
                shown = true
            }
        }

        for {
            select {
            case <-mChange.ClickedCh:
                mChange.SetTitle("I've Changed")
            case <-mChecked.ClickedCh:
                if mChecked.Checked() {
                    mChecked.Uncheck()
                    mChecked.SetTitle("Unchecked")
                } else {
                    mChecked.Check()
                    mChecked.SetTitle("Checked")
                }
            case <-mEnabled.ClickedCh:
                mEnabled.SetTitle("Disabled")
                mEnabled.Disable()
            case <-subMenuBottom2.ClickedCh:
                panic("panic button pressed")
            case <-subMenuBottom.ClickedCh:
                toggle()
            case <-mToggle.ClickedCh:
                toggle()
            case <-mQuit.ClickedCh:
                systray.Quit()
                fmt.Println("Quit2 now...")
                return
            }
        }
    }()
}
andydotxyz commented 2 years ago

Sorry @precisionpete - I can't see where SetIcon is being called here, so not sure how/when it should be expected to update...

precisionpete commented 2 years ago

Perhaps I am referencing a different issue then? My problem is that the menu does not show up on Linux.

i.e. The example code does not work on linux.

andydotxyz commented 2 years ago

Perhaps I am referencing a different issue then? My problem is that the menu does not show up on Linux.

Ah, that is quite a different issue - the menu and the icon are separate protocols on the DBus system

Possibly it relates somehow to https://github.com/hluk/CopyQ/issues/431, but I am not sure how... The protocol implementations are flakey so it's hard to know where to start debugging.

andydotxyz commented 2 years ago

@dhaavi can you please point us to your usage of SetIcon or a small code snippet that demonstrates the bug?

andydotxyz commented 2 years ago

@DeedleFake can you please try again using the latest version from master and see if the problem persists?

DeedleFake commented 2 years ago

It still does not work with 9177bf851614.

My project actually no longer has a tray icon because I switched it over to use Gtk4 and libappindicator is incompatible with it due to using Gtk3 internally, but I made a new branch off of the last version that did have a tray icon so that I could test it. If this issue is fixed, I might be able to add an icon back in, which would be really nice.

andydotxyz commented 2 years ago

@DeedleFake is there a line or a code segment that we can look at which is setting the icon and failing?

DeedleFake commented 2 years ago

@andydotxyz

https://github.com/DeedleFake/trayscale/blob/75caeb7497b10dcc198f4a7a4fae6cadc80ee071/cmd/trayscale/trayscale.go#L165

I commented out the more complex usage and simplified it down to basically just setting the icon and doing nothing else. It still doesn't work.

dhaavi commented 2 years ago

I cannot replicate the issue, can you please try a different window manager, such as XFCE, to check if it's the code?

Tried it with the latest Xubuntu - same issue: image

This is really interesting that it seems to work for you - but not for me on different systems. I will have a closer look tomorrow and check if there is anything wrong with our code.

@dhaavi can you please point us to your usage of SetIcon or a small code snippet that demonstrates the bug?

Here is a PR where I am testing the switch to this lib: https://github.com/safing/portmaster-ui/pull/227 We use SetIcon() here: https://github.com/safing/portmaster-ui/blob/0d2e47e31d70a8beeda5a0d44493ac5bd451e371/notifier/tray.go#L183

The systray usage is 100% the same as on master currently, which uses the lib from getlantern. Everything works there as expected.

If you want to actually try building that, it would be the easiest to first install Portmaster: https://safing.io/portmaster/ Then, use build script inside the notifier directory of the portmaster-ui repo.

Thanks for your help!

Hmm, I guess that ayatana is doing a different scaling algorithm... Feel free to open a separate issue and we can look into it.

Will do. (A bit later.)

dhaavi commented 2 years ago

I've experimented with quite some ideas now but I wasn't able to get it to work. (Using go1.18)

I've created a simple test program you can use to easily reproduce the issue in the PR https://github.com/safing/portmaster-ui/pull/227 at commit https://github.com/safing/portmaster-ui/pull/227/commits/ff58a32e772a82b21ce4ee909db8cb071560a90b.

That should cut out the noise.

dhaavi commented 2 years ago

Hey @andydotxyz, can you tell me which distro/version you use for testing? Then I could check if my example works there.

andydotxyz commented 2 years ago

I've tested on Debian, Mint and Ubuntu I think (Debian is my main development for Linux). I just looked at the code (sorry I missed that before) and it looks like you're blocking the process with a status monitoring for loop, it's possible that fixing that will resolve the issue.

dhaavi commented 2 years ago

Ok - setting up a Debian VM now...

I've moved the code in onReady to its own goroutine, but that did not change anything, as onReady is already run in a goroutine: https://github.com/fyne-io/systray/blob/9177bf851614ca0224100825ad3b66584f4d1cd5/systray.go#L96-L99

dhaavi commented 2 years ago

Ok. I have the same behavior on the Debian VM:

Setup:

If anyone wants to reproduce and has an environment for running untrusted code, here are the compiled binaries of the test code https://github.com/safing/portmaster-ui/blob/4a7b42a236dd3afa7b6cc9ebad1affeb83aa0e4f/notifier/test/main.go:

dhaavi commented 2 years ago

Interestingly, setting the icon again before starting the systray works.

So, this code:

func main() {
    systray.SetIcon(icons.GreenPNG)
    systray.SetIcon(icons.BluePNG)
    systray.Run(onReady, nil)
}

ends up with a blue icon.

Edited to add: This is obvious, because the data is cached until actually used by the lib. So, updating the internal cache works.

dhaavi commented 2 years ago

GOT IT!

Ok, so. Don't ask me about the specifics - I don't do much dbus stuff and this is one of these "It works but I don't know why" situations. But here is how I got it to work:

  1. Get properties of notification item:

    notifProperties, err = prop.Export(conn, path, instance.createPropSpec())
  2. Allow writing that property:

            "IconPixmap": {
                []PX{convertToPixels(t.iconData)},
                true, // <- allow write!
                prop.EmitTrue,
                nil,
            },
  3. In SetIcon I updated the dbus properties via their method:

    err := notifProperties.Set("org.kde.StatusNotifierItem", "IconPixmap", dbus.MakeVariant([]PX{convertToPixels(iconBytes)}))
andydotxyz commented 2 years ago

You're an absolute star @dhaavi thanks so much for nailing this - your solution makes so much sense. I will try to implement a minimal solution as a PR and perhaps you can test it for me?

andydotxyz commented 2 years ago

Let me know if #6 fixes the issue - it is not the complete set of changes you listed but should work. If not then I will move to the additional code you provided to make more use of the internal callback system of the dbus library.

dhaavi commented 2 years ago

You're an absolute star @dhaavi thanks so much for nailing this

Thanks! - You're welcome!

Let me know if https://github.com/fyne-io/systray/pull/6 fixes the issue

Unfortunately, no. This did not work.

dhaavi commented 2 years ago

Hey @andydotxyz, if you're interested, I'd supply a PR for the necessary changes.

andydotxyz commented 2 years ago

Thanks. I am hoping to look at it tonight. If I don't manage then yes please :)

andydotxyz commented 2 years ago

I have updated #6, with related fixes and tidies based on your suggestion - please let me know if it is working. If not, then I will close the PR and hand it to you as you can test the fix and pre-approve the result ;).

dhaavi commented 2 years ago

Great, thanks!

I've submitted some fixes and further improvements in https://github.com/fyne-io/systray/pull/8 - hope you don't mind the clean up I did.

andydotxyz commented 2 years ago

Nah, it's all good thanks for picking it up. Sorry that I missed the setting of the var! Not sure about the code for the icon scaling as noted in PR. Maybe that should have been a different PR?

dhaavi commented 2 years ago

No problem. ;)

Yup - I've moved the scaling commit to a separate PR. Sorry for the feature creep.