-
-
Notifications
You must be signed in to change notification settings - Fork 32
Implement background app monitoring #394
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.
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
Conversation
|
That ghost is really cute. Your current implementation--with the ghost moved to between launchers and workspaces--is what I would go for; I think it strikes a good balance between space and availability.
|
|
I kinda don't love having this in the dock the more I think about it. I know that's what the survey results indicated, but it feels off to me. I kinda feel like this would make more sense in the notifications center. At least that's the direction it seems iOS and Android have gone for apps communicating ongoing background tasks: |
|
@danirabbit I agree that the dock is not the place to show detailed reports on actively running tasks or state internal to the app. Those should be notifications in the notification center like in your examples. However, I think having a version of a widget like this in the dock, that simply lists running background apps, allows me to kill them and/or to access a right-click context menu, and badges for notifications, is a good idea. We already use the dock for managing what foreground apps are open, and what workspace they're on, so I think it's illogical to treat apps that are running in the background differently. I don't like tray apps, but I feel this is a suitable compromise. |
|
@danirabbit I think @wpkelso summarized it quite nicely. If I'm looking for apps running in the background I'd look where all the other running apps are and not where I can toggle the WIFI or see notifications. I think there is a difference between showing status information about tasks or things happening in the background, and just a list of apps running in the background where I can kill them. |
Yeah I agree that's probably the best. I wonder if there's something we could do to make the ghost look a bit different from the launchers? Maybe a different background like the icon groups or a separator or something? |
The simplest thing is probably to throw a separator between the ghost and the rest of the launchers. I think that effectively creates a "launch" cluster and a "manage" cluster. |
95c387e to
97aad50
Compare
|
I've updated the UI with the suggestions (see screenshot in description) so this should be ready for review now |
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Clicking the quit button closes the popover. We should probably keep it open so you can see the spinner going etc right?
- I also can't confirm that clicking the quit button actually quits the app
That is really weird it shouldn't close the popover and it doesn't for me 😅 No clue what's going on there :/
I think it sometimes it doesn't immediately update. Though for me it now works almost always. When updating this PR only once it didn't work immediately but clicking again closed it right away 🤷 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@leolost2605 okay so for me, in X11 the popover stays open, but in Wayland it closes |
Ohhh I love that idea! Then I guess separator between that icon group and the rest of the icon groups? Or do you have another idea to separate it?
That's still so weird it stays open for me both in X11 and in wayland. And also there is no code whatsoever that would connect the button press to menu close. Also can you confirm that the kill now (at least most of the time) works? Or is there something still very wrong? |
Yeah I think that works really well and makes it more glanceable! Very cool. I think I'm fine with the separator, but it needs a lighter highlight for the translucent style. I'll push a fix for it
Maybe something weird with my install then! I'll see if I can reproduce on another computer
Naw I was able to reproduce it. It's a stylesheet bug: elementary/stylesheet#1325 |
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a really interesting bug. You can drag and drop a workspace and it can replace the background apps group 😅
|
@danirabbit should be fixed now :) |
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think maybe we should remove the item when there are no background apps so it doesn't just take up space in the dock, but I won't block on that. I'm willing to try it in daily for a while if you feel strongly about it.
I have just a few requested changes, but mostly I think this is good to get in :)
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I guess one more problem, the item isn't added if building with workspace switcher is disabled 😅 You just get a separator at the end of the dock for no reason. We should probably make sure this does still build when the workspace switcher is disabled and we only add the separator when the workspace switcher is enabled
|
The item is now only visible whenever it has background apps. Also building with the workspace switcher disabled should be fixed now :) |
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple tiny little nitpicky things, otherwise this looks good!
| Timeout.add_seconds (5, () => { | ||
| // Assume killing failed | ||
| button_stack.set_visible_child_name ("button"); | ||
| return Source.REMOVE; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to come before the try/catch or it'll never be used right? And then we need to cancel it in finally probably?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is the kill is usually pretty quick (even if the action doesn't work and we have to do a flatpak kill no more than a few milliseconds) and pretty much never reports failure (I never saw flatpak kill return something other than success). But even if flatpak kill returns success the app sometimes continues running for some reason?
What really takes a while is for the freedesktop background apps monitor to pick up that the app was killed so even if we get a success we don't know if it has been actually killed and will now soon disappear or if it failed. So I did it like this that if something immediately fails (for example we fail to launch flatpak kill for some reason) we don't even start the timeout and only start it if everything in our power succeeded (which should pretty much always be the case on a correct install?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Thanks for explaining!
|
|
||
| var session_bus = yield Bus.get (SESSION, null); | ||
|
|
||
| yield session_bus.call ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing it was done this way because DesktopAppInfo.launch_action doesn't return errors? If that's the case maybe a comment would be nice so someone doesn't try to refactor it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DesktopAppInfo.launch_action only supports launching actions that are listed in the desktop file. But while a lot of application install a quit action they don't list it in their desktop file because you don't expect a quit if you right click the app in the application menu but we can still activate it remotely.
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explanations! This is great. Let's get it in 🚀





Implements background monitoring and allows to kill apps that are running in the background.
Fixes #99
Closes #395
The current UI:
