Rework scrollback settings
Needs ReviewPublic

Authored by tcanabrava on Apr 11 2019, 12:50 PM.

Details

Reviewers
hindenburg
Group Reviewers
Konsole
Summary

New configurable scrollback, when 'unlimited' is not selected, nothing changes.

When unlimited is selected, we display the option to choose the location to store it. (I would like to remove that option in the future to be honest.)

Old screen, completely away from the scrollbar option.

Settings scrollbar is split in two different panels,
half of it in "Profile", half of it in "Settings".
the majority of the settings are in profile, while in
settings we can only configure where the profile will
save the temporary file.

While one is local and another is global, I belive that
this is a bad UX: one should not need to jump thru different
dialogs to configure related preferences.

Now everything is in Profile configuration. that lead to
a code reduction of more than half, and also added maintenability.

Next: per-profile settings for the temporary file.

Diff Detail

Repository
R319 Konsole
Branch
clean_history_save_path
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10756
Build 10774: arc lint + arc unit
tcanabrava created this revision.Apr 11 2019, 12:50 PM
Restricted Application added a project: Konsole. · View Herald TranscriptApr 11 2019, 12:50 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Apr 11 2019, 12:50 PM
tcanabrava edited the summary of this revision. (Show Details)Apr 11 2019, 12:55 PM
tcanabrava edited the summary of this revision. (Show Details)
tcanabrava added reviewers: Konsole, hindenburg.

+1 for moving the setting closer to where it's actually used. That's always a good idea. I would bring back the actual paths though. In fact, maybe we should only show the actual paths:

(o) /tmp/
( ) /home/tcanabrava/.cache/konsole/
( ) Custom    [___________]

Also, if changing this setting still requires restarting Konsole to make it take effect, I would recommend showing a KMessageWidget with that information and a "Restart Konsole" button anytime the setting is changed.

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

Em qui, 11 de abr de 2019 às 19:02, Nathaniel Graham <
noreply@phabricator.kde.org> escreveu:

ngraham added a comment. View Revision
https://phabricator.kde.org/D20466

+1 for moving the setting closer to where it's actually used. That's
always a good idea. I would bring back the actual paths though. In fact,
maybe we should *only* show the actual paths:

(o) /tmp/
( ) /home/tcanabrava/.cache/konsole/
( ) Custom [___________]

Also, if changing this setting still requires restarting Konsole to make
it take effect, I would recommend showing a KMessageWidget with that
information and a "Restart Konsole" button anytime the setting is changed.

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D20466

*To: *tcanabrava, Konsole, hindenburg
*Cc: *ngraham, konsole-devel, gennad, thsurrel, maximilianocuria,
hindenburg

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

That's an argument for not having this UI/feature at all. :) As long as it exists, I think we should assume that it's for people who do care about the exact path.

mglb added a subscriber: mglb.Apr 11 2019, 8:17 PM

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

If they doesn't care, why give them ability to change the location?
User might not know exactly what "system" and "user" location is (/tmp might be obvious, ~/.cache/konsole not so much), and on which partition it will eventually end (in case of weird partitioning).

I did it this way:

As for UI itself - I would do it like this:

( ) Unlimited
    Scrollback location:
    ( ) System temporary directory (...)
    ( ) User cache directory (...)
    ( ) Custom: [____________] [_]
( ) None

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

Em qui, 11 de abr de 2019 às 22:17, Mariusz Glebocki <
noreply@phabricator.kde.org> escreveu:

mglb added a comment. View Revision https://phabricator.kde.org/D20466

In D20466#448239 https://phabricator.kde.org/D20466#448239, @tcanabrava
https://phabricator.kde.org/p/tcanabrava/ wrote:

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

If they doesn't care, why give them ability to change the location?
User might not know exactly what "system" and "user" location is (/tmp
might be obvious, ~/.cache/konsole not so much), and on which partition it
will eventually end (in case of weird partitioning).

I did it this way:
F6767147: Screenshot_2019-04-11-21-52-12.png
https://phabricator.kde.org/F6767147

As for UI itself - I would do it like this:

( ) Unlimited

Scrollback location:
( ) System temporary directory (...)
( ) User cache directory (...)
( ) Custom: [____________] [_]

( ) None

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D20466

*To: *tcanabrava, Konsole, hindenburg
*Cc: *mglb, gennad, ngraham, konsole-devel, thsurrel, maximilianocuria,
hindenburg

In D20466#448352, @mglb wrote:

As for UI itself - I would do it like this:

( ) Unlimited
    Scrollback location:
    ( ) System temporary directory (...)
    ( ) User cache directory (...)
    ( ) Custom: [____________] [_]
( ) None

That looks fine to me.

Let's give the feature a good UI, then we can later discuss whether or not it should exist. :)

tcanabrava updated this revision to Diff 56036.Apr 12 2019, 7:53 AM
  • Add missing label, Add tooltips
tcanabrava updated this revision to Diff 56037.Apr 12 2019, 7:57 AM
  • Remove useless lambda

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.

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

+1

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

+1

This is already happening dudes.

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.

no, they are still global to konsole, I did not change where konsole saves the configuration, just where konsole edits them. I did not want to remove a setting that existed. I can however remove the code for that and make everything go thrugo thru the profile.

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

+1

This is already happening dudes.

Ok now I see if you switch options - but on first opening of dialog, the info box is there regardless

The 3 options should be a radio - you have to pick one - now you can de-select all 3

If you're going to put the options in the profile dialog, they have/should to be in the profile file - then each profile can have different scrollback settings.

Ok, I'll update the code for that.

mglb added a comment.Apr 12 2019, 4:09 PM

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?

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?
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?

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?

Can you try to mock up a dialog where I don't hide the information?

mglb added a comment.Apr 12 2019, 8:15 PM

Sure:

And maybe rename labels in this style (someone with better english skills than I should verify this):

  • Fixed size → In memory, size:
  • Unlimited → On filesystem: (or "Unlimited, on filesystem:"?) + remove "history file location" label
  • Custom → Other

Then something like this would be possible


(here just disable combo when unlimited is not selected)

In D20466#448940, @mglb wrote:


(here just disable combo when unlimited is not selected)

Ooh, I like this one!

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

I don't recall anyone asking for it although I'm rather surprised no one has. You can still change a session temporarily. If these go in the profile dialog, they should be profile specific. I'm not convinced either way really other than having scrollback settings in two dialogs is likely not great.

What's the status of this?

Forgotten, I’ll look at it today