Made QS only change text to URLs if there's a valid TLD on the end #219

Closed
wants to merge 5 commits into
from

Projects

None yet

5 participants

@pjrobertson
Member

Fix for issue #214.

Uses an array of TLDs from http://data.iana.org/TLD/tlds-alpha-by-domain.txt and tests to see if the text being entered in the 1st pane contains a TLD at the end

@HenningJ
Member

Wow. Don't you think you're going a little bit overboard with this many comments? Or is this just for my benefit when checking on your code?

Apart from that:
I think your improvement is good. But I have a question about the code that was there before...
Why would an URL have 4 components (meaning three . in the host)?

Also, you might want to create the TLD array somewhere else, so it isn't created each time this method is called. I don't think it will matter much because the method isn't called THAT often, but imho it would be better anyway

@burningTyger

I like that comment style. More people should comment more code. It helps to read.

@pjrobertson
Member

The comments were more just to say what's what as I read through the code.
Personally, I don't think you can ever have too many comments. I have come
from a background of "you'll get a 0 for your code unless you include
comments and fail the unit!"

I have no idea why it was set to components == 4.
I've thought about it, NSLoged the components and everything, but I'm still
not sure. All I can say is that it would return true if there was one dot
with two words either side of it. Maybe it counts the nil at the end of the
array? I dunno!

There are a few typos in the comments, so I can tidy it up and add another
commit to this, then if you're happy (and anyone else) they can merge :)

On 20 April 2011 22:56, burningTyger <
reply@reply.github.com>wrote:

I like that comment style. More people should comment more code. It helps
to read.

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

@HenningJ
Member

Don't get me wrong, I do like comments. :-)
But I think they should be more about why something is done, not about how something is done. Because I can read the how in the code myself. Unless (of course) it's a really complicated how.

So, a comment like

