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

fixed and standardized URL creation #370

Merged
merged 7 commits into from Jun 16, 2011
Merged

fixed and standardized URL creation #370

merged 7 commits into from Jun 16, 2011

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jun 9, 2011

I can’t believe this never blew up in our faces before. The initWithURL method would assign QSURLType as the primary type, but there was never anything for that type assigned to the object.

So I fixed that by assigning the URL as QSURLType and as QSTextType. The type assignment was moved outside of initWithURL to its own method so it could be used across the application.

All parts of the application that create QSObjects for URLs now use the same code to do so. This should ensure consistent behavior, eliminate the need for devs to manually (and potentially incorrectly) assign types for URLs, and simplify future updates by only having one place to change.

There are two basic scenarios this takes care of:

  1. To create a QSObject from a URL string, you can call the class method URLObjectWithURL (which internally calls initWithURL which in turn calls the new assignURLTypesWithURL method). This is what most plug-ins are doing.
  2. If you already have a QSObject then find out later that it should be a URL, you can call [self assignURLTypesWithURL:urlString] directly. (This is the case for the HTML parser and string sniffer.)

Other notes:

  • The HTML parser and string sniffer were manually assigning the QSTextType after the fact, which is why URLs from those areas were more useful.
  • initWithURL already had some understanding of what to do with mailto: addresses, so some redundant code in the string sniffer could be removed.

skurfer added 2 commits Jun 9, 2011
…Ls - fix #369

* assigns the URL for QSURLType
* assigns the URL for QSTextType
* initWithURL modified to call this new internal method
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 9, 2011

Good idea. Having a new method will certainly make things easier

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 11, 2011

Yeah, when looking at it, I see that even the mighty initWithURL takes a string, not a URL. We can’t very well rename that one. :) I’ll change some variable names around, but I’m not going to rename any methods. (Since the other methods in that class end with “URL” and take URL strings, I think it’s actually more consistent to leave it.)

* check type instead of parsing the string again (where possible)
* use an FTP icon for FTP addresses
@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 11, 2011

Scope creep! OK, I changed all the URLs-as-strings to urlString. I also noticed some things in the setQuickIconForObject method that could be improved.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 11, 2011

don'tcha just love the creep?!

Looks good :)

But... if you want even more scope creep, I've played with setting primary
types to QSEmailAddressType - to get icons to work you just need to
uncomment line 919 of QSObject.m

It goes on and on! :P

On 11 June 2011 23:58, skurfer <
reply@reply.github.com>wrote:

Scope creep! OK, I changed all the URLs-as-strings to urlString. I also
noticed some things in the setQuickIconForObject method that could be
improved.

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

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 12, 2011

I’ve uncommented that line (and also changed it from QSContactEmailType to QSEmailAddressType). The former was disabled in QSTypes for some reason (and they both refer to the same string anyway).

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 13, 2011

Don’t merge this yet. I found a minor annoyance that I want to fix first. ("http://" shouldn’t be appended to text the user types anywhere except in the value for QSURLType.)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 14, 2011

The smarts for deciding whether or not to prepend “http://“ have been moved to the general application-wide method. This also allows us to set a URL string only for the URL type while preserving the original string for other purposes (like paste, email to, use as remote host, etc.).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 14, 2011

I've pretty sure QS always used to be like this, but what if the URL should be https://?

I don't think it'd make much difference, but whilst we're on the topic it may be worth thinking about. ftp:// as well?

Finally, I remember [NSURL scheme] behaving a bit strangely for me when I was doing the stringSniffing business.
It was the main reason why define:word was turned into a URL — because [NSURL scheme] returned @"define"

Again, I don't think it'll make a difference here, since the assignURLTypeWithURL: method should only be called if we're sure the string is an URL (it shouldn't be the job of this method to check), which has been done in stringSniffing.

All looks good. I'll keep playing

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 14, 2011

I've pretty sure QS always used to be like this, but what if the URL should be https://?

Yes, it typically worked this way. I broke it, but have fixed it now. It only adds http if there’s no scheme, so if it should have https, I imagine it already will and nothing will happen. Same with ftp. The behavior is the same as before, I just took it from sniffString and put it in a central, re-usable place.

Finally, I remember [NSURL scheme] behaving a bit strangely for me when I was doing the stringSniffing business.

Hmm. It wasn’t well documented, but I saw it in sniffString and thought it was appropriate to use. If I think about what will happen in practice, I believe it’s fine (of not desirable) to use it this way. That is to say, if an “invalid” scheme is found, the value stored for the URL type just won’t get “http://” added to it. It’ll err on the side of not screwing with the input. And the value stored for the text type is unaffected either way.

Again, I don't think it'll make a difference here, since the assignURLTypeWithURL: method should only be called if we're sure the string is an URL (it shouldn't be the job of this method to check), which has been done in stringSniffing.

I don’t think we should rely on what sniffString checks since this is meant to be a general purpose method, but from what I described above, I think it’s fine.

One related (very old) issue remains: Something in the clipboard history that looks like a hostname has “http://” prepended to it, so when you paste, you don’t actually get what was copied. I haven’t looked into that yet, but don’t expect it to be part of this request.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 14, 2011

Fine with all of the above. It was mainly just for clarification and to make
sure nothing had been forgotten.

One related (very old) issue remains: Something in the clipboard history
that looks like a hostname has “http://” prepended to it, so when you paste,
you don’t actually get what was copied. I haven’t looked into that yet, but
don’t expect it to be part of this request.

I'm not having this problem?
I'm copying google.com using ⌘C then in the clipboard viewer it's being
displayed as google.com and if I paste I get google.com.

Are you saying that when you paste you get http://google.com ?

*P.S. I'm using a modded clipboard plugin. If the above is correct I'll
check with the old clipboard plugin

