Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/3268 Add dark tab bar #185

Merged
merged 3 commits into from Dec 2, 2014

Conversation

Projects
None yet
4 participants
@wavebeem
Copy link
Contributor

wavebeem commented Oct 29, 2014

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Oct 29, 2014

Still needs a preference pane option to toggle tab modes, but I wanted to get this code up first.

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Nov 8, 2014

I feel like this is a reasonable idea but that there should be more code reuse with the superclass. For example, the way colored tabs work should be consistent across both light and dark tabs. Feel free to add methods to PSMYosemiteTabStyle that are overridden by the subclass (e.g., accessors for colors) to facilitate code sharing.

@DomT4

This comment has been minimized.

Copy link

DomT4 commented Nov 8, 2014

This looks great 👍. The light tabs can jar with the dark background at times, particularly at night.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 8, 2014

@gnachman That makes sense and sounds great. For the proof of concept I wanted to not mess around with PSMYosemiteTabStyle since I figured that was in development right now, but I'll happily work on shrinking the dark style through more code sharing.

I had an issue with drawing the colored tabs the way the superclass does. It seemed like the tab bar was being cut off by the titlebar on the top, but maybe I was just messing up my coordinates for drawing.

Also, a related issue that came when I was working on this is the fact that the titlebar does not have a border along the bottom, so I am manually drawing a top border on the tab bar to separate the two. It's probably outside the scope of this issue, but in the case where you have no tab bar this looks pretty weird (especially in a light terminal). I tried looking around for the right way to get the border along the bottom of the titlebar, but got lost quickly.

tl;dr

Will improve code sharing and push changes. Thanks for the responses and support.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 8, 2014

Here's some images showing the problem with the current colored tan drawing code. Note the size change when using windowed vs fullscreen mode.

Tab stripe is cropped

screen shot 2014-11-08 at 2 45 32 pm

Tab stripe shows normally

screen shot 2014-11-08 at 2 45 40 pm

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 8, 2014

This seems to be not only cutting off the tab stripe, but also the top border on the tabs, which causes the titlebar to visually merge right into the tab bar. I'll look into this.

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Nov 9, 2014

You're right about it overlapping. That was fixed in commit 5264273 and shouldn't be an issue any more.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 9, 2014

@gnachman I've updated DarkTabStyle so all it does is redefine colors/gradients, not any drawing code.

What would you like for the preferences UI for this feature? Maybe something like Use dark tabs under Appearance / Tabs in the preferences?

I tweaked the colors to be a bit more subtle like the new Yosemite tab style.

screen shot 2014-11-08 at 10 22 59 pm

#import "PSMYosemiteTabStyle.h"

