Background transparency #201

nathanrobinson · 2015-03-12T22:00:54Z

This fork allows setting different transparency and blur values for background windows.

gnachman · 2015-03-17T18:32:59Z

Wow, thanks for the big change. It will take me a little bit to get through this, but I am taking a look.

gnachman · 2015-03-17T18:53:58Z

I haven't dug in super deep but I think these comments cover most of the issues:

The transparency for background windows doesn't affect the margins (they seem to always use the foreground setting)
The prefs dialog is starting to look (more) like the controls on the space shuttle. How about moving the blur and transparency controls into a modal sheet, like the settings in the prefs>advanced panel? Leave behind a button labeled "Transparency and Blur…" that opens the sheet.
I saw a few places where "bool" and "true" and "false" were used instead of BOOL and YES/NO; looks like some old wrong code. Instead of being consistent, would you mind fixing those?
There's an "Inactivate transparency" label below the "Inactive transparency" checkbox. The label should be removed.
Don't use abbreviations except in extreme cases. KEY_BACKGROUND_TRANS and KEY_USE_BACKGROUND_TRANS should spell out TRANSPARENCY, even if it messes up the alignment of the source code.
Names in code (for example, useBackgroundTransparency) should match what we see in the UI (where “inactive” is used, not “background”).
Minor style issue: new code should use this format for methods:

