Mind you guys that the internal implementation without a profile is kinda
broken, so this patch makes sense to me.

None, I’m ok with it

In D20466#448806, @mglb wrote:

After thinking about scrollback a bit more I think history file path AND scrollback size should go to global settings. Does anyone use different profiles with different history sizes, or change size just for current session?

In D20466#448705, @ngraham wrote:

In D20466#448704, @hindenburg wrote:

Another way to display this would be to only show the "info box" when unlimited is selected.

+1

Did you mean -1? :P Hiding part of an UI depending on selected option is bad UX. Moving another elements around because something appears is probably even worse.
It is also against KDE HIG: https://hig.kde.org/patterns/content/settings.html#implementation (last point)

About changes:

• Maybe putting custom path field below radio button (and indenting it) would be better? Path which appear here will probably be longer than current field width (at least I think so), and it will not be needed to align it with fixed size field
• All radio buttons had equal vertical spacing, and the left label was aligned to the first radio button
  
• Button group instead of frame
• Maybe kmessagebox instead of label?

Ok, I'll update the code for that.

In D20466#448704, @hindenburg wrote:

A quick test of picking the other 2 save locations ,it doesn't seem to work. The location settings are still stored in konsolerc. Aren't they suppose to be in the .profile file now?

Another way to display this would be to only show the "info box" when unlimited is selected.

In D20466#448705, @ngraham wrote:

In D20466#448704, @hindenburg wrote:

Another way to display this would be to only show the "info box" when unlimited is selected.

+1

• Remove useless lambda

• Add missing label, Add tooltips

Only because I’m forbidden to remove configuration options that exist, for
me this is one of those niche cases that shouldn’t exist.

I tougth about showing the paths but my first tougth was “why do the user
cares where this is being stored”?

This is more future proof as if we change the type of the function we don’t
need to change that return.

We set this variable in two or three different files and in a few different
methods. I remember that one of the first things that I tried to do for
konsole was to simplify the creation of sessions (and this broke a lot of
code and you where not happy) but it seems that we do need to make some
compromise and align that.

I beg your pardon? (How do I test the new session? konsole is working
for me so maybe it's something I missed)

I forgot this for quite a while, I'll update today.

In D20115#447125, @hindenburg wrote:

In D20115#447119, @hindenburg wrote:

The one issue I have, if you open just one tab and press the move left/right I get C for 'move right' and D for 'move left" echoed on the terminal. They go away when I open another terminal and don't appear to return even if I close all but one.

ON master w/o this patch, I still get the CD echoing on all the tabs when I try the shortcuts. So maybe it is something else. Is anyone else seeing this issue?

I'll add a backtrace as soon as I test this again. since I'm succeeding in my konsole code cleanup I'll get this back on track.

too many things to fix here and there. :)

we don't have a clangformat file specified for kde projects, maybe this is
something good to add. I voted for global .editoconfigfiles but I was
outvoted. :)

• Further simplify code

When we have multiple tabs / splits this is not confusing?

this should fix most of the issues. I have another patch to go related to actions, but this one needs to land first.

• Fix action identifiers

@ngraham can you accept even with the shortcut issue that you are having? I have other reviews that depend on this one. I promise you to fix the shortcut bug <3

• Use reference with range for loops

ping?

ping?

the adition of the phabricator configuration file is unrelated to a clean, please split in two patches.

the specific warning is https://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html

I believe that he builds in clang and clang 8 added quite a bunch of new
warnings

true, but then the instance is there anyway. what we gain from this?

Hm? This doesn’t touches that.

waiting for @hindenburg ok before landing. :)

• Better variable name

• Whitespace / Todos removal

In D20224#445957, @ngraham wrote:

This seems to fix the issue I started having recently where Konsole crashes at logout and then opens as a big white window on login after session restoration.

sorry, my mistake. this is correct.
https://en.cppreference.com/w/cpp/language/initializer_list#Delegating_constructor

that looks factually wrong. what are you trying to accomplish here?

Session Save / Restore

OpenMP is missing, Andre is working on it.

This code can load sessions, it has a better behavior than the old session load code, but still has bugs. I'll finish this luckly today / tomorrow.

• Sessions partially working
• Code cleanup

I'll remove the debugs / message handlers as soon as everything is working. but it's one of the fastest ways for me to keep the debug messages between runs so I can understand what's wrong.
the session is correctly saved, and the SessionManager correctly loads all the sessions back, the ViewManager however is broken on load. that's what I'm debugging now. so even tougth we don't display any terminal, they are up and running.

• messageHandler + tons of debug
• Sabe json correctly in the configuration

Well, I finally managed to discover something:
when MainWindow::saveProperties is triggered we already destroyed the sessions and tabs, and that's the reason it's not saving anything.
I'm trying to debug the destructor order now to see if I can change something.

• Implement the Load of Sessions
• Solve crash while quitting konsole via Session End
• Still Broken, do not review unless you can help. :)

The code for sessions is commented out, in the original review you said
that you would help me see the saving the session code as now we have to
deal with splits.

• Remove empty line

This is mostly working, but I will not merge this untill we fix the current state of the shortcuts (that I'm preparing in a separate patch).
after the shortcuts are correct, there will be a follow up patch here to use the switcher style *or* the next-prev-tab style (as we have today) selectable by the user, defaulting to the current behavior.

• Simplify handling of code
• Handle list with the shortcut selected
• Mostly working
• Remove mnemonics from text

I’m in a konsole spree this week so expect patches.

how that the splits are merged, I'm preparing a round of patches that where rotting in my pc.

• Use one liners where appropriate.
• Remove sender() and QVariant() conversions
• Declare + initialize, instead of setting the value later
• Don't create unecessary temporaries
• Extract code from huge function into a smaller one