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

Opengraph #4215

Closed
wants to merge 19 commits into
base: develop
from

Conversation

Projects
None yet
8 participants
@netom
Copy link
Contributor

netom commented Jun 11, 2013

This branch adds experimental support for parsing and viewing opengraph information.

The feature is already deployed to https://huspora.hu/, and it works OK.

The solution is very similar to the current oembed parser/viewer.

I did not write any tests yet, I hope my time will allow to complete them soon.

end

def fetch_and_save_opengraph_data!
begin

This comment has been minimized.

@jhass

jhass Jun 11, 2013

Member

If the first thing you do in a method is a begin you can just omit it and unindent everything one step ;)

if !response or response.type.nil?
return
end
rescue => e

This comment has been minimized.

@jhass

jhass Jun 11, 2013

Member

No need to create a local variable if you don't use it ;)

Please try to gather the possible exceptions, too open rescue's are rarely a good style.

This comment has been minimized.

@netom

netom Jun 12, 2013

Author Contributor

Ok, I'll try to review what can happen here (that can be handled more intelligently rather just failing silently).

Oh. Here's one: we could propagate network errors, and the worker could retry later. Same for oEmbed though, I admint I copied quite some code blindly from there.

def fetch_and_save_opengraph_data!
begin
response = OpenGraph.new(self.url)
if !response or response.type.nil?

This comment has been minimized.

@jhass

jhass Jun 11, 2013

Member

return if response.blank? || response.type.blank?

@Raven24

This comment has been minimized.

Copy link
Member

Raven24 commented Jun 11, 2013

Very nice, this is certainly a valueable feature. Thank you for your work!

It looks like you are adding the OG data and OEmbed, in case the link somehow provides both ...
Personally, I would rather have OEmbed, and OG as fallback, and handle that transparently behind the scenes, so that we only embed one thing.
Also, I think if we handle OG and OEmbed as one, you can just reuse the o_embed_caches table and store the OG content in the data field.

... but let's see what other people think ;)

return cache if cache.persisted?
cache.fetch_and_save_opengraph_data!
return cache if cache.persisted?
return nil

This comment has been minimized.

@jhass

jhass Jun 11, 2013

Member

Just drop that line, the result of a modifier if with a failing condition is already nil. We could get down to three lines though:

cache = OpenGraphCache.find_or_initialize_by_url(url)
cache.fetch_and_save_opengraph_data! unless cache.persisted?
cache if cache.persisted?
@jhass

This comment has been minimized.

Copy link
Member

jhass commented Jun 11, 2013

I made some style notes on the ruby code, I'm not so much into the Javascript idioms, so somebody else should look over that.

I agree with Florian, we should use OG as a fallback if possible.

@Ruxton

This comment has been minimized.

Copy link
Contributor

Ruxton commented Jun 12, 2013

Not poopooing this feature, love that we have this PR.

But, I'd really like to see this & oEmbed playing nicer together in some kind of base "Embed" object, because they essentially do the same thing. Goto a remote point and ask it how to display. It's something I've been planning in my head since I started looking over oEmbed.

Once that's like that, I have a very good idea for hitting links up that don't have OEmbed or OpenGraph data, a lot like G+ & Facebook do/used to do. You can see a proof of concept I worked on a few years back here in Sniff N Snatch. Sniff N Snatch was a UI to enter text and fetch images and metadata from URL's in that I built for another project of mine.

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 12, 2013

Wow, thank you guys for all the comments on this. I'll improve the code based on your advices.

On oEmbed vs OG: I choose to pasrse & store both information so in the future a js/css developer can decide how to display this information. If you have all info in the db, you can pick to display only oembed, or both oembed & og, or create some nice widget that display oembed, but has some button or something that displays the otherwise hidden and redundant og data.

I too think that at least for now it's perfectly fine to display oEmbed if present, and only display og info if that's the only one available.

I like this solution because of three reasons:

  • it's flexible: there are other ways to provide a preview, like twitter card, or just some other parsing magic like Ruxton's.
  • It offers freedom: you can choose how to display your information only by switching skins (or make it configurable. Or let the user decide, and introduce some user setting for that).
  • It allows parsing multiple links in a post. The current oEmbed and the hopefully-future Opengraph stuff could parse all the links in the post. It's up to the skin to display such posts nicely.

Ok, back to work then. :)

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 12, 2013

Ruxton & base embed object:

Yeah, absolutely. This PR is yust a copy&paste mess really. My only excuse is that I wanted to try the feature ASAP and get feedback from the community. A refactor step would be nice (of not essential) later.

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 12, 2013

Raven24: I'd store the og data in a structured way rather than just dumping some html in a data field. However, considering Ruxton's inheritance suggestion, oembed and og data (and who knows what else ;) ) could live together in a single table. I think we sould plan ahead carefully so we don't lose the flexibility of the current (rather crude) solution.

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 17, 2013

I noticed that og info always apper about a minute later I post. This can be quite annoying, since I'd like to see my post content as quickly as possible. So I changed the StatusMessage's og-pasrser-firing after_create trigger to after_commit, and added and extra condition. This way the og info is usually parsed in a second or two.

