Konsole: Use KMessageWidget in TerminalDisplay
ClosedPublic

Authored by jnoack on Mar 1 2018, 1:39 AM.

Details

Summary

Depends on D10862

Before:

After:

Read-only:

Both:

This also fixes the overlapped scrollbar (see first screenshot).

Test Plan

Tested before and after, behaviour is the same.

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 requested review of this revision.Mar 1 2018, 1:39 AM
jnoack created this revision.
ngraham added a subscriber: ngraham.Mar 1 2018, 2:55 PM

Might it make more sense to have D10862 depend on this, rather than the other way around?

jnoack updated this revision to Diff 28590.Mar 4 2018, 1:14 PM

@ngraham No, I don't think so. This one here doesn't work without the other one.

Updated diff to reflect latest changes in D10862.

jnoack updated this revision to Diff 28633.Mar 4 2018, 8:48 PM

Removed unrelated code

Can you redo this patch so it apples cleanly to master?

Joshua, can you provide your email address so we can associate your authorship information with this patch?

In the future, if you upload patches using arc, this will happen automatically. See our documentation: https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches

jnoack updated this revision to Diff 28862.Mar 6 2018, 7:29 PM

Rebase on master

jnoack added a comment.Mar 6 2018, 7:31 PM

Can you redo this patch so it apples cleanly to master?

Done.

Joshua, can you provide your email address so we can associate your authorship information with this patch?

In the future, if you upload patches using arc, this will happen automatically. See our documentation: https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches

Yes, I will definitely try out Arcanist the next time.

joshua(dot)noack(at)gmail(dot)com

Thanks!

FWIW, even after that rebase, I needed to invoke arc with --skip-dependencies to get this patch to apply cleanly, or else arc erroneously tried to also patch in D10862, which is weird, since that already landed. Maybe not landing it with arc made it go mental or something?

Also, +1, works great! I love it. KMessageWidgets are quite nice, and using one here suits Konsole very well.

Thanks for the --skip-dependencies help

I'm not positive adding funcational std::function is worth it for these 2 instances but it is used in KF5 quite a bit.

If the "output has been suspended" is active and that tab is dnd to create another window, the kmessage is not shown and it seems to take an extra Ctrl+q to get the tab to resume.

jnoack added a comment.Mar 7 2018, 2:24 PM

If the "output has been suspended" is active and that tab is dnd to create another window, the kmessage is not shown and it seems to take an extra Ctrl+q to get the tab to resume.

Right, since the "suspended mode" is not restored when a new TerminalDisplay is created (i.e. detach). It's not a new bug, so a fix would be unrelated for this revision. But I can fix it separately when I find the time.

hindenburg accepted this revision.Mar 7 2018, 2:38 PM

That's fine I don't want to hold this up - I'll try to commit this today

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

@hindenburg if you need any help with using arc land, let me know and I'd be happy to render assistance. But it should be as simple as:

arc patch D10935
git commit --amend --author "Joshua Noack <joshua.noack@gmail.com>"
arc land

My only issue is how to change the commit message - doing 'git commit --amend; arc land' on the previous "read only" patch didn't use the new message.

herrold added a subscriber: herrold.Mar 7 2018, 3:05 PM

Hmm, git commit --amend should totally work for modifying the commit message. I know it does work for modifying authorship information (I've used that myself many times).

This revision was automatically updated to reflect the committed changes.

I'm missing something - arc land shows my changed message but uses the old message

kurthindenburg:src/ (arcpatch-D10935) $ arc land
Landing current branch 'arcpatch-D10935'.
TARGET Landing onto "master", the default target under git.
REMOTE Using remote "origin", the default remote under git.
FETCH Fetching origin/master...
This commit will be landed:

  • 45f12a7 Use KMessageWidget to replace suspended QLabel and for locked sesssion.

    This branch has revision 'D10935: Konsole: Use KMessageWidget in TerminalDisplay' but you are not the author. Land this revision by jnoack? [y/N] y

Landing revision 'D10935: Konsole: Use KMessageWidget in TerminalDisplay'...
PUSHING Pushing changes to "origin/master".

herrold removed a subscriber: herrold.Mar 7 2018, 3:38 PM
rkflx added a subscriber: rkflx.EditedMar 7 2018, 3:43 PM

See here.

arc land does arc amend before landing (to get the "reviewed/accepted by" line from Phab to the local checkout), so any local changes you did to title/summary/test plan/subscribers etc. will be discarded (does not apply to author/committer/patch). Either change the title on Phab, or issue this:

arc amend
git commit --amend
arc diff --edit --verbatim
arc land

Alternatively, if you know what you are doing:

arc amend
git commit --amend
git push

(Untested, but let me know if it fails for you.)

ach added a subscriber: ach.Mar 11 2018, 4:55 PM

Other apps (dolphin, kmail) use the 'cross on red bg' button in the upper right corner to close the msg.

I think that is much more intuitive and consistent (and shorter) than having a 'close' URL in the ever changing text formulations like 'Please click _here_' or '_Dismiss_' or ...

If you want I can submit a proper bug for this.

In D10935#223341, @ach wrote:

Other apps (dolphin, kmail) use the 'cross on red bg' button in the upper right corner to close the msg.

I think that is much more intuitive and consistent (and shorter) than having a 'close' URL in the ever changing text formulations like 'Please click _here_' or '_Dismiss_' or ...

If you want I can submit a proper bug for this.

I initially tried that, but when the close button has been pressed there's no way to bring back the message widget without creating a new one unless I missed something?
Go readonly -> Dismiss via close button -> Undo readonly -> Make readonly -> no message widget visible since it has been closed and not hidden.

ach added a comment.Mar 11 2018, 5:10 PM

In case it's not hidden, but destroyed, create a new KMessageWidget?
Or maybe the 'close' button can be remapped to trigger a hide instead of destroy?

In D10935#223346, @ach wrote:

In case it's not hidden, but destroyed, create a new KMessageWidget?
Or maybe the 'close' button can be remapped to trigger a hide instead of destroy?

Good question. I'll look into it.

@ach Thanks for bringing this up. Found a way better approach. Will create a new revision for it.