Persist after closing last tab when running in background mode
AbandonedPublic

Authored by EspadaV8 on Nov 12 2018, 12:21 PM.

Details

Reviewers
konsole-devel
hindenburg
ngraham
Group Reviewers
Konsole
Summary

Let Konsole continue running after closing the final tab. This allows
Konsole to always be available via a global shortcut.

Test Plan
  • Start konsole --background-mode
  • Check that existing functionality isn't broken (e.g. open a new tab; run a command that out puts something)
  • Hide Konsole and make sure that un-hiding it keeps existing sessions
  • Close all tabs using e.g. Ctrl+d
  • Once last tab has been closed, Konsole should be hidden
  • Attempt to open Konsole again with global shortcut, a new session should've been started without the program quitting

Diff Detail

Repository
R319 Konsole
Branch
persist-window-after-closing-last-tab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4866
Build 4884: arc lint + arc unit
EspadaV8 created this revision.Nov 12 2018, 12:21 PM
Restricted Application added a project: Konsole. · View Herald TranscriptNov 12 2018, 12:21 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
EspadaV8 requested review of this revision.Nov 12 2018, 12:21 PM

Sorry, I tried to update D16482 using arc but it's created a new revision instead. I wanted to update the previous one so that it used the --background-mode flag instead of adding a new option. The update should now mean that if Konsole is started with --background-mode it will behave like Yakuake does so that if the last tab is closed, the window is hidden and a new default session is started for when the window it shown again.

As I mentioned in D16482, I was originally using Yakuake, but rather than updating that it seemed better to update Konsole with a (long term) goal of deprecating Yakuake. I'm not aware of many features that Yakuake offers over Konsole while it does lack some and has bugs related to others (like the Start in same directory as current tab mentioned in D16482).

Regarding this revision, I'm not super happy with how the MainWindow needs to have the new public signal and private slot to emit lastTabClosed that the Application needs to listen to. I couldn't think of another nice way to handle this since Application is the only place that knows if the programme was started with --background-mode. Passing that down into MainWindow seemed wrong and having Application handle the closing of windows seems wrong too (although less wrong). Also, is there any way to emit lastTabClosed(this) without needing to wrap it in the allTabsClosed method? I was hoping that the connect(...) line could just pass in a pointer but I couldn't find a way to do that.

Other than that I think the code is pretty straight forward. We can easily check in Application if _backgroundInstance is set and if it isn't we know to tell window to close(), otherwise we want to create a new default session and call _backgroundInstance->hide().

EspadaV8 edited the test plan for this revision. (Show Details)Nov 20 2018, 10:10 AM
EspadaV8 added a subscriber: hein.Nov 22 2018, 10:36 AM

@ngraham Is there anyway to progress this and to decide if this change (or something like it) could be merged into Konsole? I spoke briefly with @hein on the #kde-devel channel on IRC and they didn't see any reason why Yakuake features couldn't be merged into Konsole itself (just that no one has done it before). They did mention that they'd like to see the Konsole KPart functionality improved but I'm not in a position to be able to know exactly what would need to be done to implement those changes.

If this revision isn't okay then I'd be happy to make any suggestions that you have to help get these features directly into Konsole.

@ngraham Is there anyway to progress this and to decide if this change (or something like it) could be merged into Konsole? I spoke briefly with @hein on the #kde-devel channel on IRC and they didn't see any reason why Yakuake features couldn't be merged into Konsole itself (just that no one has done it before). They did mention that they'd like to see the Konsole KPart functionality improved but I'm not in a position to be able to know exactly what would need to be done to implement those changes.

If this revision isn't okay then I'd be happy to make any suggestions that you have to help get these features directly into Konsole.

Thanks for the work, let me look at it in more detail.

This allows Konsole to always be available via a global shortcut.

That's already do-able.
System settings -> Global Shortcuts -> "+" -> select konsole

What problem are you trying to solve?

@davidedmundson this isn't what this is trying to provide. I have a global shortcut configured for "Toggle Background Window" (in my case Meta+Return). I have Konsole start at launch (konsole --background-mode). This prevents Konsole from quitting after the last tab has been closed.

Currently if you launch Konsole like that, bring the window to the foreground and exit the session (e.g. Ctrl+d), pressing the shortcut won't do anything because Konsole quit. With this patch it will instead hide the window, open up a new session so that the next time you press the global shortcut Konsole will show again.

hindenburg edited the test plan for this revision. (Show Details)Dec 13 2018, 1:52 AM

I'm still looking at this.... still pondering if this feature should be added and if so, need to look at the code.

Any update on having this reviewed? If it's unlikely to make it into Konsole would it be possible to still get a review of the code since I'm curious if it could be improved in any way. I was thinking too that this could be added as either checkbox option or as an extra command line parameter if having it as the default behaviour wasn't wanted.

Any update on having this reviewed? If it's unlikely to make it into Konsole would it be possible to still get a review of the code since I'm curious if it could be improved in any way. I was thinking too that this could be added as either checkbox option or as an extra command line parameter if having it as the default behaviour wasn't wanted.

I'm a bit behind w/ reviewing patches ATM. If this only works when using --background-mode then perhaps another cmd line option. I don't see anything obviously wrong w/ the code. I'm not absolutely against this if you want to work on it more. The current behavior can't change.

EspadaV8 abandoned this revision.Dec 12 2019, 12:01 AM

Although I would still very much like Konsole to support this, with the changes made to how Konsole does splitting, this patch no long applies cleanly and after spending a little bit of time on it I was unable to get the code to work correctly without segfaulting, so I'm closing this for now.