@interface PSMDarkTabStyle : PSMYosemiteTabStyle <PSMTabStyle>
{

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 10, 2014

Owner

No need for braces here. In general there's no reason to define ivars in header files anyway.

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 10, 2014

Author Contributor

Gotcha. Never made an Obj-C class without variables before.

@implementation PSMDarkTabStyle

- (NSString *)name
{

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 10, 2014

Owner

Let's keep the open brace at the beginning of a method on the same line as the method name, e.g.

  • (NSString *)name {

I've been slowly migrating to this new style and I want to keep new code using it.

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 10, 2014

Author Contributor

I wasn't sure which style was preferred so I went with the one I thought I saw more of. Will gladly change.

- (NSGradient *)backgroundGradientSelected:(BOOL)selected
{
CGFloat lightness = selected ? 0.24 : 0.12;
NSColor *bg = [NSColor colorWithCalibratedWhite:lightness alpha:1.00];

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 10, 2014

Owner

Let's use a slight gradient here; even just a few hundredths adds a bit of depth.

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 10, 2014

Author Contributor

Will be pushing up some slight gradients. Please critique.

@@ -267,17 +267,24 @@ - (NSAttributedString *)attributedObjectCountValueForTabCell:(PSMTabBarCell *)ce
autorelease];
}

- (NSColor *)textColorDefaultSelected:(BOOL)selected
{
return [NSColor colorWithSRGBRed:101/255.0 green:100/255.0 blue:101/255.0 alpha:1];

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 10, 2014

Owner

If selected is YES then it should be black

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 10, 2014

Author Contributor

Missed that. Thanks.

@@ -376,14 +383,34 @@ - (void)drawCellBackgroundAndFrameHorizontallyOriented:(BOOL)horizontal
inRect:(NSRect)cellFrame
selected:(BOOL)selected
withTabColor:(NSColor *)tabColor {

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 10, 2014

Owner

Extraneous white space should be removed

CGFloat angle = horizontal ? 90 : 0;
[[self backgroundGradientSelected:selected] drawInRect:cellFrame angle:angle];

if (tabColor) {

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 10, 2014

Owner

Did this block need to move? If possible keep it where it came from to keep the diff simpler.

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 10, 2014

Author Contributor

Previously the color was drawing on top of the tab borders. This reduced the contrast and made colored tabs look funny. I could alternatively keep the color drawing in the old spot but change the rectangle to not overlap.

[self drawVerticalLineInFrame:cellFrame x:NSMinX(cellFrame)];

BOOL isLeftmostTab = NSMinX(cellFrame) == 0;
if (!isLeftmostTab) {

This comment has been minimized.

Copy link
@gnachman

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 10, 2014

Author Contributor

Thanks! I'm on a mission to eliminate borders-touching-borders in all software ;)

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Nov 10, 2014

I posted a few line comments. Besides those minor things, there are two issues left to deal with:

  1. We need a UI to switch between tab styles, as you mentioned. I'd add another popup menu in prefs>appearance and the code would be in similar places as the code for top/left/bottom tabs. There used to be code to switch tab styles (which is still visible in the v2 branch). The hooks to change tab style were removed in commit 1606fbc.
  2. The spinner is nearly invisible against a dark background. I found this code snippet which will make a spinner light enough to look good here:
    #import <QuartzCore/QuartzCore.h>
...
        CIFilter *lighten = [CIFilter filterWithName:@"CIColorControls"];
        [lighten setDefaults];
        [lighten setValue:@0.3 forKey:@"inputBrightness"];
        [_indicator setWantsLayer:YES];
        [_indicator setContentFilters:@[ lighten ]];

PSMTabBarCell has the spinner in its _indicator ivar. I suppose PSMProgressIndicator could get a -light property that gets set along with the tab color.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 10, 2014

I think my most recent commit address all of the line comments you had.

How do you get the spinner to show up? I feel like I've never noticed one before.

Thanks for the code leads.

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Nov 11, 2014

The spinner shows up when there's output in an inactive tab. For example, run ls -lR / and change tabs; you'll see an activity indicator go until ls finishes.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 12, 2014

Ah, I disabled everything except the tab title long ago. That was the problem.

I have code up for the preferences dialog now. I don't think the new popup menu is great, but there really wasn't room anywhere else. Open to suggestions.

//
//

#ifndef iTerm_PSMDarkTabStyle_h

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

Obj-c doesn't need include guards because we use #import, not #include.

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 14, 2014

Author Contributor

Strangely enough, Xcode added them for me. I never used include guards in Obj-C before for that exact reason.

#import "PSMTabStyle.h"
#import "PSMYosemiteTabStyle.h"

@interface PSMDarkTabStyle : PSMYosemiteTabStyle <PSMTabStyle>

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

No space before <

}

- (NSColor *)textColorDefaultSelected:(BOOL)selected {
const CGFloat lightness = selected ? 1.00 : 0.80;

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

I think 0.8 : 1.0 looks better here, what do you think?

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 14, 2014

Author Contributor

It seems backwards to make the active tab have less readable text, to me. Safari slightly lightens the inactive tab text.

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 14, 2014

Owner

Oops, I mean to say 0.8 : 0.6
Just to make it a bit darker, the text was kind of distracting.

@@ -12,6 +12,7 @@
#import "PSMRolloverButton.h"
#import "PSMTabStyle.h"
#import "PSMYosemiteTabStyle.h"
#import "PSMDarkTabStyle.h"

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

Please keep imports alphabetized

@@ -124,7 +125,8 @@ - (void)initAddedProperties
_cellMaxWidth = 280;
_cellOptimumWidth = 130;
_tabLocation = PSMTab_TopTab;
style = [[PSMYosemiteTabStyle alloc] init];
// style = [[PSMYosemiteTabStyle alloc] init];

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

Remove commented out code and keep Yosemite as the default.

@@ -40,6 +40,8 @@
#import "PseudoTerminal+Scripting.h"
#import "PseudoTerminalRestorer.h"
#import "PSMTabStyle.h"
#import "PSMYosemiteTabStyle.h"

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

Alphabetize imports

- (void)updateTabBarStyle {
id<PSMTabStyle> style;
switch ([iTermPreferences intForKey:kPreferenceKeyTabStyle]) {
default:

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

Omit the default: so we'll get a compiler warning if another enum value is added later.

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 14, 2014

Author Contributor

It doesn't look like the compiler complains if I remove one of the cases (along with the default). But I'm fine with removing it nonetheless.

@@ -17,6 +17,11 @@ typedef NS_ENUM(NSInteger, iTermOpenTmuxWindowsMode) {
kOpenTmuxWindowsAsNativeTabsInExistingWindow = 2
};

typedef NS_ENUM(int, iTermPreferencesTabStyle) {

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

Add a comment stating that the values in this enum must be preserved because they're saved in prefs.

@@ -43,6 +43,7 @@
NSString *const kPreferenceKeyTmuxDashboardLimit = @"TmuxDashboardLimit";
NSString *const kPreferenceKeyAutoHideTmuxClientSession = @"AutoHideTmuxClientSession";

NSString *const kPreferenceKeyTabStyle = @"kPreferenceKeyTabStyle";

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 13, 2014

Owner

Names here are meant to be human readable, so let's make the value @"TabStyle".

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Nov 13, 2014

Looking good--I just sent some line comments for minor tweaks.

} else {
startValue = 0.12;
endValue = 0.15;
}

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 14, 2014

Author Contributor

Totally open to suggestions on the gradient values for tabs here. I think it might benefit from being a bit more subtle.

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 14, 2014

Owner

Maybe .27 instead of .29, otherwise looks pretty subtle on my display.

@@ -5616,6 +5622,20 @@ - (void)_refreshTerminal:(NSNotification *)aNotification
}
}

- (void)updateTabBarStyle {
id<PSMTabStyle> style;
switch ([iTermPreferences intForKey:kPreferenceKeyTabStyle]) {

This comment has been minimized.

Copy link
@gnachman

gnachman Nov 14, 2014

Owner

Cast the result of intForKey: to the type of the enum to get the benefit of removing default:

This comment has been minimized.

Copy link
@wavebeem

wavebeem Nov 14, 2014

Author Contributor

Ah, right, it's int not the enum type.

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Nov 14, 2014

This looks pretty good. Do you have any other changes to make before I pull?

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 14, 2014

I still haven't incorporated the light spinner that you asked for.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 19, 2014

I'm currently not sure about a good way to add in the changes to the spinning activity indicator. Maybe something like exposing a list of spinner filters in each tab style? The Yosemite style would expose an empty list there, and Dark style would expose the lighten filter you showed.

And then add something in PSMTabBarControl's - (void)setStyle:(id <PSMTabStyle>)newStyle to update the filter list on each tab cell indicator?

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Nov 20, 2014

I took a stab at it in my clean_up_psm_add_dark_tabs_from_saikobee_and_add_light_indicator branch, please test it and see if you can find any problems.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Nov 21, 2014

It seemed fine toying around with the themes and toggling the tab appearance checkboxes while it was running. I merged your branch into this one so it will merge cleanly.

@gnachman gnachman merged commit 5351f33 into gnachman:master Dec 2, 2014

@gnachman

This comment has been minimized.

Copy link
Owner

gnachman commented Dec 2, 2014

Thanks for all your work on this change. I made a few more improvements to the progress indicator (it was getting stuck when going from dark to light on 10.9) and pulled it.

@wavebeem

This comment has been minimized.

Copy link
Contributor Author

wavebeem commented Dec 2, 2014

Gladly! Thank you for your guidance on this PR, and all your work on iTerm!

@adriaant

This comment has been minimized.

Copy link

adriaant commented Dec 2, 2014

It looks great, thanks to you two!

@DomT4

This comment has been minimized.

Copy link

DomT4 commented Dec 2, 2014

Looks real nice. Very glad to see this in. Thanks to both! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.