search

fyne-io / systray

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

[macOS] Allow setting status item interactive removal behavior #82

Closed clintharrison closed 1 month ago

clintharrison commented 2 months ago

This is largely bringing over https://github.com/getlantern/systray/pull/262 -- except there's a key fix in this version 🙂 (the setRemovalAllowed function as implemented in the original version is incorrectly using BOOL for the parameter instead of NSNumber *, which in practice means you can't ever set it back to false :P).

The new API is very basic but I didn't want to overdo it for a single-OS-specific feature anyway.

andydotxyz commented 2 months ago

I don't understand why you are doing BOOL -> NSNumber -> BOOL

clintharrison commented 2 months ago

I don't understand why you are doing BOOL -> NSNumber -> BOOL

It does not build otherwise:

systray_darwin.m:362:50: error: cast of 'bool' to 'id' is disallowed with ARC
andydotxyz commented 2 months ago

Right, so you would not cast the BOOL to an id. This is all in code you have added so it should be possible to use whichever types you want.

clintharrison commented 2 months ago

@andydotxyz do you prefer having two separate methods, like this? Otherwise, we will need to wrap it. (https://stackoverflow.com/a/6120731 explains better than I can, as I obviously do very little macOS-specific development :))

clintharrison commented 2 months ago

@andydotxyz just wanted to check in on this again! Let me know if you have any other preferred solution here.

andydotxyz commented 2 months ago

Sorry I got lost in the Apple documentation for passing parameters into different thread calls and understand the problem you were trying to work around. What I forgot to post last week is that I think it's up to you - neither is ideal, but I see now the pattern you were copying before.

clintharrison commented 2 months ago

Well, let's go with the current (two method) approach then 🙂 This is just step one in a long side quest to free up some mandatory menu bar apps at work 😆

andydotxyz commented 2 months ago

Yeah that sounds good.

I wonder, is there any point including the RemovalAllowed for platforms where it is not supported? It feels misleading. If it is to be included it should certainly have documentation to show it is non-functional.

clint-stripe commented 2 months ago

Yeah that sounds good.

I wonder, is there any point including the RemovalAllowed for platforms where it is not supported? It feels misleading. If it is to be included it should certainly have documentation to show it is non-functional.

Do we have any way to "exclude" it from other platforms? It's documented on the method that it is macOS only, and that's also mentioned in the readme. Where else would you like it documented?

(If you have a specific idea in mind, feel free to push to this branch!)

andydotxyz commented 2 months ago

Do we have any way to "exclude" it from other platforms?

Just removing the empty methods will do that won't it? The code will only compile if you use that method on macOS. Maybe that's a bit aggressive?

It's documented on the method that it is macOS only

I know that is what it looks like, but that won't actually be visible on generated API documentation - because it is on the Darwin method only. So if the person generating the docs is on macOS then it will be published, but if they are on a linux computer the docs will be empty... Do you see the challenge here?

clintharrison commented 2 months ago

Do we have any way to "exclude" it from other platforms?

Just removing the empty methods will do that won't it? The code will only compile if you use that method on macOS. Maybe that's a bit aggressive?

Yeah, I think that is too aggressive. There are a few other methods that do not work on every platform, and follow this same pattern, e.g.:

func SetTitle(t string)
    SetTitle sets the systray title, only available on Mac and Linux.

(This is kind of an awkward example because it appears since that comment was written it does now have some dbus functionality... but this pattern was true in the past: https://github.com/fyne-io/systray/blame/727ade2a510960202300d5557c93c35a3efcb79b/systray_linux.go)

It's documented on the method that it is macOS only

I know that is what it looks like, but that won't actually be visible on generated API documentation - because it is on the Darwin method only. So if the person generating the docs is on macOS then it will be published, but if they are on a linux computer the docs will be empty... Do you see the challenge here?

The comment is on the empty method in systray_unix.go too -- I think it would still show up. I just checked quickly with go doc -all on a Linux box and see:

func SetRemovalAllowed(allowed bool)
    SetRemovalAllowed sets whether a user can remove the systray icon or not.
    This is only supported on macOS.
andydotxyz commented 2 months ago

The comment is on the empty method in systray_unix.go too -- I think it would still show up.

My apologies, not sure how that didn't work for me.

clintharrison commented 1 month ago

@andydotxyz just wanted to bump this again - happy to make more changes if desired, but I think this might be mergeable now?

andydotxyz commented 1 month ago

Thanks, I forgot sorry