Per VDG, allow closable tabs to have close buttons;
this adds this option while leaving the previous options.
Details
Details
- Reviewers
tcanabrava ngraham hindenburg - Group Reviewers
VDG Konsole - Commits
- R319:c86d468e3620: Add option to have close button on each tab
Diff Detail
Diff Detail
- Repository
- R319 Konsole
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 6509 Build 6527: arc lint + arc unit
Comment Actions
+1 on the concept; having the close button off to the side always struck me as really weird and nonstandard.
-1 on the patch formatting. :) Can you follow @hindenburg's advice?
Comment Actions
Actually, now that I apply this, I don't see any new files being created, so sorry about the comment.
Functionally, it works fine! UI-wise, I think we need to change a few things. The current UI is a bit off:
I feel fairly strongly that this should not be an option and when close buttons are shown, they should always be on the tab rather than on the bar. But if this is just too controversial, the following UI would be an improvement:
New Tab button: [ ] Show
Close buttons: (o) On each tab
( ) On the tab bar
( ) NoneIf it's absolutely imperative that a single checkbox control both the new tab and close button visibility, then we could do the following:
[] Show 'New Tab' and 'Close Tab' Buttons
Close buttons: (o) On each tab
( ) On the tab bar...And the radio buttons would only be enabled when the checkbox above them was checked.
But again, I think it would be best to always show the close buttons on the individual tabs when close buttons are visible.
Comment Actions
The labels and checkboxes are not starting with a verb. I think that can be reworked.
Comment Actions
- Added option to show 'close tab' button on each tab in the tab bar, and does not display button. Added option for 'New tab'.
| src/ViewContainer.cpp | ||
|---|---|---|
| 73 | To save you time looking for it the icon set here. | |
Comment Actions
For now the default can remain none, but I recommend we change the default to show a close button--if not in this patch, then in another one.
We are currently working on harmonizing the appearance and behavior of tabs throughout KDE software (T10233) and closable tabs needs close buttons. It's one of the ways that the tab communicates to the user that it's closable. Other KDE apps with closable tabs (Dolphin, Kate, KDevelop, Okteta, Okular) all have close buttons shown on their tabs. Konsole should too.
Comment Actions
Works for me:
Checking patch src/settings/konsole.kcfg... Checking patch src/settings/TabBarSettings.ui... Checking patch src/ViewContainer.cpp... Applied patch src/settings/konsole.kcfg cleanly. Applied patch src/settings/TabBarSettings.ui cleanly. Applied patch src/ViewContainer.cpp cleanly. OKAY Successfully committed patch.
Comment Actions
From my perspective yes, but now I can't apply it cleanly on current master either (or rather, I can't cleanly merge master into it):
$ (arcpatch-D17814) git pull --rebase origin master From git://anongit.kde.org/konsole * branch master -> FETCH_HEAD First, rewinding head to replay your work on top of it... Applying: Added button to close the tab individually Using index info to reconstruct a base tree... M src/ViewContainer.cpp M src/settings/TabBarSettings.ui M src/settings/konsole.kcfg Falling back to patching base and 3-way merge... Auto-merging src/settings/konsole.kcfg Auto-merging src/settings/TabBarSettings.ui CONFLICT (content): Merge conflict in src/settings/TabBarSettings.ui Auto-merging src/ViewContainer.cpp error: Failed to merge in the changes.
@marssola can you rebase on current master?
Also it would be nice to get @hindenburg's blessing too.
Comment Actions
@marssola are you still there? If you rebase this patch on current master so it applies again, we can continue reviewing it and hopefully get it landed.