Anyway, what is the reason for oembed being parsed asynchronously? (I have pretty good ideas, but I'd like to hear your version). Wouldn't it be nice if oembed/og data would appear immediately after posting (or even in the preview?)

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Jun 17, 2013

We really don't want the request to fail and/or wait for an external API.

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 17, 2013

I totally understand that. I've seen sites crashing because of this.

What about a short timeout (configurable, probably 3-5 seconds)? If that fails or runs out of time, we can always perform_in(1.minute,...) with a greater timeout. Is this acceptable?

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jun 17, 2013

This would allow to have embedded data in the preview, which will be awesome.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Jun 17, 2013

Sounds reasonable but also quite big and maybe out of scope for this PR :)

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 17, 2013

Yeah, definitely. I think I'll just finish this first ;)

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Jun 17, 2013

@Flaburgan Sorry, not directly. The preview is entirely rendered client side, the embeds are entirely fetched server side.

@jhass

View changes

app/helpers/open_graph_helper.rb Outdated
" <br />" +
" </div>" +
"</a>"
return html

This comment has been minimized.

@jhass

jhass Jun 17, 2013

Member

Just skip the local variable and the title local variable is unused too. So all this method needs to be is the string which is then implicitly returned.

@jhass

View changes

app/helpers/open_graph_helper.rb Outdated
end

def oembed_image_tag(cache, prefix)
image_tag(cache.data[prefix + 'url'], cache.options_hash(prefix))

This comment has been minimized.

@jhass

jhass Jun 17, 2013

Member

Really minor thing: Most people prefer "#{prefix}url" over prefix+'url'

@jhass

View changes

app/models/open_graph_cache.rb Outdated
if response.blank? || response.type.blank?
return
end
rescue

This comment has been minimized.

@jhass

jhass Jun 17, 2013

Member

This could be condensed a little bit: return if response.blank? || response.type.blank?. I'd also like to see the rescue catching the expected error classes if that's possible.

This comment has been minimized.

@netom

netom Jun 18, 2013

Author Contributor

That rescue does not really do anything. The opengraph_parser lib already catches errors. Will be removed.

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 23, 2013

Hello everyone,

I kinda finished with this PR, at least with the things I wanted to be in it.

If I can improve it any way before you accept it please let me know.

@Ruxton

This comment has been minimized.

Copy link
Contributor

Ruxton commented Jun 27, 2013

installed this PR on my pod, I think it needs a bit of UI prettying as it looks a bit out of place.


https://diaspora.digitalignition.net/posts/5748

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 27, 2013

I have to agree with that. I'm not very talented as a designer. ;)

@Ruxton

This comment has been minimized.

Copy link
Contributor

Ruxton commented Jun 27, 2013

also, there's nothing in the mobile view for it

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jun 30, 2013

And here's a bug in link parsing: https://www.huspora.hu/posts/499

@Ruxton

This comment has been minimized.

Copy link
Contributor

Ruxton commented Jul 4, 2013

It could/should check the oEmbed stuff before trying to parse because here's what happened to me last night:

  1. posted a link to soundcloud
  2. opengraph parser picked it up, parsed it, stored content
  3. 1 minute later, oEmbed parser parsed it, stored its content and displayed a better looking embed ;)

I don't see a point to getting both if we're going to use the latter.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jul 4, 2013

Yeah, first check if oEmbed can deal with that link, if it can't, then let OpenGraph deal with it ;)

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jul 4, 2013

It means that I have to merge the oEmbed and OpenGraph workers, and the *_cache models too into a single "embed" thingy.

This is something I didn't want to do in the first release, because it would have a lot of LOC.

Since this PR is already quite big and I'm getting more and more comfortable with the D* codebase and Ruby/Rails in general, I think I'm doing it anyway.

This will result in a better user experience and much cleaner code.

Thank you for the suggestions!

Oh, if anyone of you have better visual art skills than me, please post a pic or two on how the OpenGraph embed should look like. I'd be a great help.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jul 4, 2013

I'll wait for your refactor, then merge your code in my pod and do some tests, and I'll tell you if I have suggestions about graphic improvements ;)

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Jul 4, 2013

Nice, thank you!

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Aug 4, 2013

Done.

However, I discovered an advantage of parsing both opengraph and oembed data: mobile view.

Mobile view does not show oembed, but it displays opengraph data just fine. If you think it would be cool to see (for example) preview of youtube video pages in the mobile view (just an image and a description, not the video itself of course), then I can revert the last commit. The oembed parsing can be sped up by using after_commit instead of after_create.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 4, 2013

And then it would show both in the desktop view? Shouldn't we rather fix displaying oEmbed on the mobile view?

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Aug 4, 2013

Yes, and probably yes. :) (I thought that the missing oEmbed support on mobile devices is deliberate.)

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 4, 2013

No, it's not, it just didn't get implemented yet, at least IMO ;)

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Aug 4, 2013

Oh, I see :) Than I think this PR is ready to be merged. (I might do some experiments with oEmbed on mobile and make a PR if someone isn't faster than me ;) )

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 4, 2013

(looking forward to it :) )

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 4, 2013

Just awesome, thank you!

I took the liberty to squash the commits a bit together to make it easier to find the origin/context of a line in case we ever need that. That's why github doesn't show this PR as merged.

@jhass jhass closed this Aug 4, 2013

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 4, 2013

Thank you!

@netom

This comment has been minimized.

Copy link
Contributor Author

netom commented Aug 4, 2013

Ok, thank you!

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 20, 2013

it looks like when a link is made with the markdown syntax, opengraph keeps the ) at the end of the url, this often results in a 404.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 20, 2013

Do not report bugs on closed issues / pull requests. Open new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.