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

Basic style support #4

Merged
merged 26 commits into from Nov 21, 2016

Conversation

Projects
None yet
2 participants
@doches
Copy link

commented Sep 6, 2016

Mayur -- I really like the look of Komet, but I usually work with git from a full-screen, white-text-on-black Terminal and the idea of popping up a shockingly white window every time I write a commit message is enough to put me off what is otherwise a lovely, lovely app.

So I thought I'd hack a dark version for myself, and, well, got a bit carried away. Hope this isn't too big a chunk to consider digesting! Screenshots of the sample themes (dark, blue-ish, green-ish, yellow-ish, and a basic white) are including in this PR, as part of the amended README.

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 6, 2016

Wow! This looks awesome! The reason why I didn't add styles before is because I didn't know how to approach that :). I'll try to look over the changes later today. One question regarding the papyrus theme screenshot - are the comments and message intended to be same color (or are they just similar)?

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 6, 2016

Got a chance to try it out and have some initial feedback:

  • The window transparency makes it so that the user can see text behind the window, but this can be a little distracting. Someone suggested to me that the code should use NSVisualEffectView (maybe with a possible background blur filter) instead of adding transparency to the window, although I'm not too familiar with this myself.
  • The "title bar" has grown taller compared to the official branch, and the clicking region (to move the window around) is smaller (or offsetted off) than what a user may expect. Perhaps this bar should change to the same size as it looks like in official version?
  • The mouse caret color in dark mode is not visible -- perhaps we need to change and keep track of this too.
  • The scrollbar background color doesn't change across themes (eg: dark mode). What should the expected behavior here be? And what should the expected behavior be if the "show scroll bars" setting in System Preferences (General) is changed? Not entirely sure myself.
  • Vertical resizing of the commit window no longer works. Perhaps from auto layout issues?

Good work so far! I think I like Collegiate the most :).

Trevor Fountain added some commits Sep 6, 2016

Trevor Fountain
Restore vertical resizing, and shrink top bar
Also, the previous size of the top bar was silly and arbitrary --
reverting it to a smaller size to match the height of a system modal's
top bar.
Trevor Fountain
Use NSVisualEffectView for the editor content
On 10.9 and earlier, NSVisualEffectView isn't available so we fallback
to simple colored view with faint transparency.
Trevor Fountain
Re-shoot screenshots with light/dark content
The style screenshots don't show the vibrancy effect, but at least they
are honest about the background colors where the previous ones no longer
were.
Trevor Fountain
Fix janky scrollbar position
If scrollbars are set to 'Always' under system preferences they were
appearing with a weird offset to the top and right. I'd cheated and used
trailing+top space around the scroll view to offset text; this commit
changes those offsets to be what they ought to be, and instead uses
NSTextView's own insets.
@doches

This comment has been minimized.

Copy link
Author

commented Sep 7, 2016

All (very) good points. I've swapped the coloured backgrounds for NSVisualEffectViews, and done a little bit of runtime hackery to fall back to something sensible if NSVisualEffectView isn't available (on OS X 10.9 and earlier). The blurred/vibrancy background is really nice, actually -- I'd played with it before but dropped it in favour of the coloured views I eventually went with. Really glad you suggested going back to it!

I also shrunk the top bar to match what it was before, and thus to match the system modal toolbar height and be draggable like a user would expect. And yep, I just totally borked vertical resizing; my bad. That's fixed, too -- but with a minimum vertical size of 300px, as shrinking the window much smaller doesn't make a lot of sense to me.

Cursor colour is kept consistent with text colour now too, and the scrollbar handle is light on dark backgrounds/dark on light backgrounds. I hadn't event thought of the 'Always show scrollbars' preference (it's bloody ugly when turned on, thanks Apple) but the scrollview now behaves/looks sensibly when it's turned on. Or at least, it behaves/looks like Terminal.

Trevor Fountain added some commits Sep 7, 2016

Trevor Fountain
Add a red ('Mordor') theme.
Mostly, this is just to make a reference commit that demonstrates how to
add new themes. Also, some people just like red. I'm not one of those
people, but hey. What can you do?
Trevor Fountain
Move 'default' screenshot to top of README.
Replaces it in the list of themes with a screenshot of the 'Mordor'
theme.
@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 8, 2016

Thanks for addressing the issues!

All (very) good points. I've swapped the coloured backgrounds for NSVisualEffectViews, and done a little bit of runtime hackery to fall back to something sensible if NSVisualEffectView isn't available (on OS X 10.9 and earlier)

I think I'd prefer to have less code. Komet never was able to run on 10.9 in the first place. If 10.9 compatibility is a worthwhile possibility, the backwards compatibility code can always be re-added. (unrelatedly, respondsToSelector: is not a great way to determine such compatibility).

