Changeset View
Standalone View
src/panels/terminal/terminalpanel.cpp
| Show All 16 Lines | |||||
| 17 | * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA * | 17 | * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA * | ||
| 18 | ***************************************************************************/ | 18 | ***************************************************************************/ | ||
| 19 | 19 | | |||
| 20 | #include "terminalpanel.h" | 20 | #include "terminalpanel.h" | ||
| 21 | 21 | | |||
| 22 | #include <KIO/Job> | 22 | #include <KIO/Job> | ||
| 23 | #include <KIO/JobUiDelegate> | 23 | #include <KIO/JobUiDelegate> | ||
| 24 | #include <KJobWidgets> | 24 | #include <KJobWidgets> | ||
| 25 | #include <KLocalizedString> | ||||
| 26 | #include <KMessageWidget> | ||||
| 25 | #include <KParts/ReadOnlyPart> | 27 | #include <KParts/ReadOnlyPart> | ||
| 26 | #include <KPluginFactory> | 28 | #include <KPluginFactory> | ||
| 27 | #include <KPluginLoader> | 29 | #include <KPluginLoader> | ||
| 28 | #include <KService> | 30 | #include <KService> | ||
| 29 | #include <KShell> | 31 | #include <KShell> | ||
| 30 | #include <kde_terminal_interface.h> | 32 | #include <kde_terminal_interface.h> | ||
| 31 | 33 | | |||
| 34 | #include <QAction> | ||||
| 35 | #include <QDesktopServices> | ||||
ngraham: Are these all really necessary to this patch? I thought we already fixed the #includes before? | |||||
| 32 | #include <QDir> | 36 | #include <QDir> | ||
| 37 | #include <QLabel> | ||||
| 33 | #include <QShowEvent> | 38 | #include <QShowEvent> | ||
| 34 | #include <QVBoxLayout> | 39 | #include <QVBoxLayout> | ||
| 35 | 40 | | |||
| 36 | TerminalPanel::TerminalPanel(QWidget* parent) : | 41 | TerminalPanel::TerminalPanel(QWidget* parent) : | ||
| 37 | Panel(parent), | 42 | Panel(parent), | ||
| 38 | m_clearTerminal(true), | 43 | m_clearTerminal(true), | ||
| 39 | m_mostLocalUrlJob(nullptr), | 44 | m_mostLocalUrlJob(nullptr), | ||
| 40 | m_layout(nullptr), | 45 | m_layout(nullptr), | ||
| ▲ Show 20 Lines • Show All 86 Lines • ▼ Show 20 Line(s) | 128 | if (!m_terminal) { | |||
| 127 | if (service) { | 132 | if (service) { | ||
| 128 | factory = KPluginLoader(service->library()).factory(); | 133 | factory = KPluginLoader(service->library()).factory(); | ||
| 129 | } | 134 | } | ||
| 130 | m_konsolePart = factory ? (factory->create<KParts::ReadOnlyPart>(this)) : nullptr; | 135 | m_konsolePart = factory ? (factory->create<KParts::ReadOnlyPart>(this)) : nullptr; | ||
| 131 | if (m_konsolePart) { | 136 | if (m_konsolePart) { | ||
| 132 | connect(m_konsolePart, &KParts::ReadOnlyPart::destroyed, this, &TerminalPanel::terminalExited); | 137 | connect(m_konsolePart, &KParts::ReadOnlyPart::destroyed, this, &TerminalPanel::terminalExited); | ||
| 133 | m_terminalWidget = m_konsolePart->widget(); | 138 | m_terminalWidget = m_konsolePart->widget(); | ||
| 134 | m_layout->addWidget(m_terminalWidget); | 139 | m_layout->addWidget(m_terminalWidget); | ||
| 140 | if (m_konsolePartMissingMessage) { | ||||
| 141 | m_layout->removeWidget(m_konsolePartMissingMessage); | ||||
| 142 | } | ||||
| 135 | m_terminal = qobject_cast<TerminalInterface*>(m_konsolePart); | 143 | m_terminal = qobject_cast<TerminalInterface*>(m_konsolePart); | ||
| 144 | } else if (!m_konsolePartMissingMessage || !m_konsolePartMissingMessage->isVisible()) { | ||||
| 145 | m_konsolePartMissingMessage = new KMessageWidget(i18n("Terminal cannot be shown because Konsole is not installed.")); | ||||
| 146 | auto installKonsoleAction = new QAction(i18n("Install Konsole")); | ||||
progwolff: Package is called "konsole" on Arch Linux. I would leave out this part. | |||||
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. rominf: I don't like guessing, but it's better to provide the user hints on what she should install. | |||||
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 ;) broulik: Doesn't installing "Konsole" imply Konsole part being there? So I would simplify it to… | |||||
rominf: The user may want to use Konsole only as a part of Dolphin. | |||||
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). ngraham: I think it's safe enough to install all of Konsole. +1 for calling an AppStream URL (`appstream… | |||||
@ngraham, can you please provide a final variant of the message? rominf: @ngraham, can you please provide a final variant of the message? | |||||
@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 ngraham: @rominf How about:
"Terminal panel cannot be shown because Konsole is not installed."
Then… | |||||
How about using a KMessageWidget instead? (see e.g. https://git.reviewboard.kde.org/r/130007/) elvisangelaccio: How about using a KMessageWidget instead? (see e.g. https://git.reviewboard.kde.org/r/130007/) | |||||
| 147 | connect(installKonsoleAction, &QAction::triggered, []() { | ||||
elvisangelaccio: Please align with "Terminal" above. | |||||
| 148 | QDesktopServices::openUrl(QUrl("appstream://org.kde.konsole.desktop")); | ||||
This variable is used only once, we can remove it and just do if (KIO::DesktopExecParser::hasSchemeHandler(konsoleInstallUrl)) {
...
}elvisangelaccio: This variable is used only once, we can remove it and just do
if (KIO::DesktopExecParser… | |||||
| 149 | }); | ||||
ngraham: "Try installing the 'konsole' package with your package manager" | |||||
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… rkflx: The space before "Try" is a bit odd, I'd guess translators might just omit it.
Why not simply… | |||||
ngraham: +1. | |||||
elvisangelaccio: Please add `this` as parent. | |||||
| 150 | m_konsolePartMissingMessage->addAction(installKonsoleAction); | ||||
| 151 | m_layout->addWidget(m_konsolePartMissingMessage); | ||||
| 136 | } | 152 | } | ||
| 137 | } | 153 | } | ||
elvisangelaccio: Please add `this` as parent. | |||||
| 138 | if (m_terminal) { | 154 | if (m_terminal) { | ||
| 139 | m_terminal->showShellInDir(url().toLocalFile()); | 155 | m_terminal->showShellInDir(url().toLocalFile()); | ||
ngraham: "Polling" | |||||
| 140 | changeDir(url()); | 156 | changeDir(url()); | ||
| 141 | m_terminalWidget->setFocus(); | 157 | m_terminalWidget->setFocus(); | ||
| 142 | connect(m_konsolePart, SIGNAL(currentDirectoryChanged(QString)), | 158 | connect(m_konsolePart, SIGNAL(currentDirectoryChanged(QString)), | ||
| 143 | this, SLOT(slotKonsolePartCurrentDirectoryChanged(QString))); | 159 | this, SLOT(slotKonsolePartCurrentDirectoryChanged(QString))); | ||
| 144 | } | 160 | } | ||
| 145 | 161 | | |||
| 146 | Panel::showEvent(event); | 162 | Panel::showEvent(event); | ||
| 147 | } | 163 | } | ||
| ▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines | |||||
Are these all really necessary to this patch? I thought we already fixed the #includes before?