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

Html link parser #316

Merged
merged 4 commits into from May 13, 2011
Merged

Html link parser #316

merged 4 commits into from May 13, 2011

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 12, 2011

Lots of small bug fixes

  1. Credits to @fheckls for weirdness in the perl link extractor. It'd sometimes think a plain old
    <a href="link">some text</a>
    was an image being linked
  2. Icons now load correctly for all URLs (it was just a matter of using if(image.length), since the Link Parser was always returning an image string, just most of the time if was empty @""
  3. Made links that are clickable images have the image as their icon. Test it with, for example by right arrowing into http://google.com/services/
  4. Made links that are clickable images actually have a title (the image URL) (line 86 of QSHTMLLinkParser.m).
  5. Made QS parse links for sites other than .com, .net and .org domains (previously, this wouldn't work)
  6. Made QS parse .php pages
  7. EDIT: Fixed so many things, I forgot about the initial fix! Added the URLs as a QSTextType objects, so you can run things like 'show large type' on them. Fixes bug #302

If I may say so myself, this is freakin awesome!

There are still a few bugs:

  1. Only URLs that end in /, .php, .html, .htm or a tld will be parsed. (see CorePlugin-Info.plist where I've altered in these commits)
    I think what we'd need to do here is compile a list of web page extensions, then do something similar to the check with .tlds. I couldn't be bothered compiling a list now.
  2. If the image that is a link is large-ish, QS will hang whilst downloading the image for the icon. (test with http://qsapp.com/test.html - unless you have a super fast internet connection you'll get a beach ball). I was hoping @HenningJ could fix this as he's been getting all asynchronous lately ;)

P.S. @HenningJ - I've just seen loads of comments IconLoader.m:58 about 'a good place for setting the web search icon'. Was this you? I don't think it's the best place for it as it says in the comment.
What I know:
Search URLs that are added to a 'web search list' catalog entry are imported by the web search plugin. It should deal with the icons.
Search URLs that are added with the default catalog entry (from a remote URL) use the HTMLLinkParser. I've already started implementing a setIcon method there, you can see it commented out.

@philostein
Copy link
Contributor

@philostein philostein commented May 12, 2011

Wow, this looks fantastic, People will be blown away when they can get a visual indication of what they're clicking on sites without even opening a browser. And all the text manipulation power as well. Can I tweet this page?

@skurfer
Copy link
Member

@skurfer skurfer commented May 12, 2011

This is so much better. It fixes most of the things I’ve always hated and reminds me of what’s left to fix.

Made links that are clickable images actually have a title (the image URL) (line 86 of QSHTMLLinkParser.m).

IMO, the image URL should be a fallback for the name. It should prefer things in this order

  1. The title attribute of the image (which should always be present, but won’t be)
  2. The alt attribute of the image
  3. The src attribute of the image (which is what it currently uses)

The good news is that Quicksilver will already do this as is, we just have to get the script to populate the title field correctly. I tried and tried and failed, which is why I went to Python. I have a working script, but like I said, couldn’t get QS to run it. I know a lot more now than I did, so I’ll try again.

Only URLs that end in /, .php, .html, .htm or a tld will be parsed.

There are things I wish it parsed. For instance, http://projects.skurfer.com/QuicksilverPlug-inReference.mdown, but we could spend weeks arguing over what should be included, so I don’t know if it’s worth it. I suppose ideally, it would allow arrowing into any link that pointed to something that had a content-type of text/html, but that would require it to request each child from the web server which would probably be waaaaay too slow.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 12, 2011

IMO, the image URL should be a fallback for the name. It should prefer things in this order...

I think that's probably the right order. Unfortunately I don't know perl enough (at all) to be able to alter the script so that it sets the name or shortcut string to the image title/alt. I think that's the way we should go

As I said, I think the perl script works alright for the time being; we should probably try improving that first, but if the python script is better (got the email Rob) then it should be implemented - I don't feel this warrants the extra work at the time being (for me at least)

There are things I wish it parsed. For instance...

exactly. The list of 'allowed file types' could go on for ever and ever. I suggest I implement a method similar to that for checking tlds (an array) and then if we decide we should add a certain file type we just add it to an array.
I think .mdown would probably be fine to add as a type to parse. The only thing we need to watch out for is QS trying to parse something which it can't and there being a problem (e.g. a .jpg or a .pdf)

I've also noticed one more bug (which I don't plan to fix here, since I don't know enough about it)
The task viewer doesn't display the Downloading Page task in the task viewer (line 109 of QSObject_URLHandling.m)

@skurfer
Copy link
Member

@skurfer skurfer commented May 12, 2011

I got it to pass HTML through my Python script and it handles the image name precedence as described above, but I guess I need to wait until this gets merged before I submit my changes. This looks good to me. I’ll run with it for a few hours and merge it if I don’t see any problems.

skurfer added a commit that referenced this issue May 13, 2011
@skurfer skurfer merged commit c0c1fb5 into quicksilver:master May 13, 2011
@skurfer
Copy link
Member

@skurfer skurfer commented May 13, 2011

Still love it. Merged.

@skurfer
Copy link
Member

@skurfer skurfer commented May 13, 2011

Unfortunately I don't know perl enough (at all) to be able to alter the script so that it sets the name or shortcut string to the image title/alt.

I looked into it and that Perl module doesn’t even make those attributes available, so there’s nothing you could do. I tried other modules, but either they didn’t work, or weren’t available by default on Mac OS X.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 13, 2011

Nice :)

I'm close to getting the web search icon to load reliably for web searches,
so this'll be in another pull - we may need few questions/discussions on it
first though.

I'll post again in the dev discussion incase somebody would like to take a
stab at trying to change the perl script (if it is at all possible - sounds
like it isn't).

On 13 May 2011 12:10, skurfer <
reply@reply.github.com>wrote:

Unfortunately I don't know perl enough (at all) to be able to alter the
script so that it sets the name or shortcut string to the image title/alt.

I looked into it and that Perl module doesnt even make those attributes
available, so theres nothing you could do. I tried other modules, but
either they didnt work, or werent available by default on Mac OS X.

Reply to this email directly or view it on GitHub:
#316 (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

3 participants