On 14 June 2011 13:22, skurfer <
reply@reply.github.com>wrote:

I've pretty sure QS always used to be like this, but what if the URL
should be https://?

Yes, it typically worked this way. I broke it, but have fixed it now. It
only adds http if there’s no scheme, so if it should have https, I imagine
it already will and nothing will happen. Same with ftp. The behavior is the
same as before, I just took it from sniffString and put it in a central,
re-usable place.

Finally, I remember [NSURL scheme] behaving a bit strangely for me when
I was doing the stringSniffing business.

Hmm. It wasn’t well documented, but I saw it in sniffString and thought
it was appropriate to use. If I think about what will happen in practice, I
believe it’s fine (of not desirable) to use it this way. That is to say, if
an “invalid” scheme is found, the value stored for the URL type just won’t
get “http://” added to it. It’ll err on the side of not screwing with
the input. And the value of stored for the text type is unaffected either
way.

Again, I don't think it'll make a difference here, since the
assignURLTypeWithURL: method should only be called if we're sure the string
is an URL (it shouldn't be the job of this method to check), which has been
done in stringSniffing.

I don’t think we should rely on what sniffString checks since this is
meant to be a general purpose method, but from what I described above, I
think it’s fine.

One related (very old) issue remains: Something in the clipboard history
that looks like a hostname has “http://” prepended to it, so when you
paste, you don’t actually get what was copied. I haven’t looked into that
yet, but don’t expect it to be part of this request.

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

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 14, 2011

  • take that back.
    If you immediately hit ⌘V after copying it's fine. But if you drag from the
    clipboard history or get a previous entry it's not. I see the problem.

This is a problem with the putOnPasteboard: method.
I have a branch where this has been heavily modified and hopefully this is
fixed there as well.
Yep, it happens in the writeObjectToPasteboard: method in
QSObject_Pasteboard.m and I've fixed that :)
http://google.com

On 14 June 2011 13:28, Patrick Robertson robertson.patrick@gmail.comwrote:

Fine with all of the above. It was mainly just

http://google.com

for clarification and to make sure nothing had been forgotten.

One related (very old) issue remains: Something in the clipboard history
that looks like a hostname has “http://” prepended to it, so when you
paste, you don’t actually get what was copied. I haven’t looked into that
yet, but don’t expect it to be part of this request.

I'm not having this problem?
I'm copying google.com using ⌘C then in the clipboard viewer it's being
displayed as google.com and if I paste I get google.com.

Are you saying that when you paste you get http://google.com ?

*P.S. I'm using a modded clipboard plugin. If the above is correct I'll
check with the old clipboard plugin

On 14 June 2011 13:22, skurfer <
reply@reply.github.com>wrote:

I've pretty sure QS always used to be like this, but what if the URL
should be https://?

Yes, it typically worked this way. I broke it, but have fixed it now. It
only adds http if there’s no scheme, so if it should have https, I imagine
it already will and nothing will happen. Same with ftp. The behavior is the
same as before, I just took it from sniffString and put it in a central,
re-usable place.

Finally, I remember [NSURL scheme] behaving a bit strangely for me
when I was doing the stringSniffing business.

Hmm. It wasn’t well documented, but I saw it in sniffString and thought
it was appropriate to use. If I think about what will happen in practice, I
believe it’s fine (of not desirable) to use it this way. That is to say, if
an “invalid” scheme is found, the value stored for the URL type just won’t
get “http://” added to it. It’ll err on the side of not screwing with
the input. And the value of stored for the text type is unaffected either
way.

Again, I don't think it'll make a difference here, since the
assignURLTypeWithURL: method should only be called if we're sure the string
is an URL (it shouldn't be the job of this method to check), which has been
done in stringSniffing.

I don’t think we should rely on what sniffString checks since this is
meant to be a general purpose method, but from what I described above, I
think it’s fine.

One related (very old) issue remains: Something in the clipboard history
that looks like a hostname has “http://” prepended to it, so when you
paste, you don’t actually get what was copied. I haven’t looked into that
yet, but don’t expect it to be part of this request.

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

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 14, 2011

Yeah, the most recent thing copied is fine. It’s items in the history that get molested.

I thought I was using your modified clipboard plug-in, but maybe there have been further updates.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 16, 2011

Looks good.

One last thing you forgot to do was change the urlString names in the .h file

Thanks for doing that though, it definitely looks a lot better (to me at least :) )

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 16, 2011

One last thing you forgot to do was change the urlString names in the .h file

Good catch. Done.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 16, 2011

Pssst! You changed the URLObjectWithURL, initWithURL urlString names as well ;)

8523ce6

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jun 16, 2011

I blame it on lack of sleep. :)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 16, 2011

Tsk. Tell me about it! :)

pjrobertson added a commit that referenced this issue Jun 16, 2011
fixed and standardized URL creation
@pjrobertson pjrobertson merged commit 256980f into quicksilver:master Jun 16, 2011
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 6, 2013

Just an FYI, here's what Apple have reported on creating an url as follows: [NSURL urlWithString:example.com:80 ]

Engineering has determined that this issue behaves as intended based on the following information:

With no scheme in your URL string, you are creating a relative reference. The rules for a relative reference (see rfc3986) look like this:

   relative-ref  = relative-part [ "?" query ] [ "#" fragment ]

   relative-part = "//" authority path-abempty
                 / path-absolute
                 / path-noscheme
                 / path-empty

Since "example.com:80" is the authority, you need to start your relative-part string with "//". With that change, this code:

    NSURL *url = [NSURL URLWithString:@"//example.com:80"];
    NSLog(@"Scheme: %@, Host: %@, Port: %@",[url scheme], [url host], [url port]);

displays:

    Scheme: (null), Host: example.com, Port: 80

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