Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

'Minimize to tray' functionality using SNI (StatusNotifierItem) #1209

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

'Minimize to tray' functionality using SNI (StatusNotifierItem) #1209

wants to merge 7 commits into from

Conversation

emildekeyser
Copy link

@emildekeyser emildekeyser commented Mar 9, 2022 •
edited
Loading

Issue: #98

progress:

  • SNI dbus api
  • Quit from main burger menu
  • Systray toggleable in settings
  • Tested on:
    • KDE
    • Cinnamon
    • i3 (no DE, no tray)
    • on xfce4 by user 143981
  • Start Minimized toggleable in settings
  • Gtk4 ready
  • New message icon
  • keep window position/sizes/stacking when hidden/showed
  • Fix msg marked read in background bug

@emildekeyser emildekeyser mentioned this pull request Mar 9, 2022
Open
@mar-v-in
Copy link
Member

mar-v-in commented Mar 14, 2022 •
edited
Loading

  • Here is a demo app of using SNI natively in Vala, so you don't need to use a library. You can just copy the code from it into Dino's codebase. https://gist.github.com/mar-v-in/a7642e15dee51f4def03da5aeb4eb5b0
  • Do not link activate against a menu. Activate should activate the application (show the window) and not the menu. If some tray implementation does neither support context_menu event nor dbusmenu, that's not our fault and we shouldn't adjust for that.
  • Do not use Gtk.Menu. It's no longer supported in GTK4 and next release of Dino is planned to use GTK4 instead of GTK3.
  • Do not change the meaning of the quit action. Quit is really meant to quit the app, not to close/hide the window. If you want, you can, independently, add a new action to close the window, which would probably have the keyboard shortcut Ctrl-W.

@emildekeyser
Copy link
Author

Awsome I will use the code you linked, where did you get that from ? Also I will fix the issues you addressed.

@emildekeyser
Copy link
Author

emildekeyser commented Mar 16, 2022 •
edited
Loading

BTW, I am meaning to redo the commits once you guys are happy with the code because I find them very ugly (no/bad messages, redundancy) if you would like that ofcourse. (and do rebase if needed)

mar-v-in
mar-v-in reviewed Mar 17, 2022
View reviewed changes
main/src/ui/application.vala Outdated Show resolved Hide resolved
libdino/src/dbus/status_notifier_item.vala Show resolved Hide resolved
main/src/ui/application.vala Outdated Show resolved Hide resolved
libdino/src/dbus/status_notifier_item.vala Outdated Show resolved Hide resolved
mar-v-in
mar-v-in reviewed Mar 17, 2022
View reviewed changes
Copy link
Member

@mar-v-in mar-v-in left a comment

Choose a reason for hiding this comment

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

Seems good beside a few minors. I'll do a bit of testing this weekend and merge if everything works as expected.

libdino/src/dbus/status_notifier_item.vala Outdated Show resolved Hide resolved
main/data/menu_app.ui Show resolved Hide resolved
libdino/src/dbus/status_notifier_item.vala Show resolved Hide resolved
main/src/ui/application.vala Outdated Show resolved Hide resolved
@emildekeyser
Copy link
Author

woopsie ignore the poundsign 2 that last commit, I didnt know that it has special meaning on gh

@emildekeyser
Copy link
Author

Strange, I dont get that int to str error that makes the build fail on my machines. But when I was talking with the guy from xapps he also had the same issue. Do you guys know what is causing that ?

@emildekeyser emildekeyser requested a review from mar-v-in March 18, 2022 17:39
@2561024
Copy link
Contributor

2561024 commented Apr 3, 2022

Left click to systray works same a right - showed menu too, its problem only at me? xfce 4.16.0

@2561024
Copy link
Contributor

2561024 commented Apr 3, 2022

@emildekeyser please "Fetch upstream" on you fork...

@emildekeyser
Copy link
Author

emildekeyser commented Apr 3, 2022 via email

@emildekeyser
Copy link
Author

emildekeyser commented Apr 3, 2022 via email

@2561024
Copy link
Contributor

2561024 commented Apr 3, 2022

It is (most likely) an XFCE problem. More specifically xfce4-panel is probably not calling the 'Activate' dbus method on left click. I did some looking around and strangly enough I did find this: https://github.com/xfce-mirror/xfce4-panel/blob/master/plugins/systray/sn-item.c#L1082. Maybe search the issues/post a bug at the official xfce4-panel repo about this ?
…
On 22/04/03 10:51, wkg wrote: Left click to systray works same a right - showed menu too, its problem only at me? xfce 4.16.0 -- Reply to this email directly or view it on GitHub: #1209 (comment) You are receiving this because you authored the thread. Message ID: @.***>

I looked in the xfce-panel sources, they also noticed the problem:
https://github.com/xfce-mirror/xfce4-panel/blob/master/plugins/systray/sn-button.c#L299

according to their code, this happens if "is_menu = true":
https://github.com/xfce-mirror/xfce4-panel/blob/master/plugins/systray/sn-item.c#L798-L803

i tried changed it in you fork to "false" and this is fixed it...
do we really need "true" for this? or for others(kde, gnome) need "true"?

@emildekeyser
Copy link
Author

i looked at the spec, actually setting is_menu to true is an error on my part, changing and testing it now

@emildekeyser
Copy link
Author

emildekeyser commented Apr 4, 2022 •
edited
Loading

It seems that on my old laptop running cinnamon (with no login manager) there is a race condition which causes the SNI icon to not show up on autostart. It can be fixed by adding some seconds delay. I could make the systray code concurently retry on failure or monitor dbus till it sees the watcher for X amount of seconds.

I have inspected libappindicator's code a bit and they seem to be doing something similar (not 100% sure).

@1223421
Copy link

1223421 commented Aug 21, 2022

@emildekeyser please "Fetch upstream" again for gtk4

emildekeyser and others added 7 commits November 6, 2022 12:27
@emildekeyser
Quit Dino from app menu
993d1d4 
I thought this might be a good idea in general. But also because I
encountered problems with the SNI menu om Cinnamon I think it is nice to
have an additional way of hard quitting the app. I think having only
Ctrl-Q when the SNI dbus menu doesnt work is not very friendly to
hotkey/tech illiterates (for which I am specifically adding this feature
anyway).
Systray toggle in settings
6752101
@emildekeyser
StatusNotifierItem (SNI) dbus api
2bad1cc
@emildekeyser
SNI: Implement EventGroup
2218717 
Some DE software (observed in Cinnamon) uses EventGroup instead of just Event.
@emildekeyser
'Start minimized'
839419d 
+ togglable in settings
@emildekeyser
SNI: Fix race condition between SNHost and SNItem
6eceaab 
Without this, ie when we just call watcher.register_item, there is a race
condition between the registration of SNHost and SNItem. This can occur
when your system is configured to launch Dino at login or on startup of
the graphical environment. This was observed on Cinnamon on an old
laptop. But I assume it is a problem that could very well occur in more
places. I am hoping that this is the only race condition in this
scenario, I have done some testing manually and it seems to be fixed.
Gtk4 updates for systray
85b482b
@emildekeyser
Copy link
Author

emildekeyser commented Nov 6, 2022 •
edited
Loading

@emildekeyser please "Fetch upstream" again for gtk4

done

@1223421
Copy link

1223421 commented Nov 6, 2022 •
edited
Loading

ERROR:promise.c:667:gee_promise_future_set_value: assertion failed: (_state == State.INIT)
Bail out! ERROR:promise.c:667:gee_promise_future_set_value: assertion failed: (_state == State.INIT)

this only on my system?

@emildekeyser
Copy link
Author

ERROR:promise.c:667:gee_promise_future_set_value: assertion failed: (_state == State.INIT)
Bail out! ERROR:promise.c:667:gee_promise_future_set_value: assertion failed: (_state == State.INIT)

this only on my system?

well I dont get that on my system

@1223421
Copy link

1223421 commented Nov 6, 2022

rm -r build and rebuild fix my problem....
Test it on xfce4, good work.
@emildekeyser Thank you!

The only thing missing is changing the icon when a new message arrives...

@emildekeyser
Copy link
Author

rm -r build and rebuild fix my problem.... Test it on xfce4, good work. @emildekeyser Thank you!

The only thing missing is changing the icon when a new message arrives...

alright il look into adding that, you mean whenever there are any unread msgs there should be a little envelope or something in the corner of the dino icon ?

@1223421
Copy link

1223421 commented Nov 6, 2022 •
edited
Loading

rm -r build and rebuild fix my problem.... Test it on xfce4, good work. @emildekeyser Thank you!
The only thing missing is changing the icon when a new message arrives...

alright il look into adding that, you mean whenever there are any unread msgs there should be a little envelope or something in the corner of the dino icon ?

yes, or just change icon if any new message(if window not in focus) and change back after focus window

@1223421
Copy link

1223421 commented Nov 6, 2022

icon.patch.txt
I would like this, but this is a bad decision, of course

@1223421
Copy link

1223421 commented Nov 7, 2022 •
edited
Loading

i continue testing, and another wish: keep the position/sizes of the window when it is hidden/showed if it is possible

Copy link

I was testing your pull request and it is working fine I only found a small issue.
If i close the app (leaving it open in background) I no longer receive notification for new messages.
Also the messages on the chat that was open are marked as read even if the app is in tray.
It seems to me that dino thinks to be open even while in tray.

Copy link

It's unfortunate to see this feature is not merged in Dino. However, I would like to inform anyone it may concern that this code has been really helpful to me in order to continue indicator support in Haguichi, which I very recently ported to GTK4 and libadwaita. Without the existence of this example I would likely have dropped indicator support. But now the indicator is even working on more distros than before, because this implementation doesn't depend on libappindicator!

I made few minor modifications to suit specific needs for Haguichi. But I also made some more generic tweaks to update the menu item properties and to set the host_registered state. Also silenced all Vala compiler warnings by adding minimal error handling. The file is located at src/status-notifier-item.vala. I didn't change any original code formatting so the file is easily diffable with the one from this pull request or the gist from @mar-v-in.

The modified file contains attribution to the "Dino contributors". Let me know if you are an author and would like to be personally attributed using your real name.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mar-v-in mar-v-in Awaiting requested review from mar-v-in

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@emildekeyser @mar-v-in @2561024 @1223421