Add basic border-radius support#629
Conversation
| if ( xx<clip.left || xx>=clip.right ) { | ||
| // OOB, don't plot it! | ||
| continue; | ||
| } |
There was a problem hiding this comment.
All of these guards should be obsolete due to being precomputed now.
| imp_bit_content, | ||
| imp_bit_cr_hint | ||
| imp_bit_cr_hint, | ||
| // Border radius (per-corner, horizontal and vertical radii) | ||
| imp_bit_border_radius_tl_h, | ||
| imp_bit_border_radius_tl_v, | ||
| imp_bit_border_radius_tr_h, | ||
| imp_bit_border_radius_tr_v, | ||
| imp_bit_border_radius_br_h, | ||
| imp_bit_border_radius_br_v, | ||
| imp_bit_border_radius_bl_h, | ||
| imp_bit_border_radius_bl_v |
There was a problem hiding this comment.
Maybe move the new ones near the other imp_bit_border*, I'd like imp_bit_cr_hint to keep being the last one.
| // border-radius | ||
| border_radius_h[0] = css_length_t(css_val_unspecified, 0); | ||
| border_radius_h[1] = css_length_t(css_val_unspecified, 0); | ||
| border_radius_h[2] = css_length_t(css_val_unspecified, 0); |
There was a problem hiding this comment.
The initiall value seems to be 0 : https://developer.mozilla.org/en-US/docs/Web/CSS/border-top-left-radius.
which would be css_length_t(css_val_screen_px, 0);, which is the default value for a css_lenght_t, so these may not ne needed (see the comment at the top of this block:
// css_length_t fields are initialized by css_length_tag()
// to (css_val_screen_px, 0)
// The following ones should not.It is also what is used later/below in the changes when handling "initial":
if ( g == css_g_initial ) {
buf<<(lUInt32) css_val_screen_px; // type
buf<<(lUInt32) 0; // valueUNLESS this css_val_unspecified is used as a flag to skip the border radius computing stuff.
Anyway, these 2 snippets should use the same value (I checked a few of my if ( g >=0 ) and they are consistent with the stuff in lvstyles.h.
| const int byteindex = (xx >> 3); | ||
| const int bitindex = ((xx & 7)); | ||
| const lUInt8 mask = 0x80 >> (bitindex); | ||
| dcl = dcl << (7-bitindex); | ||
| row[ byteindex ] = (lUInt8)((row[ byteindex ] & (~mask)) | dcl); | ||
| const int byteindex = (x >> 3); | ||
| const int bit = 7 - (x & 7); | ||
| const lUInt8 mask = (lUInt8)(0x01 << bit); | ||
| const lUInt8 dclbit = dcl ? mask : 0x00; | ||
| row[ byteindex ] = (lUInt8)((row[ byteindex ] & (~mask)) | dclbit); |
There was a problem hiding this comment.
Many changes of variable names, here and above (x => ix). May be you could ask the AI to "avoid changes in existing variable names to keep the diff small" ?)
I can't (don't want to and not competent enough :)) the drawing/math code :/
There was a problem hiding this comment.
This is what I said in my comment: the reason it was named xx went away. Only x remains. But yes, the diff could be made slightly smaller by normalizing it to a somewhat confusing xx (because where's x then).
| "border-top-left-radius", | ||
| "border-top-right-radius", | ||
| "border-bottom-right-radius", | ||
| "border-bottom-left-radius", | ||
| "border-radius", | ||
| "border-collapse", | ||
| "border-spacing", |
There was a problem hiding this comment.
You could move them nearer the other normal cssd_border_* above - border-collapse and border-spacing are more related to <table> than generic borders.
The parsing just below has been placed in the correct place for me, after the generic cssd_border_* and before cssd_background_image.
| } | ||
|
|
||
| // Compute per-corner border radii in pixels, applying CSS scaling rules | ||
| // Order of corners: 0=TL, 1=TR, 2=BR, 3=BL | ||
| static void computeBorderRadiiPx(ldomNode * node, css_style_rec_t * style, int box_w, int box_h, int rx[4], int ry[4]) { |
There was a problem hiding this comment.
AI bot is making his home in my living room :) You should add to the prompt "try to guess and avoid @poire-z 's anal retentive nitpicks from his past reviews" !
These helpers do not need to be at the start of the file, between unrelated stuff. There should be near where they are used, just before void DrawBorder I think.
Are these comments yours or the bot's ?
Not reviewing all these maths. I guess that if emited by the AI, there's no risk of typos and indices update forgotten from copying&pasting from the first implemented TL into the 3 other corners.
There was a problem hiding this comment.
The risk from AI paraphrasing the same thing you wrote elsewhere is both smaller and much larger (it probably won't typo, but it might add weird stuff). But when it's incorrect, the border looks visually very wrong.
The comments are nice when hovering over where a function is used to see what it does. The order of corners stuff is perhaps slightly redundant but border-radius is quite unique and everything else border goes top, right, bottom, left. It's something I tend to have to remind myself of (mainly whether it starts TL or TR). Was there something specific that bothered you about them?
There was a problem hiding this comment.
No, nothing bothering. I just wondered if the comments were yours or rewritten by you (I haven't even read them :/).
And generally, how much (if any) tweaks by your fingers to the AI output ?
I'd just like this whole section to be moved near void DrawBorder.
| if (h.type != css_val_unspecified || h.value != 0) | ||
| rx[i] = lengthToPx(node, h, box_w); | ||
| if (v.type != css_val_unspecified || v.value != 0) | ||
| ry[i] = lengthToPx(node, v, box_h); |
There was a problem hiding this comment.
Here, if we get (css_val_screen_px,0) (that we get with initial), we would go compute lenghToPx() while it's not needed ?
| int rx[4]={0,0,0,0}, ry[4]={0,0,0,0}; | ||
| computeBorderRadiiPx(enode, style.get(), fmt.getWidth(), fmt.getHeight(), rx, ry); | ||
| if (rx[0]||rx[1]||rx[2]||rx[3]||ry[0]||ry[1]||ry[2]||ry[3]) { |
There was a problem hiding this comment.
This computeBorderRadiiPx() is called here for any block element, so it ought to be optimized.
| if (restore_rounded) { | ||
| // Restore original rounded state | ||
| dei->rounded_clip_active = saved_active; | ||
| dei->rounded_clip_rect = saved_rounded_rect; | ||
| for (int i=0;i<4;i++){ dei->rounded_rx[i]=saved_rx[i]; dei->rounded_ry[i]=saved_ry[i]; } |
There was a problem hiding this comment.
These (defined above:
bool restore_rounded = false;
lvRect saved_rounded_rect;
int saved_rx[4] = {0,0,0,0};
int saved_ry[4] = {0,0,0,0};
bool saved_active = false;
if (dei) {
// Save prev state
saved_active = dei->rounded_clip_active;
saved_rounded_rect = dei->rounded_clip_rect;
for (int i=0;i<4;i++){ saved_rx[i]=dei->rounded_rx[i]; saved_ry[i]=dei->rounded_ry[i]; }) could have better names and similar prefixes, restore_xyz matching saved_xyy, ie.
if (restore_rounded_clip) {
// Restore original rounded state
dei->rounded_clip_active = saved_rounded_clip_active;
dei->rounded_clip_rect = saved_rounded_clip_rect;
for (int i=0;i<4;i++){ dei->rounded_clip_rx[i]=saved_rounded_clip_rx[i]; ...(just wrote that quickly, may be wrong).
| computeBorderRadiiPx(enode, style.get(), fmt.getWidth(), fmt.getHeight(), rx, ry); | ||
| if (rx[0]||rx[1]||rx[2]||rx[3]||ry[0]||ry[1]||ry[2]||ry[3]) |
There was a problem hiding this comment.
computeBorderRadiiPx() already does the rx[0]||rx[1]..., so it could return a bool (has_rounded_border) so we don't need to redo-it.
|
Not fun having to review some AI output :/ but as you have been doing this to improve @offset-torque 's weak trust and love for crengine: koreader/koreader.github.io#59 (comment)
koreader/koreader.github.io#59 (comment)
I'm fine with that :) |
|
Pinging @NiLuJe and @benoit-pierre for a quick review of the AI computing/drawing code (you both have hacked images/drawing stuff in the past, so you'll probably have a better feel for its quality than me). |
Most of your comments are directed at me, not to worry.
This you guessed correctly. |
My intention was not throwing shade on crengine, really :) I think KOReader does its job very well. EPUBs I read are generally very simple and with all the customization options and style tweaks they render perfectly. This lack of css features never inconvenienced me when reading EPUBs. And fancy formatted books/scientific articles already come in PDF format. I was specifically talking about the user guide there, which is a more demanding document - closer to a CS book instead of a novel with all the infoboxes, menu breadcrumbs, links... So the guide is more usable as a HTML document or a PDF instead of a downgraded EPUB especially considering the monochrome color restriction. And this is not KOReader's fault of course. |
|
@Frenzie : tired of whiping your AI slave on this PR ? :) |
|
I'll probably pick it back up sometime. I may have made a mistake when I told it to copy the logic for the bg image since that made it all much larger. :-) |
See #628 for more details.
Basic test case: border-radius.html
What works:
What doesn't work as nicely yet:
For example here it is with dotted (which looks like squares instead of dots but that's a separate pre-existing issue).
This change is