The blurred/vibrancy background is really nice, actually -- I'd played with it before but dropped it in favour of the coloured views I eventually went with. Really glad you suggested going back to it!

Cool. Are the setOpaque: and setBackgroundColor: calls still necessary on the window? Are the horizontal line and colored view classes still needed? I don't see any visual difference when I comment out the drawRect: code from ZGHorizontalLine class.

Good work on adjusting the scrollbar. Yeah, it's ugly to have it always visible even though some people do that.

Some observations:

  • The official branch has a white background and grayish titlebar, whereas this version has grayish background and white titlebar (under the default theme). Perhaps the background is okay (I'm not sure yet), but the bright contrasting titlebar color strikes out to me.
  • With the dark theme, you can no longer see the button border for the commit button.
  • Styles toolbar item should be re-ordered to be before Advanced in Preferences.
  • The warning highlight text should avoid being red in the themes because red screams out "ERROR". Yellow is indicative of warning, but other colors could be okay too depending. Also, the highlight color should not be heavy and the text should still be quite readable across all themes.
  • The padding between the text and the text view (or is it something else?) from the side and top has gotten larger since the official branch. Perhaps it's too large now?
@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 8, 2016

Ah also, don't use @synthesize. Let the compiler implicitly auto-synthesize a = _a

If you're editing using Xcode, I think you can set the project settings to use Tabs in the File Inspector so you don't accidentally use spaces :).

@doches

This comment has been minimized.

Copy link
Author

commented Sep 8, 2016

I think I'd prefer to have less code. Komet never was able to run on 10.9 in the first place.

Yep, it wasn't until after I added that that I realised you hadn't intended it to run on anything short of 10.10 anyway. Out the door it goes.

Out of curiousity, what's a better way you'd do that check? isKindOfClass:?

Are the setOpaque: and setBackgroundColor: calls still necessary on the window? Are the horizontal line and colored view classes still needed?

Nope. Coloured view still is (that's what draws the filled bar) but I've axed the line subclass and the unnecessary calls.

The official branch has a white background and grayish titlebar, whereas this version has grayish background and white titlebar (under the default theme). Perhaps the background is okay (I'm not sure yet), but the bright contrasting titlebar color strikes out to me.

A matter of taste, but sounds fair to me. I've darkened the title bar colour on the default theme.

With the dark theme, you can no longer see the button border for the commit button.

Bleurgh. It doesn't seem to be easy to style that button border without subclassing NSButton and, I suspect, entirely re-implementing its drawing. I'd rather not do that (overkill!) so I propose that we either leave it as missing on the dark theme or remove it entirely and find some other way to differentiate those two buttons (bold commit, or slightly lighten cancel). I'd bias towards removing it and not differentiating them, but it's (obviously) your call.

Styles toolbar item should be re-ordered to be before Advanced in Preferences.

Yep.

The warning highlight text should avoid being red in the themes because red screams out "ERROR". Yellow is indicative of warning, but other colors could be okay too depending. Also, the highlight color should not be heavy and the text should still be quite readable across all themes.

Yep.

The padding between the text and the text view (or is it something else?) from the side and top has gotten larger since the official branch. Perhaps it's too large now?

Ah, here I disagree. The official branch feels very cramped to me; the spacing I've gone with is intended to give the text room to breath. A little whitespace around the edges of the window (with inset space between the bottom of the bar and top of the content to maintain visual symmetry) makes the whole thing much more readable to me.

Ah also, don't use @synthesize. Let the compiler implicitly auto-synthesize a = _a

Huh. It's been a while since I've written any Objective-C -- that's a nice improvement.

If you're editing using Xcode, I think you can set the project settings to use Tabs in the File Inspector so you don't accidentally use spaces :).

Yep. I just have it set to spaces for pretty much everything else I work on, and forgot to swap it back. There's probably a way to setup Xcode to handle per-project styles, but eh.

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2016

Thanks for the changes. Some feedback:

Out of curiousity, what's a better way you'd do that check? isKindOfClass:?

-[NSProcessInfo isOperatingSystemAtLeastVersion:] would be one way; the reason is that a method can be implemented privately in an older OS and be half-baked, so the objc runtime is not a reliable test. Swift's #available feature tests the OS version as well.

Nope. Coloured view still is (that's what draws the filled bar) but I've axed the line subclass and the unnecessary calls.

I think _topBar.backgroundColor = _style.barColor can be turned into _topBar.layer.backgroundColor = _style.barColor.CGColor so ZGColoredView may not be necessary.

A matter of taste, but sounds fair to me. I've darkened the title bar colour on the default theme.

I like the colors better together now, but I'm wondering if the default theme should still be a white background which may be more normal (eg: default Terminal is white bg). What part of the code/interface is responsible for that light darkish background color? Also should the background color change when the window is active vs inactive? It's kind of interesting to see the color change suddenly when you launch Komet from Xcode.

It's also kind of interesting that the focus is more on the top bar background color rather than on the text view background color, where this may be the opposite of what other theme-able text editors focus on changing. Should or could the focus be switched?

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2016

Ah, I didn't realize the bg color had an effect depending on what window I had behind it.

One suggestion that was given to me was just having an option/checkbox for translucency, which is whether to use the effect view or not verse a bg color. The top bar color can still be changed.

@doches

This comment has been minimized.

Copy link
Author

commented Sep 12, 2016

I don't think we could easily switch the focus, as you can't tint an NSVisualEffectView. We could drop the top bar colours entirely, but I think they bring something to the table: they draw your eye when the editor pops open, if nothing else.

Adding a toggle for translucency was, if nothing else, super easy. So there's that now. Oh, and _topBar.layer.backgroundColor = _style.barColor.CGColor doesn't work for me; the bar remains a stalwart black.

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 12, 2016

I like the option to not use translucency :). Some feedback:

I don't think we could easily switch the focus, as you can't tint an NSVisualEffectView. We could drop the top bar colours entirely, but I think they bring something to the table: they draw your eye when the editor pops open, if nothing else.

Yeah, I think I like the top bar color change.

Oh, and _topBar.layer.backgroundColor = _style.barColor.CGColor doesn't work for me; the bar remains a stalwart black.

It works for me, but I had to comment out the ZGColoredView drawRect: implementation. The idea would be to remove ZGColoredView all together though.

Ah, here I disagree. The official branch feels very cramped to me; the spacing I've gone with is intended to give the text room to breath. A little whitespace around the edges of the window (with inset space between the bottom of the bar and top of the content to maintain visual symmetry) makes the whole thing much more readable to me.

Can you use the textContainerInset (grep for this in ZGEditorWindowController.m; it's already being altered) instead of positioning the scroll view away from the content view instead?

I was trying to figure out why the background color was slightly gray in the default theme and I think a friend explained to me probably what was happening:

The grayness is coming from the effect view, because no other view on top has any color.
They're all transparent with transparent background colors
so it hits the effect view and that does seem to be resulting in gray,
rather than itself being transparent and showing the window's background color
The correct thing to be doing is setting the background color of the scrollview
to something other than fully transparent.
@doches

This comment has been minimized.

Copy link
Author

commented Sep 13, 2016

I can't believe I didn't change the colored view back to an NSView when I tried setting the layer background. D'oh. Of course that didn't work. And you're right; we should use the inset to offset text instead of shifting the scroll view around.

Both done in the above commits.

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 13, 2016

Awesome. For the scrollview background color, I may venture doing something like this (I'm quickly jotting this possibility down):

     // Style content area
-    _contentView.state = (ZGReadDefaultWindowVibrancy() ? NSVisualEffectStateFollowsWindowActiveState : NSVisualEffectStateInactive);
+       BOOL vibrant = ZGReadDefaultWindowVibrancy();
+    _contentView.state = (vibrant ? NSVisualEffectStateFollowsWindowActiveState : NSVisualEffectStateInactive);
     _contentView.material = _style.material;

        // Style scroll view
        _scrollView.scrollerKnobStyle = _style.scroll;
+       
+       if (vibrant)
+       {
+               _scrollView.drawsBackground = NO;
+       }
+       else
+       {
+               _scrollView.drawsBackground = YES;
+               _scrollView.backgroundColor = _style.fallbackBackgroundColor;
+       }
@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Sep 14, 2016

We may want to also consider if we should invoke -[NSTextView setSelectedTextAttributes:] in updateWindowStyle - maybe change highlight foreground/background at least for light vs dark? This may override the highlight color the user chooses in System Preferences though, but editors appear to do this on style changes.

The Style View auto layout constraints aren't valid at the moment, although I could fix that up later if you don't want to deal with that.

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Oct 13, 2016

Sorry I've been neglecting this! I've been kind of side tracked.. I do want to merge this, but I may still contemplate on adjustments afterwards, like I'm not quite sold on the extra inset space yet :).

Could you fix up the indenting to use tabs, and change brace style to match rest of the code? In addition, let me know what name you'd like to be credited with, or create a standard Credits document to show up in the About window :).

If you want to put more time into the changes, these are some considerations that still have come to my mind:

  • Potential horizontal divider below top bar
  • Suitable text highlight color only for Dark theme
  • Autolayout constraints in Preferences Style view

Thanks!

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Nov 14, 2016

I have created a branch based on this pull request and started doing some tweaking.

@zorgiepoo zorgiepoo merged commit 4b947b6 into zorgiepoo:master Nov 21, 2016

@zorgiepoo

This comment has been minimized.

Copy link
Owner

commented Nov 21, 2016

I've finally merged the changes into master! Any future improvements can be made in an additional pull request. I have credited you in the about window, thanks for all the work! 😊

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.