Differential D17747

Only show tab icon if the user has specifically chosen one
ClosedPublic
Actions

Authored by ngraham on Dec 23 2018, 6:26 AM.

Details

Reviewers
ndavis
hindenburg
Group Reviewers
Konsole
VDG
Commits
R319:e1fde67a69c7: Only show tab icon if the user has specifically chosen one
Summary

This patch introduces a UI change: tabs only show an icon if it's not
the default one; that is to say, only if the user has actually customized
the icon. There are several reasons for this:

  1. Conceptual: When only non-default profile icons show up in the tab, it does a better job of highlighting the fact that a non-default profile is being used.
  2. Aesthetic: you no longer have the same icon in every tab by default, which looks bad and causes the eye to ignore it over time.
  3. Icon-specific: the utilities-terminal icon doesn't look good against a dark background (https://bugs.kde.org/show_bug.cgi?id=367696)
  4. Code hygiene: reduces a bit of redundancy in the session controller code.

BUG: 401864
CCBUG: 367696
FIXED-IN: 19.04.0

Test Plan

Open multiple tabs and make one of them use a profile with a
non-default icon:

Verify that bell signals still cause tabs to get icons.

Diff Detail

Repository
R319 Konsole
Branch
only-show-customized-tab-icons (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6277
Build 6295: arc lint + arc unit
ngraham created this revision.Dec 23 2018, 6:26 AM
Restricted Application added a project: Konsole. · View Herald TranscriptDec 23 2018, 6:26 AM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
ngraham requested review of this revision.Dec 23 2018, 6:26 AM
ngraham edited the test plan for this revision. (Show Details)Dec 23 2018, 6:27 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)
hindenburg edited the summary of this revision. (Show Details)Dec 23 2018, 6:35 PM
hindenburg edited the test plan for this revision. (Show Details)
hindenburg added a subscriber: hindenburg.Dec 23 2018, 6:45 PM

I don't see any issues - tested w/ monitor for silence/activity works as before. The tabs look odd to me w/o any icon but that's likely just due I'm used to seeing them.

src/SessionController.cpp
1531

For empty strings use QString()

abetts added a subscriber: abetts.Dec 23 2018, 6:55 PM

I have seen implementations like this one before and it makes it so that the tab expands and shrinks when the focus is on the tab because it needs to accommodate for the icon that appears and disappears. Is that the case with this patch?

ngraham updated this revision to Diff 48093.Dec 23 2018, 8:45 PM
ngraham marked an inline comment as done.

Use QString() for an empty string

ngraham added a comment.Dec 23 2018, 8:46 PM
In D17747#381462, @abetts wrote:

I have seen implementations like this one before and it makes it so that the tab expands and shrinks when the focus is on the tab because it needs to accommodate for the icon that appears and disappears. Is that the case with this patch?

Yes, when the tab gains an icon, it will become wider to accommodate it. It already has this behavior when it's displaying the name of the running command and that command finishes and is replaced with another, so it's not unprecedented.

hindenburg added a comment.Dec 27 2018, 1:42 AM

Anyone else have any comments on the VD aspect?

ngraham edited the summary of this revision. (Show Details)Dec 27 2018, 3:17 AM
ngraham edited the summary of this revision. (Show Details)
ndavis accepted this revision.EditedDec 27 2018, 3:51 AM

While I would like it if the tab could remain a consistent size, regardless of whether or not there is an icon, it doesn't need to be fixed in this revision. What we have here now is a better default because it saves more space.

For anyone that mainly uses only one profile, the icons become useless. For anyone who likes to use multiple profiles, they can easily add icons that have more meaning than the Konsole icon.

This revision is now accepted and ready to land.Dec 27 2018, 3:51 AM
fabianr added a subscriber: fabianr.Dec 27 2018, 7:49 AM
In D17747#382609, @ndavis wrote:

While I would like it if the tab could remain a consistent size, regardless of whether or not there is an icon, it doesn't need to be fixed in this revision.

+1 . That tabs should stay consistent in size. Because of any size changes not directly triggered by the user you prevent any kind of interaction by spatial memory.

ngraham added a comment.Dec 27 2018, 8:17 PM
In D17747#382623, @fabianr wrote:

+1 . That tabs should stay consistent in size. Because of any size changes not directly triggered by the user you prevent any kind of interaction by spatial memory.

With this patch, the only way to make a tab gain an icon (and therefore become wider) is to switch it to use a profile that has explicitly been assigned a custom icon. So any width changes are in fact explicitly triggered by the user.

hindenburg accepted this revision.Dec 28 2018, 3:25 AM
hindenburg edited the summary of this revision. (Show Details)
Closed by commit R319:e1fde67a69c7: Only show tab icon if the user has specifically chosen one (authored by ngraham, committed by hindenburg). · Explain WhyDec 28 2018, 3:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/SessionController.cpp (16 lines)

Diff 48043

View Options

src/SessionController.cpp

Loading...