feat: Add link preview#1259
Conversation
bpavuk
left a comment
bpavuk
left a comment
There was a problem hiding this comment.
I don't have expertise to evaluate most of C++ code, but some of it does rub me the wrong way, aside from C++ being biblically verbose in general
| <file>migrations/002_add_url_metadata.sql</file> | ||
| <file>migrations/003_add_og_image.sql</file> |
There was a problem hiding this comment.
nitpick: could we squash them?
| Image { | ||
| visible: root.host.detailOgImageUrl !== "" | ||
| source: root.host.detailOgImageUrl | ||
| Layout.fillWidth: true | ||
| Layout.fillHeight: true | ||
| Layout.margins: 10 | ||
| fillMode: Image.PreserveAspectFit | ||
| sourceSize.width: 500 | ||
| cache: true | ||
| asynchronous: true | ||
| } |
There was a problem hiding this comment.
lightly rounding corners would be beneficial for consistency across Vicinae
| #include <libxml/HTMLparser.h> | ||
| #include <libxml/tree.h> |
There was a problem hiding this comment.
unless there is an overwhelmingly massive benefit to interfacing with a C library directly (which I don't see the code below utilizing) I strongly encourage you to have a look at libxml++. idiomatic C++ is much easier to read than "C with classes"-style code, and perhaps we could apply XPath queries here.
| xmlFree(prop); | ||
| xmlFree(name); | ||
| xmlFree(content); |
| auto *text = xmlNodeGetContent(cur); | ||
| if (text) { | ||
| result.ogTitle = QString::fromUtf8(reinterpret_cast<const char *>(text)); | ||
| xmlFree(text); |
| }; | ||
|
|
||
| walk(xmlDocGetRootElement(doc)); | ||
| xmlFreeDoc(doc); |
There was a problem hiding this comment.
guess no RAII is fine for now, eventually we will ditch libxml or just wrap it in a C++ class
There was a problem hiding this comment.
eventually? why not do it good now?
There was a problem hiding this comment.
because it's already in use in other places like this, and I don't yet know what to replace it with. needs some consideration. I don't want to block this PR just because of that
I still have to review this one anyways
|
Sorry for delay, will fix in a week (on vacation right now) |
SiriusCrain
force-pushed
the
feat/add-link-preview
branch
from
80d2117 to
b43415d
Compare
SiriusCrain
force-pushed
the
feat/add-link-preview
branch
from
b43415d to
f7bb33c
Compare
|
@aurelleb updated |
|
if this is getting merged, make it optional/opt-out, I don't think its very secure to fetch EVERY link you copy. |
4 participants
Closes #1256
slopped with claude
Updated design: