Allow view-controlling keys in read-only mode
ClosedPublic

Authored by mglb on Mar 25 2018, 12:21 AM.

Details

Summary

Allow to use scrolling (Shift+Up/Down/PgUp/PgDown/Home/End) and a key
for showing URL hints when the view is in read-only mode.

Test Plan
  • Prepare
    • Turn on scrollback
    • Set at least one modifier key for "Show URL hints" (edit profile → advanced)
    • Generate a few screens of text in Konsole
    • Display some URL (e.g. echo 'www.kde.org')
    • Display current time every second: while sleep 1; do printf '\r%s' "$(date)"; done
    • Turn on read-only mode
  • Patch tests
    • Hold down URL hint key - the URL should be underlined with square and a digit on the left
    • Press <URL-hint-key>+1 - the URL should open in a web browser
    • Scroll the view with Shift+Up/Down/PgUp/PgDown/Home/End - the view should scroll
    • Split the view, do previous test in each split - only currently active split should scroll
  • Regressions tests
    • Press different keys (including Ctrl+C, Ctrl+Z, etc; skip those mapped to Konsole actions) - nothing in terminal should happen
    • Press Ctrl+S - the time should not stop updating
    • Use IME to enter text - nothing in terminal should be shown

Diff Detail

Repository
R319 Konsole
Branch
arc/allow-view-controlling-keys-in-read-only-mode
Lint
No Linters Available
Unit
No Unit Test Coverage
mglb created this revision.Mar 25 2018, 12:21 AM
Restricted Application added a project: Konsole. · View Herald TranscriptMar 25 2018, 12:21 AM
Restricted Application added a subscriber: Konsole. · View Herald Transcript
mglb requested review of this revision.Mar 25 2018, 12:21 AM
mglb updated this revision to Diff 30460.Mar 25 2018, 12:47 AM

Check for valid session pointer before checking it for read-only state.

The case with null session happened when using iBus + Anthy, typing Japanese and accepting the text.

hindenburg accepted this revision.Mar 25 2018, 1:38 PM
hindenburg added a subscriber: hindenburg.

LGTM thanks

This revision is now accepted and ready to land.Mar 25 2018, 1:38 PM
This revision was automatically updated to reflect the committed changes.
mglb added a comment.Mar 25 2018, 3:46 PM

You should also revert TerminalDisplay.cpp part - with partial revert you can input text in read-only mode.

BTW. Can this be reopened for further investigation (possibly after release), or new review request will be better?

hindenburg reopened this revision.Mar 25 2018, 3:49 PM
This revision is now accepted and ready to land.Mar 25 2018, 3:49 PM

yea I should have just revert the entire commit - my mistake

you can patch against master - I just won't backport to 18.04 until we're sure there are no issues.

mglb updated this revision to Diff 30608.Mar 26 2018, 8:50 AM
mglb edited the test plan for this revision. (Show Details)

Fix null pointer dereference; minor fixes

mglb updated this revision to Diff 30609.Mar 26 2018, 8:55 AM

git rebase master

mglb requested review of this revision.Mar 26 2018, 10:16 PM

thanks - passes the ASAN on my box - I'll double-check and commit to master - if no issues, I'll commit to 18.04 before the release.

hindenburg accepted this revision.Mar 27 2018, 1:51 PM
This revision is now accepted and ready to land.Mar 27 2018, 1:51 PM
This revision was automatically updated to reflect the committed changes.

Same test still fails on isReadOnly = currentView->sessionController()->isReadOnly();

note that is only seen when using ASAN - I'll look at it later

https://build.kde.org/job/Applications%20konsole%20kf5-qt5%20FreeBSDQt5.9/166/testReport/junit/(root)/TestSuite/TerminalInterfaceTest/
https://build.kde.org/job/Applications%20konsole%20kf5-qt5%20SUSEQt5.9/173/testReport/junit/(root)/TestSuite/TerminalInterfaceTest/

I won't commit to 18.04 until this is resolved.

I committed two changes so the test doesn't crash - it still fails but thats another issue

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

I've made one more change, in separate review as this one is already on master: https://phabricator.kde.org/D11762