Differential D11762

Process key event even without active TerminalDisplay
ClosedPublic
Actions

Authored by mglb on Mar 28 2018, 1:10 AM.

Details

Reviewers
hindenburg
Group Reviewers
Konsole
Commits
R319:c0a46e2b08ce: Process key event even without active TerminalDisplay
R319:594368b534a8: Process key event even without active TerminalDisplay
Summary

There are (more or less) valid cases when currentTerminalDisplay can be
not set:

  • konsole started with --background-mode, sendText called via D-Bus
  • text send through KPart's sendInput()

It can be assumed read-only is not set in this case.

This patch should also fix TerminalInterfaceTest. Currently the shell
probably doesn't get commands send through sendInput.

Test Plan
  • Run konsole --background-mode --nofork & ; sleep 2; qdbus org.kde.konsole-$! /Sessions/1 sendText $'echo test\n'
  • Press Ctrl+Shift+F12
  • The terminal should show echo test command and its result

Diff Detail

Repository
R319 Konsole
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mglb created this revision.Mar 28 2018, 1:10 AM
Restricted Application added a project: Konsole. · View Herald TranscriptMar 28 2018, 1:10 AM
Restricted Application added a subscriber: Konsole. · View Herald Transcript
mglb requested review of this revision.Mar 28 2018, 1:10 AM
mglb planned changes to this revision.
mglb requested review of this revision.Mar 28 2018, 1:13 AM
mglb edited the summary of this revision. (Show Details)
mglb added a reviewer: Konsole.
hindenburg added a subscriber: hindenburg.Mar 28 2018, 1:18 AM

Thanks , now that I look at the previous code; can't we just return if (isReadOnly) right after 1051 line?

mglb added a comment.EditedMar 28 2018, 1:26 AM

No, because screen scrolling is always allowed (starts on 1114). The rest is filtered out on 1138 and 1142 when read-only is active.

hindenburg added a comment.Mar 28 2018, 1:31 AM

OK this looks better that what I had thanks

hindenburg added a comment.Mar 28 2018, 1:38 AM

Does this fix the man scrolling crash? I couldn't produce it

mglb added a comment.Mar 28 2018, 1:42 AM

Your patch fixed it (well.. disabled), with this patch it is enabled again, without crashing. I am uploading patch which fixes scroll in read only mode, and sets currentTerminalDisplay.

hindenburg accepted this revision.Mar 28 2018, 1:47 AM
This revision is now accepted and ready to land.Mar 28 2018, 1:47 AM
hindenburg added a comment.Mar 28 2018, 1:55 AM

So if currentView is nullptr, shouldn't that be handle below as well? That's why I returned early.

mglb added a comment.Mar 28 2018, 2:15 AM

currentView (exactly the same declaration) was there since 2011, declared just below 1114 ( if ( entry.command() != KeyboardTranslator::NoCommand )) , so I think it is safe to assume in this scope it is not null.

Closed by commit R319:c0a46e2b08ce: Process key event even without active TerminalDisplay (authored by mglb, committed by hindenburg). · Explain WhyMar 28 2018, 2:25 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/Vt102Emulation.cpp (5 lines)

Diff 30775

View Options

src/Vt102Emulation.cpp

Loading...