Differential D11118 Diff 29037 src/panels/terminal/terminalpanel.cpp

Changeset View

Standalone View

View Options

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>
ngrahamUnsubmitted
Done

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

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>
39​#include <QTimer>
34#include <QVBoxLayout> 40​#include <QVBoxLayout>
35 41​
36TerminalPanel::TerminalPanel(QWidget* parent) : 42​TerminalPanel::TerminalPanel(QWidget* parent) :
37 Panel(parent), 43​ Panel(parent),
38 m_clearTerminal(true), 44​ m_clearTerminal(true),
39 m_mostLocalUrlJob(nullptr), 45​ m_mostLocalUrlJob(nullptr),
40 m_layout(nullptr), 46​ m_layout(nullptr),
41 m_terminal(nullptr), 47​ m_terminal(nullptr),
▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Line(s)129 if (!m_terminal) {
127 if (service) { 133​ if (service) {
128 factory = KPluginLoader(service->library()).factory(); 134​ factory = KPluginLoader(service->library()).factory();
129 } 135​ }
130 m_konsolePart = factory ? (factory->create<KParts::ReadOnlyPart>(this)) : nullptr; 136​ m_konsolePart = factory ? (factory->create<KParts::ReadOnlyPart>(this)) : nullptr;
131 if (m_konsolePart) { 137​ if (m_konsolePart) {
132 connect(m_konsolePart, &KParts::ReadOnlyPart::destroyed, this, &TerminalPanel::terminalExited); 138​ connect(m_konsolePart, &KParts::ReadOnlyPart::destroyed, this, &TerminalPanel::terminalExited);
133 m_terminalWidget = m_konsolePart->widget(); 139​ m_terminalWidget = m_konsolePart->widget();
134 m_layout->addWidget(m_terminalWidget); 140​ m_layout->addWidget(m_terminalWidget);
141​ if (m_konsolePartMissingMessage) {
142​ m_layout->removeWidget(m_konsolePartMissingMessage);
143​ }
135 m_terminal = qobject_cast<TerminalInterface*>(m_konsolePart); 144​ m_terminal = qobject_cast<TerminalInterface*>(m_konsolePart);
145​ } else if (!m_konsolePartMissingMessage) {
146​ m_konsolePartMissingMessage = new KMessageWidget(i18n("Terminal cannot be shown because Konsole is not installed."));
147​ m_konsolePartMissingMessage->hide();
progwolffUnsubmitted
Done

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

progwolff: Package is called "konsole" on Arch Linux. I would leave out this part.
rominfAuthorUnsubmitted
Done

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.
broulikUnsubmitted
Done

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…
rominfAuthorUnsubmitted
Done

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

rominf: The user may want to use Konsole only as a part of Dolphin.
ngrahamUnsubmitted
Done

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…
rominfAuthorUnsubmitted
Done

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

rominf: @ngraham, can you please provide a final variant of the message?
ngrahamUnsubmitted
Done

@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…
elvisangelaccioUnsubmitted
Done

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/)
148​ auto installKonsoleAction = new QAction(i18n("Install Konsole"));
elvisangelaccioUnsubmitted
Done

Please align with "Terminal" above.

elvisangelaccio: Please align with "Terminal" above.
149​ connect(installKonsoleAction, &QAction::triggered, []() {
elvisangelaccioUnsubmitted
Done

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…
150​ QDesktopServices::openUrl(QUrl("appstream://org.kde.konsole.desktop"));
ngrahamUnsubmitted
Done

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

ngraham: "Try installing the 'konsole' package with your package manager"
rkflxUnsubmitted
Done

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…
ngrahamUnsubmitted
Done

+1.

ngraham: +1.
elvisangelaccioUnsubmitted
Done

Please add this as parent.

elvisangelaccio: Please add `this` as parent.
151​ });
152​ m_konsolePartMissingMessage->addAction(installKonsoleAction);
153​ m_layout->addWidget(m_konsolePartMissingMessage);
154​ m_layout->addStretch();
elvisangelaccioUnsubmitted
Done

Please add this as parent.

elvisangelaccio: Please add `this` as parent.
155​ QTimer::singleShot(0, m_konsolePartMissingMessage, &KMessageWidget::animatedShow);
156​ } else {
ngrahamUnsubmitted
Done

"Polling"

ngraham: "Polling"
157​ m_konsolePartMissingMessage->animatedShow();
136 } 158​ }
137 } 159​ }
138 if (m_terminal) { 160​ if (m_terminal) {
139 m_terminal->showShellInDir(url().toLocalFile()); 161​ m_terminal->showShellInDir(url().toLocalFile());
140 changeDir(url()); 162​ changeDir(url());
141 m_terminalWidget->setFocus(); 163​ m_terminalWidget->setFocus();
142 connect(m_konsolePart, SIGNAL(currentDirectoryChanged(QString)), 164​ connect(m_konsolePart, SIGNAL(currentDirectoryChanged(QString)),
143 this, SLOT(slotKonsolePartCurrentDirectoryChanged(QString))); 165​ this, SLOT(slotKonsolePartCurrentDirectoryChanged(QString)));
▲ Show 20 Lines • Show All 82 Lines • Show Last 20 Lines