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

String value #424

Merged
merged 5 commits into from Jul 19, 2011
Merged

String value #424

merged 5 commits into from Jul 19, 2011

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jul 19, 2011

The discussion on #419 has made me look at the stringValue method and there are some things that needed cleaning up.

  1. The string value for an e-mail address should just be the e-mail address. That way, you don’t have to request the string value, then manually strip the “mailto:” prefix. Actions that want to use the e-mail in a URL context request the URL type which will contain the prefix.
  2. I didn’t like the technique of blindly grabbing a type whether it exists or not, so I switched to testing for the type’s existence and then returning the first one found.
  3. We decided long ago that even one-line if statements should use curly braces.

Are there any other types we should be checking for before falling back on the text type/display name?

@pjrobertson
Copy link
Member

Seems fine to me. I've had a quick play, and typically for objects of other
types the displayName seems to give what is expected.

I'll merge it with my plainText branch just to make sure it's all fine, then
merge here

On 19 July 2011 05:40, skurfer <
reply@reply.github.com>wrote:

The discussion on #419 has made me look at the stringValue method and
there are some things that needed cleaning up.

  1. The string value for an e-mail address should just be the e-mail
    address. That way, you dont have to request the string value, then manually
    strip the mailto: prefix. Actions that want to use the e-mail in a URL
    context request the URL type which will contain the prefix.
  2. I didnt like the technique of blindly grabbing a type whether it
    exists or not, so I switched to testing for the types existence and then
    returning the first one found.
  3. We decided long ago that even one-line if statements should use curly
    braces.

Are there any other types we should be checking for before falling back on
the text type/display name?

Reply to this email directly or view it on GitHub:
#424

@pjrobertson
Copy link
Member

I've found one place where this doesn't work: Address Book contacts.

Since EmailAddressType is first, trying to paste a contact pastes their email address. I'd expect it to paste their name.

Perhaps the right way was to have least specific → most specific?
Or we could just add:

    if ([self containsType:QSABPersonType]) {
        return [self objectForType:QSABPersonType];
    }

@pjrobertson
Copy link
Member

^^ That doesn't work. Since QSABPersonType is a UUID

it should possible be:

    return [self name];

@skurfer
Copy link
Member Author

skurfer commented Jul 19, 2011

Since EmailAddressType is first, trying to paste a contact pastes their email address. I'd expect it to paste their name.

So it does. Hmmm. The reason I put the more specific value first is that URLs (and by extension, e-mail addresses) have a value for QSTextType and it includes the mailto: prefix for addresses. I don’t know if there’s a good reason for that. Maybe the mailto: prefix should only exist in the URL type.

I don’t think that would have any unwanted side-effects. The existence of the text type on URLs is necessary to make certain actions appear, but I suspect the actual value associated with the text type is rarely used.

I’ll make the change and run with it for a while.

@skurfer
Copy link
Member Author

skurfer commented Jul 19, 2011

By the way… testing tip: An easy way to see what stringValue does with an object is to hit ‘.’ for text entry mode.

@pjrobertson
Copy link
Member

By the way testing tip: An easy way to see what stringValue does with
an object is to hit . for text entry mode.

Cool! :)

On 19 July 2011 14:02, skurfer <
reply@reply.github.com>wrote:

By the way testing tip: An easy way to see what stringValue does with an
object is to hit . for text entry mode.

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

@skurfer
Copy link
Member Author

skurfer commented Jul 19, 2011

OK, try with these two commits.

Another mystery I’m now looking into: Why does the e-mail type get an array instead of a string? This was original from the first commit, so I didn’t alter it (just moved it), but it seems weird.

@pjrobertson
Copy link
Member

Have you tried just using a string instead of an array for that bit?

Edit not very clear

@pjrobertson
Copy link
Member

Well, so that still doesn't work for Address book types. I think you may need to add those lines that I suggested, as opposed to 0878456 and moving from least → most specific.

@skurfer
Copy link
Member Author

skurfer commented Jul 19, 2011

Have you tried just using a string instead of an array for that bit?

No, but I’m about to. Looking around (in QS and in plug-ins) it looks like everything that Alcor wrote sets an array (always with only one object) for the e-mail type. Makes me wonder:

  1. Why?
  2. Why doesn’t that break things? I can’t find any method that pulls the array apart before using it. Seems like using an array where a string is expected would cause things to blow up.

@pjrobertson
Copy link
Member

maybe for [object arrayForType:] or [object objectForType] where object
is multiple objects?

Don't know!

...please document it if you find out! ;)

On 19 July 2011 15:39, skurfer <
reply@reply.github.com>wrote:

Have you tried just using a string instead of an array for that bit?

No, but Im about to. Looking around (in QS and in plug-ins) it looks like
everything that Alcor wrote sets an array (always with only one object) for
the e-mail type. Makes me wonder:

  1. Why?
  2. Why doesnt that break things? I cant find any method that pulls the
    array apart before using it. Seems like using an array where a string is
    expected would cause things to blow up.

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

@skurfer
Copy link
Member Author

skurfer commented Jul 19, 2011

Not only does it appear to work just fine with a string, you get to see the e-mail address in the details now. So I’ve added another commit.

Well, so that still doesn't work for Address book types.

What’s not working? If I select a contact and hit ., the person’s name is what appears in the text box. I notice the paste action doesn’t work (beeps), but your new paste plain does. But I’m pretty sure Paste always failed on a contact.

@pjrobertson
Copy link
Member

*sorry, it is working now :)

Re-merging this with my plainText branch, I can now paste Address book contacts (don't get the beep)

Now Paste doesn't fail on a contact :)

pjrobertson added a commit that referenced this pull request Jul 19, 2011
Updated `stringValue` method to allow for more types (QSEmailAddressType)
@pjrobertson pjrobertson merged commit e9f97e8 into quicksilver:master Jul 19, 2011
@skurfer
Copy link
Member Author

skurfer commented May 2, 2012

That last comment didn’t belong here.

@pjrobertson
Copy link
Member

I haven’t been using the “reset search after X seconds” pref any more

I swear we're both being able to read each other's thoughts... I made a few
changes this morning related to the reset search after x seconds! Nothing
that would fix this, but it appears that, even if your not using this
setting, QS still sets the lastSearch NSDate every time you type a letter.
I want to change it so that it does this only if you have the option
enabled. And - I don't like reading from the preferences every time either.
Still, playing with QSSearchObjectView will probably mean we'll need to
build new interfaces so I might wait until 64 bit :)

On 2 May 2012 14:10, Rob McBroom <
reply@reply.github.com

wrote:

Looks good now from what I can tell.

One thing I noticed while testing: Remember when we were messing with
stringValue a while back and I found that you could see the string value
for any object
just by hitting .? When testing this, I couldn’t get
that to work any more. I found that tabbing away and back or “deleting”
your search (but not clearing the object) made it work again. Turn out it’s
because I haven’t been using the “reset search after X seconds” pref any
more. Just an FYI. I told you it was dumb luck that leads me to these
things. :-) Nothing to fix, just an FYI.

I’ll probably merge this today.


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

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

2 participants