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

Text preferences #2162

Merged
merged 6 commits into from Jan 29, 2016
Merged

Text preferences #2162

merged 6 commits into from Jan 29, 2016

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Nov 21, 2015

Adds 4 new preferences for controlling the appearance of text in a QSObjectCell:

  • Underline matched text
  • Make matched text “glow” (a shadow that matches the details color, which is the text color faded by 20%)
  • Allow users to turn off “name ⟷ label” and just show the matched string (the original behavior). A gift for @hmelman and I’m sure others.
  • Allow users to disable text tightening. It causes letters to overlap with some fonts.

This is mostly motivated by some problems that were plaguing users, and my long-time hatred of the underlined letters. (I’ve been using “glow” for like a year locally.) I thought it made sense to do them all together. We could also add a color picker for matched text and some other options, but this is a good starting point.

Note that I updated the NIB’s deployment target.

The defaults for these prefs will mimic the existing behavior.

skurfer added 3 commits Nov 21, 2015
* rename the rankedStringIsName to showRankedStringOnly to better reflect its new behavior
* rearrange boolean tests so faster ones are tried first

fixes #1391, fixes #2133
also, update the NIB deployment target to match the project’s
@hmelman
Copy link

@hmelman hmelman commented Nov 21, 2015

On Nov 21, 2015, at 11:18 AM, Rob McBroom notifications@github.com wrote:

• Allow users to turn off “name ⟷ label” and just show the matched string (the original behavior). A gift for @hmelmanand I’m sure others.

Gifts don't go unnoticed. Thanks :)

Howard

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we have a convention of // Drop 10.10 ... or something whenever we used [NSApplication isSomeOS]?

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't very readable. Any chance we can make it an if/else?

Never mind if you don't want to change it

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you calculation the radius? Why /16?

skurfer
Copy link
Member Author

@skurfer skurfer commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t that be the same character soup with more (parentheses) added to the mix? Or did you have something else in mind?

skurfer
Copy link
Member Author

@skurfer skurfer commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a convention like // drop10.10. I think we only talked about what it was for, but not necessarily when it should be used. I think if there’s a call to isSomeOS, we can find it. The comment is for the less obvious ones.

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, actually once I figured out what it'd be then it would be exactly the same just with more parentheses. Yeah, maybe you can call me a ()() lover

skurfer
Copy link
Member Author

@skurfer skurfer commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh… trial and error? It was over a year ago when I first did that. I only made it public recently. 😃

I’m open to discussing it, or making it a hidden pref. Or maybe even a slider?

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on f8698ef Jan 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, alright! I was just wanting to know if there was any fanciness about this. Maybe it could be made a hidden pref? Don't think it's that crucial to be a slider.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 28, 2016

Cool. Looks good. The code all looks fine apart from a few stylistic things. Gets my blessing if the dev previewers are also happy with it ;-)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jan 29, 2016

Maybe it could be made a hidden pref? Don't think it's that crucial to be a slider.

Done. I edited a line that was indented with spaces, hence the failure. Can’t hold that against me.

Sounds like you’re OK with all this, so I’ll merge tomorrow if you don’t object (or merge) first.

pjrobertson added a commit that referenced this issue Jan 29, 2016
@pjrobertson pjrobertson merged commit aaad643 into master Jan 29, 2016
1 of 2 checks passed
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 29, 2016

There's my confirmation :-)

I haven't tested it, but I assume that's what the dev preview is for

@pjrobertson pjrobertson deleted the textprefs branch Jan 29, 2016
pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on 1d4ae6f Jan 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, quick question:

What happens in the case that a user already has QS? Do any 'missing' QSDefaults still get copied over?
I'm just worried that [defaults objectForKey:kQSTextGlowDivisor] might return nil

skurfer
Copy link
Member Author

@skurfer skurfer commented on 1d4ae6f Jan 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s why it’s important to always add something to this file. If it’s not in ~/Library/Preferences, the value here will get used. From my system:

% defaults read com.blacktree.Quicksilver "Text Glow Divisor"
2016-01-28 19:16:35.757 defaults[79278:7226546] 
The domain/default pair of (/Users/rob/Library/Preferences/com.blacktree.Quicksilver, Text Glow Divisor) does not exist

and yet it still works. 👍

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on 1d4ae6f Jan 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jan 29, 2016

Reports from users that at least two of the prefs do what they were designed to do. (And I’ve been using it for weeks.) Thanks!

skurfer added a commit that referenced this issue Jan 29, 2016
pjrobertson pushed a commit that referenced this issue Jan 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants