Differential D10862

Konsole: Add read-only mode
ClosedPublic
Actions

Authored by jnoack on Feb 26 2018, 2:05 PM.

Details

Reviewers

hindenburg

Group Reviewers

Konsole

Commits

R319:4f55526575ee: Konsole: Add read-only mode

Summary

This patch adds a read-only option for TerminalDisplays. When active,
all keyboard events are eaten. Mouse input is not affected and works like before.
The setting is not persisted and only lasts for the duration of the session.

Screenshots:

VDG input is highly appreciated. Also, I'm not sure if I can just change the rc-files without bumping the version?

FEATURE: 126930

Test Plan

Shortcuts still work
Paste and drop actions are disabled when readonly
Switching between read-only and normal tabs works as expected
Mouse input works like before

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.

jnoack created this revision.Feb 26 2018, 2:05 PM

Restricted Application added a subscriber: Konsole. · View Herald TranscriptFeb 26 2018, 2:05 PM

jnoack requested review of this revision.Feb 26 2018, 2:05 PM

Putting on my VDG hat: we definitely need a visible UI during read-only mode that tells you that the terminal is read-only, or else there is a near-100% chance that people will file bugs after they turn on read-only mode by accident or forget that it's on.

Konsole already shows a yellow-colored contextual bar that appears near the top of the window in various other contexts; maybe we could re-use that here?

In D10862#214408, @ngraham wrote:

Putting on my VDG hat: we definitely need a visible UI during read-only mode that tells you that the terminal is read-only, or else there is a near-100% chance that people will file bugs after they turn on read-only mode by accident or forget that it's on.

Konsole already shows a yellow-colored contextual bar that appears near the top of the window in various other contexts; maybe we could re-use that here?

Konsole uses a QLabel to show the bar at the top, which isn't resuable at all without refactoring (I refer to the bar which pops up when CTRL+S is pressed).

Instead I tried to use the KMessageWidget, which looks quite ok imo:

What do you think? Furthermore, once this is ready I would like to port the QLabel-"bar" to the KMessageWidget in another revision as well if nothing speaks against it. Then I can share most of the code.

I have code that changes that label to a kmessagewidget; the changes were so small that I haven't bothered committing.

And you'll have to allow the possibility of having both visible at the same time.

I'll look at the code later this week

In D10862#215452, @hindenburg wrote:

I have code that changes that label to a kmessagewidget; the changes were so small that I haven't bothered committing.

And you'll have to allow the possibility of having both visible at the same time.

I'll look at the code later this week

This is what I have right now:

I would like to create a separate revision for the changes regarding the message widget and make it depend on this one. It goes beyond the scope of the read-only mode. Will do it later tonight.

With the move to KMessageWidget, let's make sure we don't regress anything: in those screenshots the widget is overlapping the scrollbar and presumably the top few lines of text too. We'll need to fix those issues.

In D10862#215637, @ngraham wrote:

With the move to KMessageWidget, let's make sure we don't regress anything: in those screenshots the widget is overlapping the scrollbar and presumably the top few lines of text too. We'll need to fix those issues.

It's no regression, overlapping is a problem right now as well. Additionally, I imagine it a bit difficult to find a proper solution for the overlapped lines at the top given how it works right now. That's why the messages can be dismissed.
The scrollbar overlap should be easily fixed though.

I see, thanks!

jnoack added a dependent revision: D10935: Konsole: Use KMessageWidget in TerminalDisplay.Mar 1 2018, 1:39 AM

Anyone please comment on the below suggestions/ideas:

You have to increase the *rc version or the new files don't get installed
Drag and drop the ro tab undos the ro
The current patch has no warning, correct? I did see the recent patch to use kmessagewidget

4 . Changing the tab icon to the lock version would be nice IMHO
5 . Add the lock option when right clicking on tab

In D10862#216038, @hindenburg wrote:

You have to increase the *rc version or the new files don't get installed

Just bumping is enough, right. Noted.

Drag and drop the ro tab undos the ro

Hm... Drag and Drop is weird, even now Konsole is buggy when dragging a tab and CTRL+S is active. Will look into it.

The current patch has no warning, correct? I did see the recent patch to use kmessagewidget

Yes, because I touched the QLabel warning as well and changed the layout behaviour. Can't commit it separately without writing throwaway code.

4 . Changing the tab icon to the lock version would be nice IMHO

Noted.

5 . Add the lock option when right clicking on tab

Noted.

ngraham mentioned this in D10935: Konsole: Use KMessageWidget in TerminalDisplay.Mar 1 2018, 2:55 PM

We need to have any new strings committed by the 21st for the 18.04 release schedule - I would prefer a week earlier. If you're having issues w/ some of the issues, we could separate the patches to get the new strings in first.

https://community.kde.org/Schedules/Applications/18.04_Release_Schedule

Read-only flag is now part of the Session and thus moving / detaching a tab won't remove it
Change tab icon when read-only
Add action to tab context menu
Bumped rc version

I'll update D10935 to reflect the changes above.

Thanks, overall I don't see any major issues.

There are some issues below which I don't think should hold up committing this. Perhaps later you could work on them if you're interested.

dbus methods still work on locked tabs: example qdbus $KONSOLE_DBUS_SERVICE /Sessions/3 sendText hi
Certain menus should be disabled on locked tabs: (right click menu) Switch Profile, * Scrollback, Rename(?). Split View..... and more...

src/SessionController.cpp
461	In general we want to compare against nullptr
src/ViewContainer.cpp
307	Why remove this action when the menu closes?
634	nullptr
637	nullptr

In D10862#218315, @hindenburg wrote:

Thanks, overall I don't see any major issues.

There are some issues below which I don't think should hold up committing this. Perhaps later you could work on them if you're interested.

dbus methods still work on locked tabs: example qdbus $KONSOLE_DBUS_SERVICE /Sessions/3 sendText hi

Certain menus should be disabled on locked tabs: (right click menu) Switch Profile, * Scrollback, Rename(?). Split View..... and more...

Yeah, I can take a look at them next weekend I think.

I will update the diff regarding your comments. Should be good to go then.

src/ViewContainer.cpp
307	The action is added dynamically in openTabContextMenu from the respective sessionController for the underlying tab. If the action is not removed again, the context menu will keep all the actions from all different controllers.

Explicit nullptr check

Looks fine - konsolepart also works - when you correct issues, you can mark them as done.

hindenburg accepted this revision.Mar 5 2018, 2:26 PM

This revision is now accepted and ready to land.Mar 5 2018, 2:26 PM

Closed by commit R319:4f55526575ee: Konsole: Add read-only mode (authored by hindenburg). · Explain WhyMar 6 2018, 3:28 PM

This revision was automatically updated to reflect the committed changes.

It looks like I should have done a 'arc amend' with my updated message otherwise it is ignored.

Looks like Joshua's authorship information was not preserved either.

Revision Contents
Changeset List

			Path	Packages
M			desktop/partui.rc (3 lines)
M			desktop/sessionui.rc (5 lines)
M			src/Session.h (4 lines)
M			src/Session.cpp (19 lines)
M			src/SessionController.h (3 lines)
M			src/SessionController.cpp (33 lines)
M			src/TerminalDisplay.cpp (29 lines)
M			src/ViewContainer.cpp (21 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	28104			Feb 26 2018, 1:52 PM	★	★
Diff 2	28579			Mar 4 2018, 12:07 PM	★	★
Diff 3	28641			Mar 4 2018, 9:15 PM	★	★
Diff 4	28839	6605596	R319:4f55526575eef4dba71d6e1650370fac48abd2a3	Mar 6 2018, 3:28 PM	★	★