- (void)foo {

rather than

- (void)foo
{

Always put a space between an if statement and the opening paren of its condition (e.g., in recheckBlur)
The use of Hungarian notation is being phased out. Avoid names like "fVal" (e.g., in setBackgroundTransparency:). In the case of setters, the preferred name matches the property name (so, - (void)setBackgroundTransparency:(double)backgroundTransparency { would be correct).
Why the + 10 in -[ProfilePreferencesViewController awakeFromNib]? Looks fine without it to me.
The /////// Background transparency comments in ProfilesWindowPreferencesViewController.m should be removed or improved (if you keep them, don't use lots of /s, just the two that are required)
In general, avoid lines over 100 characters, unless it would compromise readability. For example, in ProfilesWindowPreferencesViewController.m:94

nathanrobinson · 2015-03-17T21:07:49Z

Thanks for the feedback.
This is my first foray into objective C, so for the first two bullet items I am completely lost. I will try to track them down, though.
The + 10 is there because, at least on my screen, when a preferences screen scrolls off the bottom of the window, a scroll bar is added to the view which covers up part of the controls.
This would fix itself if I am able to move the transparency and blur controls to a popup.
The rest should be easy to fix.

gnachman · 2015-03-18T04:49:30Z

You're doing well for a first foray! Feel free to ignore the second comment, I'll take care of that. As for the margins not being correct, I think it's because the wrong alpha value is picked in -[PTYSession textViewDrawBackgroundImageInView:viewRect:blendDefaultBackground:].

I think the +10 should stay as the advanced prefs actually look pretty ugly with always-visible scrollbars, but instead of always adding 10, add [NSScroller scrollerWidthForControlSize:NSRegularControlSize scrollerStyle:NSScrollerStyleLegacy]. That'll always be correct even if apple changes how it's rendered (plus it explains what's going on). I always forget to test that configuration so I'm glad you caught it.

…ency * origin/master: Don't try to add a titlebar accessory view controller to a window without a title bar. Issue 3449. Make sure to remove tracking rects on the tab bar control before adding new ones. I wasn't able to repro issue 3452 but this should fix it. Clean up tab activity indicator code and various odds and ends

nathanrobinson · 2015-03-18T14:55:43Z

I think I got them all except the popup for transparency sliders on the preferences dialog.

gnachman · 2015-03-18T19:41:19Z

sources/PTYSession.m

@@ -801,6 +801,8 @@ - (BOOL)setScreenSize:(NSRect)aRect parent:(id<WindowControllerInterface>)parent
     horizontalSpacing:[[_profile objectForKey:KEY_HORIZONTAL_SPACING] floatValue]
       verticalSpacing:[[_profile objectForKey:KEY_VERTICAL_SPACING] floatValue]];
    [self setTransparency:[[_profile objectForKey:KEY_TRANSPARENCY] floatValue]];
+    [self setInactiveTransparency:[[_profile objectForKey:KEY_INACTIVE_TRANSPARENCY] floatValue]];


New code should not look up values in _profile directly because it bypasses the relatively new mechanism for defaults. Older code generally had default values of 0 so it happens to work, and I'm slowly moving everything the new model. Instead, write:
[iTermProfilePreferences objectForKey:KEY_INACTIVE_TRANSPARENCY inProfile:_profile]

gnachman · 2015-03-18T20:37:20Z

I added a bunch of niggling line notes, mostly style stuff. Thanks for taking the time to do this!

nathanrobinson · 2015-03-18T22:11:05Z

Well, I think I got it, but you'll want to double check me.

gnachman · 2015-03-18T22:23:15Z

sources/PTYSession.m

+    return [iTermProfilePreferences floatForKey:KEY_INACTIVE_BLUR_RADIUS inProfile:_profile];
+}
+
+- (float)blurRadius {


Both float and double are used here; I'm sure that's due to some ancient folly. Let's use double for all new code.

gnachman · 2015-03-18T22:27:18Z

sources/PTYTextView.m

+    [self setNeedsDisplay:YES];
+}
+
+- (void)setIsBackground:(BOOL)isBackground


For style and consistency, this should be setWindowInactive: and the backing variable should be _windowInactive.

I tested out appCode’s reformatting and wanted to get your thoughts. It handles most of your styles, but seems to add extra indents on some multi-line constructs.

nathanrobinson · 2015-03-19T01:19:39Z

I tested out appCode’s reformatting and wanted to get your thoughts. It
handles most of your styles, but seems to add extra indents on some
multi-line constructs.

gnachman · 2015-03-19T03:32:00Z

sources/PTYTab.m

 }

- (double)blurRadius
-{
+- (double)averageBlurRadiusForInactive:(BOOL)inactive {
    double sum = 0;
    double count = 0;
    NSArray* sessions = [self sessions];


Space before *, also on next line

gnachman · 2015-03-19T03:49:05Z

Please revert the automatic style changes to PTYTextView and let's revisit that later. I'm working on a significant refactor of that file in a feature branch and this will introduce thousands of merge conflicts.

This reverts commit e64105e.

…ency] is used by [PTYSession useInactiveTransparency] so I had to leave that in.

nathanrobinson · 2015-03-19T19:31:30Z

I'm not sure what happened, but now the original blur radius settings aren't working. I'll see if I can track that down.

nathanrobinson added 3 commits March 12, 2015 14:59

added controls for inactive window transparency and blur

26dd7b3

blur on dimm

b4cc2bd

transparent values for background windows

821aee9

nathanrobinson added 2 commits March 18, 2015 07:52

Fix all of these issues except the perks dialog: gnachman#201 (comment)

63b5c18

gnachman reviewed Mar 18, 2015
View reviewed changes

This should fix all these issues: gnachman#201 (comment)

c36180c

gnachman reviewed Mar 18, 2015
View reviewed changes

nathanrobinson added 2 commits March 18, 2015 20:14

https://github.com/gnachman/iTerm2/pull/201#discussion-diff-26713933

4b536ec

testing appCode's reformatting

e64105e

I tested out appCode’s reformatting and wanted to get your thoughts. It handles most of your styles, but seems to add extra indents on some multi-line constructs.

gnachman reviewed Mar 19, 2015
View reviewed changes

nathanrobinson and others added 3 commits March 19, 2015 07:54

Revert "testing appCode's reformatting"

fe38c26

This reverts commit e64105e.

re: gnachman#201 (diff) it turns out [PTYTextView useInactiveTranspar…

700e01e

…ency] is used by [PTYSession useInactiveTransparency] so I had to leave that in.

https://github.com/gnachman/iTerm2/pull/201/files#r26714096

5f1a31c

nathanrobinson added 2 commits March 19, 2015 16:10

update blur when window goes active/inactive

1dea36b

add setting for text transparency on inactive windows

951bcce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background transparency #201

Background transparency #201

nathanrobinson commented Mar 12, 2015

gnachman commented Mar 17, 2015

gnachman commented Mar 17, 2015

nathanrobinson commented Mar 17, 2015

gnachman commented Mar 18, 2015

nathanrobinson commented Mar 18, 2015

gnachman Mar 18, 2015

gnachman commented Mar 18, 2015

nathanrobinson commented Mar 18, 2015

gnachman Mar 18, 2015

gnachman Mar 18, 2015

nathanrobinson commented Mar 19, 2015

gnachman Mar 19, 2015

gnachman commented Mar 19, 2015

nathanrobinson commented Mar 19, 2015

Background transparency #201

Are you sure you want to change the base?

Background transparency #201

Conversation

nathanrobinson commented Mar 12, 2015

gnachman commented Mar 17, 2015

gnachman commented Mar 17, 2015

nathanrobinson commented Mar 17, 2015

gnachman commented Mar 18, 2015

nathanrobinson commented Mar 18, 2015

gnachman Mar 18, 2015

Choose a reason for hiding this comment

gnachman commented Mar 18, 2015

nathanrobinson commented Mar 18, 2015

gnachman Mar 18, 2015

Choose a reason for hiding this comment

gnachman Mar 18, 2015

Choose a reason for hiding this comment

nathanrobinson commented Mar 19, 2015

gnachman Mar 19, 2015

Choose a reason for hiding this comment

gnachman commented Mar 19, 2015

nathanrobinson commented Mar 19, 2015