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

Ensure large type text is sized based on screen HEIGHT as well as width #733

Merged
merged 3 commits into from Mar 23, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 25, 2012

Also, change the string name from number to aString. Makes more sense

If you have a string that is really really long, or has many lies, then Quicksilver would previously not set the font size to ensure all the lines are included in the large type display.

e.g. paste this into QS's 1st pane before, and after this fix.

a
b
c
d
e
f
g
h
i
j
k

Also, change the string name from `number` to `aString`. Makes more sense
@skurfer
Copy link
Member

@skurfer skurfer commented Feb 27, 2012

I like it. I’ll run with it for a while.

textSize = [aString sizeWithAttributes:[NSDictionary dictionaryWithObject:textFont forKey:NSFontAttributeName]];
if (textSize.width > displayWidth+[textFont descender] *2 || (textSize.height > displayHeight+[textFont descender] *2)) {
break;
} // ***warning * use ascenders to calculate
Copy link
Contributor

@HenningJ HenningJ Feb 27, 2012

Choose a reason for hiding this comment

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

What is this warning? What does it mean? Is it still valid?

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Feb 27, 2012

I like it as well. I just had a few questions about the code. Not really about your changes, but about what was there before...

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 27, 2012

I was unsure about that 2*EDGEINSET bit as well, but I considered the 'if
it aint broke leave it' philosophy :)

Perhaps when Rob looks at merging all this into one global method (as he
mentioned in the dev groups) he can look into this? (Hehe - sorry Rob!)

To be perfectly honest, I have no idea what a descender/ascender is (of
course I could find out, so don't bother explaining!)

On 27 February 2012 14:17, Henning Jungkurth <
reply@reply.github.com

wrote:

I like it as well. I just had a few questions about the code. Not really
about your changes, but about what was there before...


Reply to this email directly or view it on GitHub:
#733 (comment)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Feb 27, 2012

I have no idea what a descender/ascender is (of course I could find out, so don't bother explaining!)

I don't have a clue either. :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 17, 2012

Any further problems with this? It's annoying not having the full text when I want to see a song's lyrics... :)

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 17, 2012

I noticed one small thing. If you have a narrow column of text, the Large Type display is still very wide. For instance, try ls -1 ~ → Run Command in Shell then Large Type the result.

I don’t think that should hold this up, so I’ll merge it as is, but… Just in case you plan to fix it and would prefer to do it on this existing branch, I’ll give you a few hours to speak up before I merge.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 17, 2012

I am looking into this now, thanks for holding off!

Been busy watching Wales win the Grand Slam - rugby :)

On 17 March 2012 14:49, Rob McBroom <
reply@reply.github.com

wrote:

I noticed one small thing. If you have a narrow column of text, the Large
Type display is still very wide. For instance, try ls -1 ~ → Run Command
in Shell then Large Type the result.

I don’t think that should hold this up, so I’ll merge it as is, but… Just
in case you plan to fix it and would prefer to do it on this existing
branch, I’ll give you a few hours to speak up before I merge.


Reply to this email directly or view it on GitHub:
#733 (comment)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 18, 2012

OK @skurfer
Take a look at the change I've made :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 18, 2012

Definitely better for narrow columns, but now I see other weirdness.

The shadow for some taller characters looks like it’s getting cut off at the bottom (You may have to look at it full size. See the ]; at the end in particular):

Truncated Shadow

If the text being displayed has some leading whitespace, the position gets weird:

Leading Whitespace

This message would have been much easier to put together with a Dropbox plug-in to grab those links for me. ;-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 18, 2012

Hmm... OK, I was worried there might be some problems.
I think I might just revert the commit and then let you merge it.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 22, 2012

I think I might just revert the commit and then let you merge it.

Sounds fine.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 22, 2012

OK I've reverted.

On 22 March 2012 13:19, Rob McBroom <
reply@reply.github.com

wrote:

I think I might just revert the commit and then let you merge it.

Sounds fine.


Reply to this email directly or view it on GitHub:
#733 (comment)

skurfer added a commit that referenced this issue Mar 23, 2012
Ensure large type text is sized based on screen HEIGHT as well as width
@skurfer skurfer merged commit 63f456d into quicksilver:master Mar 23, 2012
@skurfer
Copy link
Member

@skurfer skurfer commented Mar 23, 2012

Merged

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