Show a message if Konsole part is not installed
BUG: 371822
FIXED-IN: 18.04.0
Details
Details
Diff Detail
Diff Detail
- Repository
- R318 Dolphin
- Branch
- konsole-part-missing (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 161 | Package is called "konsole" on Arch Linux. I would leave out this part. | |
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 161 | Doesn't installing "Konsole" imply Konsole part being there? So I would simplify it to "Terminal cannot be shown because Konsole is not installed." - bonus points for using AppStream to open Discover to install it ;) | |
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 161 | I don't like guessing, but it's better to provide the user hints on what she should install. I'm suggesting listing all variants. After all, this text shouldn't be beautiful, as the user will read it only once. He either installs the required package and the text will gone, or not, but then he won't open terminal panel anymore. | |
| 161 | The user may want to use Konsole only as a part of Dolphin. | |
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 160–161 | How about using a KMessageWidget instead? (see e.g. https://git.reviewboard.kde.org/r/130007/) | |
Comment Actions
+1 on using a KMessageWidget here. They're really nice for this kind of transient, non-blocking notification.
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 161 | I think it's safe enough to install all of Konsole. +1 for calling an AppStream URL (appstream://org.kde.konsole.desktop), which will show Konsole in whatever the user's software center is. It may not be Discover, since the liklihood of using a KDE-centric distro that doesn't ship Konsole is somewhat low (I guess it's possible on Arch, but in that case, the user probably wouldn't have Discover installed either). | |
Comment Actions
- Add install Konsole button
@ngraham, did you mean something like this? This button doesn't work for me (openSUSE Thumbleweed) even after appstream package installation.
Comment Actions
Works great for me!
Perhaps openSUSE has lousy AppStream metadata. what does this say on your system?
appstreamcli search konsole
Found a bug: If you click the X button to dismiss the message, then close and re-open the terminal panel, it doesn't show up again.
I have some UI suggestions:
- Don't even try to open the terminal panel, and instead show the KMessageWidget at the top of the main view, like other KMessageWidgets in Dolphin.
- Since Dolphin has to be restarted after Konsole is installed, either mention this in the initial message (easy) or make Dolphin detect that Konsole has been newly-installed and show a new message with a "restart Dolphin" button (hard). If you implement my first UI suggestion, bonus points if after the re-launch, Dolphin opens with the Terminal panel displayed! That would be really, really slick.
Comment Actions
appstream itself does not handle appstream:// urls. You will need a frontend like discover for this, otherwise xdg-open appstream://org.kde.konsole.desktop shows Unsupported URL scheme.
Not sure if there is a way to find out if there is an appstream frontend installed.
Comment Actions
Right, Appstream is just the protocol, and Discover handles appstream:// URLs. So if you don't have Discover (or another appstream:// URL handler) installed, then nothing will happen. The primary target audience here is going to be users of non-KDE distros who like Dolphin, since most KDE distros will ship with both Konsole and Discover. On GNOME-based non-KDE distros, GNOME Software will be opened. But all of this is distro-specific stuff; opening an appstream:// URL seems perfectly reasonable to me, and Distros should make sure that they ship with a handler app for them.
Comment Actions
And users of distros that don't ship one will probably know how to install the right package. Nonetheless it might be a good idea to use the return value of openUrl and show an error message on failure instead of just doing nothing.
Comment Actions
Yes, that's a good idea. Since we can probably assume you're a power user if you don't have an appstream:// URL handler installed, maybe we should at that point display a more technical message indicating possible names of the package you can give to your your command-line package management utility.
Comment Actions
Fixed.
I have some UI suggestions:
- Don't even try to open the terminal panel, and instead show the KMessageWidget at the top of the main view, like other KMessageWidgets in Dolphin.
I do not agree. My variant approximately shows how the things should look like after Konsole installation.
- Since Dolphin has to be restarted after Konsole is installed...
No, it has not. Just reopen panel, that's it.
I think that this patch is good enough for a quite rare problem, let's ship it!
Comment Actions
What I don't like is how the KMessageWidget is right in the middle of the panel, really. I'd accept the current UI if we display it at the top of the panel, rather than in the middle.
The "no appstream:// Url handler installed" message from KIO is accurate, but kind of unpleasant right now, and it doesn't give the user a clear path forward. It would be nice if we provided our own message that offered rudimentary instructions to help the user install the necessary packages using the command line, like your original patch did. This is that right place for that!
And what do you think about the idea of starting a watcher for the presence of Konsole if the URL open task returns 0, and once it's installed, automatically re-opening the panel? That way, the moment Konsole is installed, the panel can instantly work! That would be a truly fantastic UI, and people will fall in love with it.
Let's ship it!
When it's done, we certainly will! :-) The purpose of this review process is not to be an annoying gate, but to make sure that code that gets committed is of the highest quality and doesn't require follow-up commits to fix bugs or improve the UI. People get busy or lose interest and those follow-up -patches often don't materialize. It's important to get it right in the first go as often as we can.
Comment Actions
+1
The "no appstream:// Url handler installed" message from KIO is accurate, but kind of unpleasant right now, and it doesn't give the user a clear path forward. It would be nice if we provided our own message that offered rudimentary instructions to help the user install the necessary packages using the command line, like your original patch did. This is that right place for that!
Try looking at Gwenview's Plugins menu. When I have Discover installed, it offers to install KIPI plugins, if not, it just says "No plugins found". As for giving manual instructions, I think that won't work, given the variety of distros there are. I would just disable the install button here, no need to make it too complicated.
Let's ship it!
When it's done, we certainly will! :-) The purpose of this review process is not to be an annoying gate, but to make sure that code that gets committed is of the highest quality and doesn't require follow-up commits to fix bugs or improve the UI. People get busy or lose interest and those follow-up -patches often don't materialize. It's important to get it right in the first go as often as we can.
Hear, hear! Also, this needs code review from experienced Dolphin developers.
As someone involved with the topic, you should know better than spreading cheap FUD. Of course it works fine on openSUSE, you just have to install Discover (that's where the problem might be, but that probably also depends on the "readiness" of Discover…).
Comment Actions
The "perhaps" was there for a reason, because I was wondering, rather than making a statement (I have no idea how good or bad openSUSE's AppStream metadata is). In fact bad data is sadly common: Ubuntu for example really *does* have Lousy AppStream metadata, and this won't work in Kubuntu 17.10 because it doesn't have appstream://org.kde.konsole.desktop (thankfully it will work in 18.04, where the AppStream metadata is less lousy).
If openSUSE doesn't want to ship with Discover, they should make YaST handle appstream:// URLs.
Comment Actions
Is the UI better now? Note the animation and layout.
As for good message for a user, I don't know how to check whether appstream:// is supported or not. The return value for openUrl doesn't help, it's true even if it fails (bug or maybe it's just me?).
Comment Actions
👍
(I only wonder whether we need the close button, as it does not really do anything particularly useful and the message will re-appear after the next start anyway. Does KMessageWidget allow to disable it?)
Comment Actions
+1 now!
As for good message for a user, I don't know how to check whether appstream:// is supported or not. The return value for openUrl doesn't help, it's true even if it fails (bug or maybe it's just me?).
For me, without Discover installed, I get a KIO error message after I click the button:
Comment Actions
Very nice. One textual nitpick below.
Alas, now Dolphin crashes in openUrl() when I click the "Install Konsole" button when Discover is installed. Please remember to keep testing as you go. Here's the full backtrace:
Thread 1 "dolphin" received signal SIGSEGV, Segmentation fault.
0x00007ffff17bcb10 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
(gdb) bt
#0 0x00007ffff17bcb10 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#1 0x00007ffff2435826 in QDesktopServices::openUrl(QUrl const&) ()
from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#2 0x00007ffff7b4893b in TerminalPanel::showEvent(QShowEvent*)::{lambda()#2}::operator()() const
() from /usr/lib/x86_64-linux-gnu/libkdeinit5_dolphin.so
#3 0x00007ffff7b4984f in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, TerminalPanel::showEvent(QShowEvent*)::{lambda()#2}>::call({lambda()#2}&, void**) ()
from /usr/lib/x86_64-linux-gnu/libkdeinit5_dolphin.so
#4 0x00007ffff7b49821 in void QtPrivate::Functor<TerminalPanel::showEvent(QShowEvent*)::{lambda()#2}, 0>::call<QtPrivate::List<>, void>({lambda()#2}&, void*, {lambda()#2}&*) ()
from /usr/lib/x86_64-linux-gnu/libkdeinit5_dolphin.so
#5 0x00007ffff7b497ef in QtPrivate::QFunctorSlotObject<TerminalPanel::showEvent(QShowEvent*)::{lambda()#2}, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) () from /usr/lib/x86_64-linux-gnu/libkdeinit5_dolphin.so
#6 0x00007ffff19787b6 in QMetaObject::activate(QObject*, int, int, void**) ()
from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#7 0x00007ffff29114a2 in QAction::triggered(bool) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#8 0x00007ffff2913c10 in QAction::activate(QAction::ActionEvent) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#9 0x00007ffff2a10900 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#10 0x00007ffff2a10a3d in QAbstractButton::mouseReleaseEvent(QMouseEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#11 0x00007ffff2aed0da in QToolButton::mouseReleaseEvent(QMouseEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#12 0x00007ffff2956f88 in QWidget::event(QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#13 0x00007ffff2aed1b9 in QToolButton::event(QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#14 0x00007ffff2917acc in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#15 0x00007ffff292003b in QApplication::notify(QObject*, QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#16 0x00007ffff194b3c8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#17 0x00007ffff291e9df in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#18 0x00007ffff2970f4d in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#19 0x00007ffff297397b in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#20 0x00007ffff2917acc in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#21 0x00007ffff291f417 in QApplication::notify(QObject*, QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#22 0x00007ffff194b3c8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#23 0x00007ffff216fef0 in QGuiApplicationPrivate::processMouseEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#24 0x00007ffff2171e45 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#25 0x00007ffff214b8fb in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#26 0x00007fffe29671ab in ?? () from /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5
#27 0x00007ffff194964a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#28 0x00007ffff1952854 in QCoreApplication::exec() ()
from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#29 0x00007ffff7af3c9d in kdemain () from /usr/lib/x86_64-linux-gnu/libkdeinit5_dolphin.so
#30 0x00000000004007d8 in main ()| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 164 | "Try installing the 'konsole' package with your package manager" | |
Comment Actions
This comment has been deleted.
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 164 | The space before "Try" is a bit odd, I'd guess translators might just omit it. Why not simply add a newline between both messages? Would help for languages with longer translations too… | |
Comment Actions
- Replace space with newline in Konsole missing message
- Fix crash on appstream installation
Comment Actions
Wonderful! Now there are only two more things we need to do, from my perspective:
- Try to break it every way we can
- Investigate automatically re-opening the panel as soon as Dolphin detects that Konsole has been installed (if that's possible, or feasible)
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 164 | +1. | |
Comment Actions
I really like that install button. Maybe it could be added to KWidgetsAddons?
I see usecases in Okular for installing backends or in Telepathy for installing plugins.
Comment Actions
Couldn't break it so far
- Investigate automatically re-opening the panel as soon as Dolphin detects that Konsole has been installed (if that's possible, or feasible)
I don't think this is feasible without polling. And I don't think polling would be a good idea here.
Looks good from my perspective.
Comment Actions
So far I can't break it either. Looking pretty good!
if we don't want to poll, could we adjust the message to say something like "To show the Terminal Panel correctly, please close and re-open it after installing Konsole."
The reason why I'm pushing hard on this is because people will install Konsole and then say, "OK, now what? It still doesn't work." If we can't do it for them, it needs to be super obvious what they need to do.
Comment Actions
Hmm, I disagree with polling. We are going to add a lot of non-trivial code (which will have to be maintained forever) just to handle a corner case.
We can just tell the user to reopen the terminal panel, imho that's more than enough.
Comment Actions
Is this the intended design? It looks different from the screenshot in OP (mine is not vertical centered)
Comment Actions
Yes that's now the intended design. We all preferred having the KMessageWidget at the top of the panel rather than in the center, to match the location of other KMessageWidgets in Dolphin.
| src/panels/terminal/terminalpanel.cpp | ||
|---|---|---|
| 163 ↗ | (On Diff #29103) | Please align with "Terminal" above. |
| 164 ↗ | (On Diff #29103) | This variable is used only once, we can remove it and just do if (KIO::DesktopExecParser::hasSchemeHandler(konsoleInstallUrl)) {
...
} |
| 165 ↗ | (On Diff #29103) | Please add this as parent. |
| 169 ↗ | (On Diff #29103) | Please add this as parent. |