Add a TerminalHeader when using splits
AbandonedPublic

Authored by tcanabrava on Mon, May 20, 9:32 AM.

Details

Summary

The idea is that a SplitterHeaderBar will be added in the
near future so the checks for the parent widget could be
wrong.
If instead of checking for a specific widget we check for
anything and walk to the parent list untill we find
a TerminalWidget seems sane, and safer.

Phabricator messed with this review and send the wrong diff together (because of the same parent)
so I'm rephrasing it.

Diff Detail

Repository
R319 Konsole
Branch
splitters_header_bar_v2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12027
Build 12045: arc lint + arc unit
tcanabrava created this revision.Mon, May 20, 9:32 AM
Restricted Application added a project: Konsole. · View Herald TranscriptMon, May 20, 9:32 AM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Mon, May 20, 9:32 AM
tcanabrava updated this revision to Diff 58370.Mon, May 20, 3:18 PM
  • Add HeaderBar to TerminalDisplay
tcanabrava retitled this revision from Relax rules for split selection via keyboard to Add a TerminalHeader when using splits.Mon, May 20, 3:19 PM
tcanabrava edited the summary of this revision. (Show Details)
tcanabrava added a reviewer: ngraham.
This revision was not accepted when it landed; it landed in state Needs Review.Tue, May 21, 1:59 PM
This revision was automatically updated to reflect the committed changes.
tcanabrava reopened this revision.Tue, May 21, 2:05 PM

phabricator is tracking the wrong branch , it's *not* merged. (can I have a review? :)

@hindenburg I have a long list of branches that are build on top of other branches, and phabricator is breaking my reviews (four to date, just this week).
would you accept konsole to move to gitlab?

ngraham added a reviewer: VDG.Tue, May 21, 2:09 PM

It's on my to-do list today. I took a look and here are some initial comments:

  • The button next to the close button needs a tooltip because its icon doesn't make it obvious what it does. Maybe a better icon too, like zoom-in or view-fullscreen
  • Once in zoomed/maximized mode, it's not obvious how to get out of it
  • Might wanna add a horizontal line under the menubar to separate it from the split views' headerbars

@hindenburg I have a long list of branches that are build on top of other branches, and phabricator is breaking my reviews (four to date, just this week).
would you accept konsole to move to gitlab?

+1, I think that would be fantastic!

tcanabrava updated this revision to Diff 58410.Tue, May 21, 2:36 PM
  • Add toggle of the Maximize / Minimize split
tcanabrava updated this revision to Diff 58412.Tue, May 21, 2:51 PM
  • Fix build
ngraham requested changes to this revision.Tue, May 21, 3:01 PM

If the maximize button is a toggle, it needs to look visually toggled when active. Also the icon should be something more like view-fullscreen.

I'd like the active split's header to look more like an active tab so it's easy to tell what's active. So inactive headers should probably be darkened like inactive tabs

This revision now requires changes to proceed.Tue, May 21, 3:01 PM
tcanabrava updated this revision to Diff 58413.Tue, May 21, 3:10 PM
  • Icon / Tooltip
tcanabrava updated this revision to Diff 58415.Tue, May 21, 3:35 PM
  • Palette highligth fixes

Fantastic. The dark inactive tab is maybe a bit too dark, but that's a fairly minor thing.

One remaining UI niggle I can see is that I'd like a horizontal line separating the headerbars from the menubar for the case where the tab bar is hidden. When the tab bar is shown, it provides its own line, which looks good:

But when the tab bar is hidden, there's no separation and it looks a bit weird:

Or maybe this isn't a big deal? What do you think? VDG?

shubham added inline comments.
src/TerminalHeaderBar.h
2

Please add copyright header

tcanabrava updated this revision to Diff 58420.Tue, May 21, 4:13 PM
  • Add license headers
ndavis added a subscriber: ndavis.Tue, May 21, 6:44 PM

Fantastic. The dark inactive tab is maybe a bit too dark, but that's a fairly minor thing.

I agree, the header for the prompt on the right seems too dark because text has poor contrast.

One remaining UI niggle I can see is that I'd like a horizontal line separating the headerbars from the menubar for the case where the tab bar is hidden. When the tab bar is shown, it provides its own line, which looks good:

But when the tab bar is hidden, there's no separation and it looks a bit weird:

Or maybe this isn't a big deal? What do you think? VDG?

Without separation, it reminds me of old Safari: https://www.uberphones.com/wp-content/uploads/2011/06/ipad-safari-tabs-update.jpg

I don't think it's a big deal, but it might be nice to have a separating line.

Also, if the button with the view-fullscreen icon is a maximize button, wouldn't it be better to use window-maximize or window-restore?

I'll do what the VDG decides.
but this is fixing an actuall issue so maybe it's worth to accept and then
fine tune later?

@hindenburg I have a long list of branches that are build on top of other branches, and phabricator is breaking my reviews (four to date, just this week).
would you accept konsole to move to gitlab?

+1, I think that would be fantastic!

I thought everyone was moving soon-ish?

We are but it’s a per project basis

Em qui, 23 de mai de 2019 às 16:59, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D21301

In D21301#467910 https://phabricator.kde.org/D21301#467910, @ngraham
https://phabricator.kde.org/p/ngraham/ wrote:

In D21301#467903 https://phabricator.kde.org/D21301#467903, @tcanabrava
https://phabricator.kde.org/p/tcanabrava/ wrote:

@hindenburg https://phabricator.kde.org/p/hindenburg/ I have a long
list of branches that are build on top of other branches, and phabricator
is breaking my reviews (four to date, just this week).
would you accept konsole to move to gitlab?

+1, I think that would be fantastic!

I thought everyone was moving soon-ish?

*REPOSITORY*
R319 Konsole

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

*To: *tcanabrava, Konsole, hindenburg, ngraham, VDG
*Cc: *kvermette, thsurrel, rizzitello, mart, hindenburg, gennad, fabianr,
ndavis, shubham, konsole-devel, ngraham, maximilianocuria

Anyway for now let's make the dark header less dark and add a line and get this in IMO. Then we can tweak later.

ndavis added a comment.EditedThu, May 23, 11:47 PM

Anyway for now let's make the dark header less dark and add a line and get this in IMO. Then we can tweak later.

Right, I can agree with this. I think the color should be the same as the inactive tab color.

We are but it’s a per project basis

Oh, I was just waiting for the sysadmins to tell everyone. If I need to do something, let me know please.

tcanabrava abandoned this revision.Mon, Jun 3, 2:23 PM

Moving to gitlab