// If there's a .
if ([stringValue rangeOfString:@"."] .location != NSNotFound) {

imho only clutters the code with useless lines. Instead it should be something like

// a mail address or url have to contain at least one ., so if it exists, do some more checking on that

The same goes for

// If the string is prefixed with / or ~ it's a file path
if ([stringValue hasPrefix:@"/"] || [stringValue hasPrefix:@"~"]) {

I can see its checking for the / and ~ prefixes, so all I need is that this means its a file path.

But maybe that's just me.

Also, remove the NSLogs :-)

@pjrobertson
Member

Yeah I'd forgotten about the logs.

I've cut things back a little on the comments front. You're right with no
need to say 'if such and such', unless it's really complicated.

In my eyes, you should really consider the code cluttering up the comments.
If you want to read some code, you just read the comments like a book (I
guess that's what applescript is trying to be). If you actually want to
change stuff then you change the code :)

I know most of my comments are pretty basic. But it's going to help new
people who've never seen code before get started. I started coding with
Quicksilver and it was just so ridiculously hard!

I'll add another commit with an updated check for 'define:word' and
':something' (Apple still thinks that's a URL) e.g. :o would be a word.

Where would you suggest I stick the array? I had a think about it, but
couldn't think of anywhere good. As a class variable?

On 20 April 2011 23:18, HenningJ <
reply@reply.github.com>wrote:

Don't get me wrong, I do like comments. :-)
But I think they should be more about why something is done, not about how
something is done. Because I can read the how in the code myself. Unless (of
course) it's a really complicated how.

So, a comment like

// If there's a .
if ([stringValue rangeOfString:@"."] .location != NSNotFound) {

imho only clutters the code with useless lines. Instead it should be
something like

// a mail address or url have to contain at least one ., so if it
exists, do some more checking on that

The same goes for

// If the string is prefixed with / or ~ it's a file path
if ([stringValue hasPrefix:@"/"] || [stringValue hasPrefix:@"~"]) {

I can see its checking for the / and ~ prefixes, so all I need is that this
means its a file path.

But maybe that's just me.

Also, remove the NSLogs :-)

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

@HenningJ
Member

In my eyes, you should really consider the code cluttering up the comments.
If you want to read some code, you just read the comments like a book (I
guess that's what applescript is trying to be). If you actually want to
change stuff then you change the code :)

Well, I mostly read the code like a book. ;-)
Also, when you have to many comments, the comments and the code tend
to diverge with time. You do a quick fix in the code, but forget to
adjust the comment.
I always find it hard to decide the correct amount of comments. And
sadly, most of the time I go for to few comments.

Where would you suggest I stick the array? I had a think about it, but
couldn't think of anywhere good. As a class variable?

I was thinking something like that:
http://stackoverflow.com/questions/1059708/defining-a-constant-in-objective-c/1060075#1060075

@skurfer
Member
skurfer commented Apr 20, 2011

I’m testing this now. No issues so far (and it fixes that thing with the clipboard somehow).

@pjrobertson
Member

Somehow...

There's one more commit to make.

I've added

static NSArray *tld; outside the class method definitions in the .m file

Then within @implementation QSObject
I've added an initialize method to create the tld array.
Should only be called once now

Check out the commit - I'm scared of memory stuff :/

On 21 April 2011 00:01, skurfer <
reply@reply.github.com>wrote:

Im testing this now. No issues so far (and it fixes that thing with the
clipboard somehow).

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

@HenningJ
Member

Well...it should only be called once...and once for every object the inherits from this object. So, i think you should do

if (tld == nil) {
    tld = [[NSArray arrayWithObjects
}

instead.

But I'm also a little scared of memory stuff ;-)

@skurfer
Member
skurfer commented Apr 20, 2011

I don’t see how it could possibly be related, but while testing with this branch, I noticed some icons were blown-up from tiny versions and were blocky and blurry as a result. (Specifically, the Ticket Viewer application and any file owned by TextMate.)

I also lost the ability to use Paste or Large Type on “Current Web Page”. (OK, I can see how that might be related.)

I’ll build it again to make sure, but don’t merge it just yet.

@pjrobertson
Member

Nope. I think it's my commits. Also, .pdf icons aren't changing to the
actual file's icon.

I'll look at why this might be now/tomorrow morning.

On 21 April 2011 00:37, skurfer <
reply@reply.github.com>wrote:

I dont see how it could possibly be related, but while testing with this
branch, I noticed some icons were blown-up from tiny versions and were
blocky and blurry as a result. (Specifically, the Ticket Viewer application
and any file owned by TextMate.)

I also lost the ability to use Paste o Large Type on Current Web Page.
(OK, I can see how that might be related.)

Ill build it again to make sure, but dont merge it just yet.

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

@pjrobertson
Member

Removing the initialize method and tdl constant array fixes it.

I thought it might be because the intialize method should call

[super initialize]

but that doesn't work.

Solutions are:
a) figure out why this is happening
b) stick the NSArray back where it was (called every time)

On 21 April 2011 01:00, Patrick Robertson robertson.patrick@gmail.comwrote:

Nope. I think it's my commits. Also, .pdf icons aren't changing to the
actual file's icon.

I'll look at why this might be now/tomorrow morning.

On 21 April 2011 00:37, skurfer <
reply@reply.github.com>wrote:

I dont see how it could possibly be related, but while testing with this
branch, I noticed some icons were blown-up from tiny versions and were
blocky and blurry as a result. (Specifically, the Ticket Viewer application
and any file owned by TextMate.)

I also lost the ability to use Paste o Large Type on Current Web Page.
(OK, I can see how that might be related.)

Ill build it again to make sure, but dont merge it just yet.

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

@skurfer
Member
skurfer commented Apr 20, 2011

I was trying to solve a similar problem (avoid creating the same dictionary over and over) in the Remote Hosts plug-in. I have no idea if I did it correctly, but here's what I ended up with.

In the @interface:

// array to store details about which actions work with what
NSDictionary *actionCapabilities;
// keep a list of all known actions
NSArray *actionList;

In the @implementation:

- (id)init {
    if ((self = [super init])) {
        // define what each action will work with (for validation)
        actionCapabilities = [[NSDictionary dictionaryWithObjectsAndKeys:
            // details omitted
        ] retain];
        // store known actions
        actionList = [[actionCapabilities allKeys] retain];
    }
    return self;
}
@pjrobertson
Member

I thought I read something like that back in the day when I started reading
Obj-C books.
It definitely looks more familiar than what I had put in the code a few
hours back

Unfortunately being the other side of the world I don't have my cocoa books
with me.
If anybody has these, it was in one of them:
http://www.amazon.com/dp/0321503619/
http://www.amazon.com/Programming-Objective-C-2-0-Stephen-Kochan/dp/0321566157

On 21 April 2011 01:26, skurfer <
reply@reply.github.com>wrote:

I was trying to solve a similar problem (avoid creating the same dictionary
over and over) in the Remote Hosts plug-in. I have no idea if I did it
correctly, but here's what I ended up with.

In the @interface:

// array to store details about which actions work with what
NSDictionary *actionCapabilities;
// keep a list of all known actions
NSArray *actionList;

In the @implementation:

  • (id)init {
    if ((self = [super init])) {
    // define what each action will work with (for validation)
    actionCapabilities = [[NSDictionary
    dictionaryWithObjectsAndKeys:
    // details omitted
    ] retain];
    // store known actions
    actionList = [[actionCapabilities allKeys] retain];
    }
    return self;
    }

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

@pjrobertson
Member

Did the build also make all your triggers go (null)? It may have been that
or just me messing about.

On 21 April 2011 01:31, Patrick Robertson robertson.patrick@gmail.comwrote:

I thought I read something like that back in the day when I started reading
Obj-C books.
It definitely looks more familiar than what I had put in the code a few
hours back

Unfortunately being the other side of the world I don't have my cocoa books
with me.
If anybody has these, it was in one of them:
http://www.amazon.com/dp/0321503619/

http://www.amazon.com/Programming-Objective-C-2-0-Stephen-Kochan/dp/0321566157

On 21 April 2011 01:26, skurfer <
reply@reply.github.com>wrote:

I was trying to solve a similar problem (avoid creating the same
dictionary over and over) in the Remote Hosts plug-in. I have no idea if I
did it correctly, but here's what I ended up with.

In the @interface:

// array to store details about which actions work with what
NSDictionary *actionCapabilities;
// keep a list of all known actions
NSArray *actionList;

In the @implementation:

  • (id)init {
    if ((self = [super init])) {
    // define what each action will work with (for validation)
    actionCapabilities = [[NSDictionary
    dictionaryWithObjectsAndKeys:
    // details omitted
    ] retain];
    // store known actions
    actionList = [[actionCapabilities allKeys] retain];
    }
    return self;
    }

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

@skurfer
Member
skurfer commented Apr 20, 2011

Yes, triggers were all jacked up with missing icons and “(null)” everywhere.

@pjrobertson
Member

Hmm.... I can't get this to work.

Doing what Rob says and sticking a
NSArray *tldArray;
in the

@interface QSObject (StringHandling)

gives an error - presumably since it's not a class (the brackets)
Placing it in an actual class

@interface QSStringObjectHandler : NSObject

means it can't be accessed. I need my books :/

@fheckl
Contributor
fheckl commented Apr 21, 2011

Maybe the damaged behavior is caused by StringHandling only being a category so basically we're dealing with QSObject which has its own initialize method already. I still have to confirm by a small test, but maybe the ObjC runtime does not call every initialize method then.

Anyway why don't you put both the declaration and initialization of the tld array into the sniffString method itself right before it's needed. I know that this may look strange when you're not used to it, but C supports function static memory, so it goes like this:

-(void)sniffString {
 static NSArray *tld = nil;
 ...
 if (tld == nil) {
  tld = [[NSArray ...
 }
 ...
}

At least that works for me and you have the initialization only once.

@pjrobertson pjrobertson Made tldArray a static NSArray. Only gets set once
I've checked this with NSLog and it only gets set once.
4d8147e
@pjrobertson
Member

OK. So I think I've got this sorted.

The one difference I made to what fheckl said was to not set

 static NSArray *tldArray = nil;

at the start of the sniffString method. Surely ever time QS sniffs the string it would reset it, and tld would always be nil?
I set it in the .h file - it gives a warning saying it's unused (I guess it's not necessarily used) but it's not too bad.

I'm not sure whether this will last for ever though, it should get wiped from memory at some point?

Secondly,
I'm still getting exceptions (in the Console) about there being no specified encoding.
I've tried setting the encoding to everything I can think of;
NSUTF8Encoding, NSASCIIEncoding etc. etc. and it still gives the same logs which is really strange.
I've even tried setting it as a number (e.g. 4 which corresponds to NSUTF8Encoding)

Good places to read about encoding:

http://www.cocoadev.com/index.pl?CharacterEncoding
http://www.joelonsoftware.com/articles/Unicode.html

@pjrobertson
Member

Added an extra commit

@pjrobertson
Member

Woops this was means to be closed.
Everything here is included in #238

@pjrobertson
Owner

This commit did horrible things to QS so the changes made here were quickly removed and are irrelevant

@dichen001 dichen001 referenced this pull request in dichen001/Paper-Reading Jun 28, 2016
Open

Summary of the 20 issues in Herbsleb's 2014 FSE paper. #6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment