Throw toast when app is installed#1456
Conversation
Co-authored-by: Cassidy James Blaede <cassidy@elementary.io>
Co-authored-by: Cassidy James Blaede <cassidy@elementary.io>
|
Screenshot from list view
|
cassidyjames
left a comment
There was a problem hiding this comment.
UX approval from me. My only open question is if it would make sense to suppress the toast on the app's own app info view (since it's redundant with the existing "Open" button that gets revealed), but that's not a blocker for me.
|
I will look into hiding the toast on the app's own app info view. |
src/Application.vala
Outdated
| var toast = main_window.toast; | ||
| toast.title = _("“%s” has been installed").printf (package.get_name ()); | ||
|
|
||
| toast.send_notification (); |
There was a problem hiding this comment.
It would be better to handle sending the toast and saving the package reference inside MainWindow, instead of trying to expose last_installed_package in Application and then accessing it back in MainWindow.
Simply declare a private AppCenterCore.Package? last_installed_package member and a public void send_installed_toast (AppCenterCore.Package package) method in MainWindow and manage sending the toast there instead.
There was a problem hiding this comment.
I now also need selected_package in AppCenter.App to check if a Toast should be sent.
There was a problem hiding this comment.
I mean sure, but that didn't resolve my issue. The toast is still half managed in Application and MainWindow at the same time. And there's also no need to store selected_package globaly inside Application. Notice how MainWindow actually calls show_package and return_clicked where this property could be set within MainWindow as well.
There was a problem hiding this comment.
Thanks for your feedback. I have refactored that except for selected_package.
The show_package call in MainWindow is not the same as in View.
There was a problem hiding this comment.
I refactored it using a signal.
@donadigo thanks for being persistent 😃 The code looks much cleaner now 👍
cassidyjames
left a comment
There was a problem hiding this comment.
Again, UX-wise this meets my approval. 😄 But I'd like to see a re-review from @elementary/desktop-developers or @donadigo.
danirabbit
left a comment
There was a problem hiding this comment.
Just a few inline comments. Otherwise, looks pretty clean to me :)
| try { | ||
| last_installed_package.launch (); | ||
| } catch (Error e) { | ||
| warning ("Failed to launch %s: %s".printf (last_installed_package.get_name (), e.message)); |
There was a problem hiding this comment.
Hm, I wonder if this should throw a dialog. If a user clicked the open button and then nothing happened, that seems bad. Thoughts @cassidyjames?
There was a problem hiding this comment.
For reference: This code is mostly a copy from AbstractAppContainer.launch_package_app()
There was a problem hiding this comment.
@danrabbit I'm indifferent. It probably makes sense to do what we do in other instances in AppCenter, and then we can file an issue to throw a dialog on failure.
Co-authored-by: Daniel Foré <daniel@elementary.io>
Closes #1455