Changeset View
Standalone View
src/panels/terminal/terminalpanel.cpp
Show All 13 Lines | |||||
14 | * You should have received a copy of the GNU General Public License * | 14 | * You should have received a copy of the GNU General Public License * | ||
15 | * along with this program; if not, write to the * | 15 | * along with this program; if not, write to the * | ||
16 | * Free Software Foundation, Inc., * | 16 | * Free Software Foundation, Inc., * | ||
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/DesktopExecParser> | ||||
22 | #include <KIO/Job> | 23 | #include <KIO/Job> | ||
23 | #include <KIO/JobUiDelegate> | 24 | #include <KIO/JobUiDelegate> | ||
24 | #include <KJobWidgets> | 25 | #include <KJobWidgets> | ||
26 | #include <KLocalizedString> | ||||
27 | #include <KMessageWidget> | ||||
25 | #include <KParts/ReadOnlyPart> | 28 | #include <KParts/ReadOnlyPart> | ||
26 | #include <KPluginFactory> | 29 | #include <KPluginFactory> | ||
27 | #include <KPluginLoader> | 30 | #include <KPluginLoader> | ||
28 | #include <KService> | 31 | #include <KService> | ||
29 | #include <KShell> | 32 | #include <KShell> | ||
30 | #include <kde_terminal_interface.h> | 33 | #include <kde_terminal_interface.h> | ||
31 | 34 | | |||
35 | #include <QAction> | ||||
36 | #include <QDesktopServices> | ||||
ngraham: Are these all really necessary to this patch? I thought we already fixed the #includes before? | |||||
32 | #include <QDir> | 37 | #include <QDir> | ||
38 | #include <QLabel> | ||||
33 | #include <QShowEvent> | 39 | #include <QShowEvent> | ||
40 | #include <QTimer> | ||||
34 | #include <QVBoxLayout> | 41 | #include <QVBoxLayout> | ||
35 | 42 | | |||
36 | TerminalPanel::TerminalPanel(QWidget* parent) : | 43 | TerminalPanel::TerminalPanel(QWidget* parent) : | ||
37 | Panel(parent), | 44 | Panel(parent), | ||
38 | m_clearTerminal(true), | 45 | m_clearTerminal(true), | ||
39 | m_mostLocalUrlJob(nullptr), | 46 | m_mostLocalUrlJob(nullptr), | ||
40 | m_layout(nullptr), | 47 | m_layout(nullptr), | ||
41 | m_terminal(nullptr), | 48 | m_terminal(nullptr), | ||
▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Line(s) | 130 | if (!m_terminal) { | |||
127 | if (service) { | 134 | if (service) { | ||
128 | factory = KPluginLoader(service->library()).factory(); | 135 | factory = KPluginLoader(service->library()).factory(); | ||
129 | } | 136 | } | ||
130 | m_konsolePart = factory ? (factory->create<KParts::ReadOnlyPart>(this)) : nullptr; | 137 | m_konsolePart = factory ? (factory->create<KParts::ReadOnlyPart>(this)) : nullptr; | ||
131 | if (m_konsolePart) { | 138 | if (m_konsolePart) { | ||
132 | connect(m_konsolePart, &KParts::ReadOnlyPart::destroyed, this, &TerminalPanel::terminalExited); | 139 | connect(m_konsolePart, &KParts::ReadOnlyPart::destroyed, this, &TerminalPanel::terminalExited); | ||
133 | m_terminalWidget = m_konsolePart->widget(); | 140 | m_terminalWidget = m_konsolePart->widget(); | ||
134 | m_layout->addWidget(m_terminalWidget); | 141 | m_layout->addWidget(m_terminalWidget); | ||
142 | if (m_konsolePartMissingMessage) { | ||||
143 | m_layout->removeWidget(m_konsolePartMissingMessage); | ||||
144 | } | ||||
135 | m_terminal = qobject_cast<TerminalInterface*>(m_konsolePart); | 145 | m_terminal = qobject_cast<TerminalInterface*>(m_konsolePart); | ||
146 | } else if (!m_konsolePartMissingMessage) { | ||||
147 | const auto konsoleInstallUrl = QUrl("appstream://org.kde.konsole.desktop"); | ||||
148 | const auto konsoleNotInstalledText = i18n("Terminal cannot be shown because Konsole is not installed. " | ||||
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/) | |||||
149 | "Please install it and then reopen the panel."); | ||||
elvisangelaccio: Please align with "Terminal" above. | |||||
150 | m_konsolePartMissingMessage = new KMessageWidget(konsoleNotInstalledText, this); | ||||
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… | |||||
151 | m_konsolePartMissingMessage->setCloseButtonVisible(false); | ||||
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. | |||||
152 | m_konsolePartMissingMessage->hide(); | ||||
153 | if (KIO::DesktopExecParser::hasSchemeHandler(konsoleInstallUrl)) { | ||||
154 | auto installKonsoleAction = new QAction(i18n("Install Konsole"), this); | ||||
155 | connect(installKonsoleAction, &QAction::triggered, [konsoleInstallUrl]() { | ||||
elvisangelaccio: Please add `this` as parent. | |||||
156 | QDesktopServices::openUrl(konsoleInstallUrl); | ||||
157 | }); | ||||
ngraham: "Polling" | |||||
158 | m_konsolePartMissingMessage->addAction(installKonsoleAction); | ||||
159 | } | ||||
160 | m_layout->addWidget(m_konsolePartMissingMessage); | ||||
161 | m_layout->addStretch(); | ||||
162 | QTimer::singleShot(0, m_konsolePartMissingMessage, &KMessageWidget::animatedShow); | ||||
163 | } else { | ||||
164 | m_konsolePartMissingMessage->animatedShow(); | ||||
136 | } | 165 | } | ||
137 | } | 166 | } | ||
138 | if (m_terminal) { | 167 | if (m_terminal) { | ||
139 | m_terminal->showShellInDir(url().toLocalFile()); | 168 | m_terminal->showShellInDir(url().toLocalFile()); | ||
140 | changeDir(url()); | 169 | changeDir(url()); | ||
141 | m_terminalWidget->setFocus(); | 170 | m_terminalWidget->setFocus(); | ||
142 | connect(m_konsolePart, SIGNAL(currentDirectoryChanged(QString)), | 171 | connect(m_konsolePart, SIGNAL(currentDirectoryChanged(QString)), | ||
143 | this, SLOT(slotKonsolePartCurrentDirectoryChanged(QString))); | 172 | this, SLOT(slotKonsolePartCurrentDirectoryChanged(QString))); | ||
▲ Show 20 Lines • Show All 82 Lines • Show Last 20 Lines |
Are these all really necessary to this patch? I thought we already fixed the #includes before?