Show a message if Konsole part is not installed
ClosedPublic

Authored by rominf on Mar 7 2018, 11:36 AM.

Details

Summary

Show a message if Konsole part is not installed

BUG: 371822
FIXED-IN: 18.04.0

Diff Detail

Repository
R318 Dolphin
Branch
konsole-part-missing
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
rominf requested review of this revision.Mar 7 2018, 11:36 AM
rominf edited the summary of this revision. (Show Details)Mar 7 2018, 11:38 AM
progwolff added inline comments.
src/panels/terminal/terminalpanel.cpp
140

Package is called "konsole" on Arch Linux. I would leave out this part.

rominf updated this revision to Diff 28925.Mar 7 2018, 12:44 PM
rominf edited the summary of this revision. (Show Details)

Hide a message when Konsole part is installed during the Dolphin session.

broulik added a subscriber: broulik.Mar 7 2018, 2:10 PM
broulik added inline comments.
src/panels/terminal/terminalpanel.cpp
140

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 ;)

rominf added inline comments.Mar 7 2018, 2:13 PM
src/panels/terminal/terminalpanel.cpp
140

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.

140

The user may want to use Konsole only as a part of Dolphin.

ngraham edited the summary of this revision. (Show Details)Mar 7 2018, 2:41 PM
elvisangelaccio added inline comments.
src/panels/terminal/terminalpanel.cpp
139–140

How about using a KMessageWidget instead? (see e.g. https://git.reviewboard.kde.org/r/130007/)

rominf updated this revision to Diff 28994.Mar 8 2018, 9:09 AM

Replace QLabel with KMessageWidget

rominf marked 5 inline comments as done.Mar 8 2018, 9:10 AM
ngraham added a subscriber: ngraham.Mar 8 2018, 2:55 PM

+1 on using a KMessageWidget here. They're really nice for this kind of transient, non-blocking notification.

src/panels/terminal/terminalpanel.cpp
140

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).

rominf added inline comments.Mar 8 2018, 3:54 PM
src/panels/terminal/terminalpanel.cpp
140

@ngraham, can you please provide a final variant of the message?

ngraham added inline comments.Mar 8 2018, 4:20 PM
src/panels/terminal/terminalpanel.cpp
140

@rominf How about:

"Terminal panel cannot be shown because Konsole is not installed."

Then the KMessageWidget could have a button with the text "Install Konsole" that, when clicked, opens appstream://org.kde.konsole.desktop

rominf updated this revision to Diff 29025.Mar 8 2018, 5:21 PM
  • 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.

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.

Works great for me!

Perhaps openSUSE has lousy AppStream metadata. what does this say on your system?
appstreamcli search konsole

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.

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.

[...] and Distros should make sure that they ship with a handler app for them.

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.

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.

rominf updated this revision to Diff 29032.Mar 8 2018, 6:36 PM

Show message that Konsole is not installed even after closing

rominf added a comment.Mar 8 2018, 6:40 PM

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.

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!

rominf edited the summary of this revision. (Show Details)Mar 8 2018, 6:41 PM

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.

rkflx added a subscriber: rkflx.Mar 8 2018, 7:26 PM

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.

+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.


Perhaps openSUSE has lousy AppStream metadata.

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…).

Perhaps openSUSE has lousy AppStream metadata.

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…).

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.

rominf updated this revision to Diff 29037.Mar 8 2018, 7:57 PM
  • Improve UI for Konsole missing message
rominf added a comment.Mar 8 2018, 7:59 PM

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?).

rkflx added a comment.Mar 8 2018, 8:00 PM

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?).

Did you look at how Gwenview is doing it?

rkflx added a comment.EditedMar 8 2018, 8:06 PM

Is the UI better now? Note the animation and layout.

👍

(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?)

Is the UI better now? Note the animation and layout.

+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:

rominf updated this revision to Diff 29038.Mar 8 2018, 8:16 PM
  • Hide Install Konsole button if OS doesn't handle appstream
  • Hide close button
ngraham requested changes to this revision.EditedMar 8 2018, 8:23 PM

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
143

"Try installing the 'konsole' package with your package manager"

This revision now requires changes to proceed.Mar 8 2018, 8:23 PM
rkflx added a comment.EditedMar 8 2018, 8:25 PM
This comment has been deleted.
src/panels/terminal/terminalpanel.cpp
143

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…

rominf updated this revision to Diff 29039.Mar 8 2018, 8:30 PM
  • Replace space with newline in Konsole missing message
  • Fix crash on appstream installation
rominf marked 3 inline comments as done.Mar 8 2018, 8:31 PM

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
143

+1.

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.

progwolff accepted this revision.Mar 9 2018, 10:31 AM
  • Try to break it every way we can

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.

rominf marked 3 inline comments as done.Mar 9 2018, 10:52 AM

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.

rominf updated this revision to Diff 29103.Mar 9 2018, 5:06 PM
  • Automaticaly load console on installation
ngraham added inline comments.Mar 9 2018, 5:13 PM
src/panels/terminal/terminalpanel.cpp
34

Are these all really necessary to this patch? I thought we already fixed the #includes before?

149

"Polling"

rominf updated this revision to Diff 29104.Mar 9 2018, 5:16 PM
rominf marked an inline comment as done.
  • Remove unused #include
rominf marked an inline comment as done.Mar 9 2018, 5:16 PM
rominf updated this revision to Diff 29105.Mar 9 2018, 5:22 PM
  • Fix hideEvent
elvisangelaccio requested changes to this revision.Mar 10 2018, 11:57 AM

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.

This revision now requires changes to proceed.Mar 10 2018, 11:57 AM
rominf updated this revision to Diff 29203.Mar 11 2018, 7:44 AM
  • Show a message about reopening the terminal panel instead of polling
ngraham accepted this revision as: ngraham.Mar 11 2018, 3:43 PM

I am satisfied now! This is great stuff. Really user-friendly.

Is this the intended design? It looks different from the screenshot in OP (mine is not vertical centered)

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.

rominf edited the summary of this revision. (Show Details)Mar 11 2018, 4:53 PM
src/panels/terminal/terminalpanel.cpp
141

Please align with "Terminal" above.

142

This variable is used only once, we can remove it and just do

if (KIO::DesktopExecParser::hasSchemeHandler(konsoleInstallUrl)) {
    ...
}
143

Please add this as parent.

147

Please add this as parent.

rominf updated this revision to Diff 29255.Mar 11 2018, 5:18 PM
rominf marked 4 inline comments as done.
  • Take criticism into account
elvisangelaccio removed a reviewer: Dolphin.
This revision is now accepted and ready to land.Mar 11 2018, 5:40 PM
This revision was automatically updated to reflect the committed changes.

Awesome! This is a really nice feature.