Fix ugly frame#713
Conversation
tsipinakis
left a comment
tsipinakis
left a comment
There was a problem hiding this comment.
Thanks for the PR! These are all code style nitpicks, the code looks good.
| y += settings.frame_width; | ||
| height -= settings.frame_width; | ||
| if (settings.frame_width > 0) | ||
| { |
There was a problem hiding this comment.
[code style] Braces should be in the same line as the statement
| if (first) { | ||
| y += settings.frame_width; | ||
| height -= settings.frame_width; | ||
| if (settings.frame_width > 0) |
There was a problem hiding this comment.
[code style] I'm placing the comment here but it's about the entire PR: The indentation is inconsistent, we're using 8 spaces per indentation level.
| * use a circle for every corner and two overlapping rectangles */ | ||
| int fw = settings.frame_width / 2 + settings.frame_width % 2; | ||
| if (fw < 1) | ||
| fw=1; |
There was a problem hiding this comment.
[code style] Please use a space before and after the operator fw = 1
|
After a closer look I found a bug. There seems to be some artifacts in the corners,
|
|
On closer look this does look like a bug, there are no rounded corners on the window shape, so most likely some of the math is off. |
Codecov Report
@@ Coverage Diff @@
## master #713 +/- ##
==========================================
- Coverage 66.29% 66.00% -0.29%
==========================================
Files 29 29
Lines 5076 5098 +22
==========================================
Hits 3365 3365
- Misses 1711 1733 +22
Continue to review full report at Codecov.
|
|
One problem, previously invisible - when some notifications are stacked (even if e.g. startup low priority notification disappeared with new appeared before), it looks as if they all are rendered, with same effect at antialiased edge as with 0px edge before second commit. You might want to comment out shaping call to see, how it's rendered when in stack. No even need to trigger notifications, just recall one many times. I guess, it assumes that notification sequence is in vertical row, so when they are stacked, it looks even more strange). As for mask painting (if you are about x_win_round_corners()) it is rounded, but somewhat ineffective. Two crossing rectangles, yet circles are full 360deg, while 1/4 would be enough. Though could be more effective in terms of xserver calls, I'm not sure). Yet can't understand, where 0.5,0.5 displacement is set (originall integer coordinates would point to pixel corner instead of its center). |
|
I casually got to making transparent (argv) override-redirect window, like xfce4-notifyd has. But even better. xfce4-notifyd seems not using xshape, only relying to alpha chan. When some compositing manager (compton) tries to make its shadow, with rounded corners there are just gaps, between square shadow and rounded window edge (although this chan affects to rounded corner detection). XShape(ing) affects shadow geometry as well. Edit: Previous assumption, about drawing of each notification in stack was wrong, it was simply some accumulation, due to unset background_pixel attr. |
| 0, height - dia - 1, | ||
| -fw, -fw, | ||
| width - dia - 1, -fw, | ||
| -fw, height - dia - 1, |
There was a problem hiding this comment.
Yet can't understand, where 0.5,0.5 displacement is set (originall integer coordinates would point to pixel corner instead of its center).
This was bothering me since the beginning and I finally figured out why: According to the X11 docs, the coordinates in XFillArc "specify the upper-left corner of the bounding rectangle", so at (0,0) this includes the frame, this change over-compensates and moves the arcs too much, breaking the round corners if the frame is large enough.
I'd only put 1 or 2 pixels of leeway here. The transparency changes are awesome, but they only work if there's a running compositor. Limiting the number of transparent pixels preserves the AA effect but minimizes the black artifacts in the corners.
There isn't even any need to modify the centercoords, simply expanding the radius by a few pixels should be enough.
There was a problem hiding this comment.
It's not center, XDrawArc takes rectangle coords, containing arc. Did not test all frame width, only 1 and 0 (both with and without compositing). Imho not sure, what's better - aliased border or several too dark pixels. Comparing with Xfce4-notifyd it just disabled rounding for no-comp.
Edit: I meaned displacement in cairo :)
|
Oh, and with compositing and compton shadow I got white artifacts due to some transparent pixels, above white areas))) |
|
I was slow, thinking cairo had 0.5,0.5 displacement and not noticing, that there are no strokes))). |
Dropping it in cairo wouldn't help us here though, we need a way to remove them from the window/on the X11 side to fix the shadow artifacts. This seems like that last hurdle to have this working correctly. Do you know if there are any functions in the XShape extension to do that? Edit: I realised you were proposing a different solution here, tuning the cairo strokes to match the window shape, this approach would also work but I haven't worked with this part of cairo extensively enough to know how feasible it is. If you can improve it, please do. |
|
As far as I understand, xshape is 1-bit alpha channel implementation, available without compositing. Applied above all rgb/rgba content. Initial implementation was fine in no-comp, if only paintings were not accumulating (it seems, that window appeared, its content was just a garbage from what was there before, not a true attempt to use background for transparency). I tried to get internal radius more natural, but could not get satisfied with simple radius-fw difference. |
|
One problem (was) ugly xlib drawing quality, even at 1-bit cairo surpasses it. On case less to use xlib drawing. I did not even add that extra pixel for antialiased border, just repeated border painting with given radius against bitmap surface. So that's what I meaned some pixels are cut off, but overall quality is still better. (well, may be if xlib had floating point support it could have better quality) I'm not sure, that xshaping makes sence with compositing, if AA quality is important. Of course, those artifacts with shadow, at screenshot above, would appear only at too bright background (differing from shadow color). |
|
While configuring picom I randomly triggered xfce4-notifyd without compositing. I was certainly wrong about it not using xshape, yet its quality was same as on my last shots above. Just to not mimic xfce policy about config options, there could be option to force xshape in compositing mode if anyone likes shadows under notifications. But I'm not sure it's necessary. Same picom (former compton, so blamed for ugly shadows with rounded argb windows) now has option for custom shader, could be used for custom shado. Yet there is full-shadow toggle for winlist block (though not a place for arbitrary rules like for "shadow-exclude"). |
|
From my tests this looks perfect now! There are still some artifacts but they're barely visible IMO.
From a quick test with compton, the shadows were rounded and looked like they matched the window. So I'm not sure if there's anything wrong there. Edit: I'll try to do the final review on this over the weekend. |
|
While learning code, I noticed, that all drawing is done to image surface. What if it were done to recording surf as some way to draw right to xlib surf? When I tried so for self, I did not notice any visible difference. Of I'm aware that not all cairo ops work via backend, as case with opengl backend (remebering some notes about gradients). |
| r2 = r * h / (h + (w - r) * 2); // w >= r | ||
|
|
||
| radius_int = (r > w) ? r1 : (r / 2 < r2) ? r / 2 : r2; | ||
| radius_int /= s; |
There was a problem hiding this comment.
I've been trying to decipher this block, some things I can't seem to figure out:
- What's the point of
s? In a standard, 64 bit, OS this will evaluate to 8 (4*8/4), my best guess is that it's for increasing the accuracy of the calculations but why8/4? And why not just use floating points? - Why is
heightinvolved inr2? In fact, I can't figure out how you came up with these formulas.
|
r1 - first supposed to change linearly as well from outer radius to some value for (width == radius), which I indeed got just by taste :) . However, this factor seemed to make less impact with high (radius/width) ratios, so I made it than slightly non linear... more exactly, with linear dependency on another factor. r2 - I made it keeping same ration to outer radius as int height to (outer height outer radius). But since its bigger than r1 at Btw I thought even about different drawing way:
|
I still don't get what's the purpose, to increase accuracy? Then why not use floating points? I couldn't find anything wrong functionality wise, so what's left here is: |
tsipinakis
left a comment
tsipinakis
left a comment
There was a problem hiding this comment.
I wanted to show something I did in lomath to explain it better, when need comes. Got system freeze (can't remember reason), requiring to reboot.
I just used geogebra to plot these and understand exactly what's going on in the frame, I've been testing this for the past week with different configurations and I can't find anything wrong with it as-is so I'd say let's merge this as-is and see if it needs further tweaking later on.
I've added some final code style comments, please resolve them first thought :)
| * Simple r-w is not enough for too small r/w ratio. | ||
| * simplifications: r/2 == r - w + w*w / (r * 2) with (w == r) | ||
| **/ | ||
| { const int s = 2 << (sizeof(int) * 8 / 4); // Integer precision scaler |
There was a problem hiding this comment.
[code style] There should be a newline after the brace
{
// code here
}
| if (settings.corner_radius != 0) | ||
| sprintf(astr, "_NET_WM_CM_S%i", win->cur_screen); | ||
| cm_sel = XInternAtom(xctx.dpy, astr, true); | ||
| if (settings.corner_radius != 0 && XGetSelectionOwner(xctx.dpy, cm_sel) == None) |
There was a problem hiding this comment.
This should probably be a new function bool has_compositing() or something similar, keep things simple
| scr_n = DefaultScreen(xctx.dpy); | ||
| root = RootWindow(xctx.dpy, scr_n); | ||
| if (XMatchVisualInfo(xctx.dpy, scr_n, 32, TrueColor, &vi)) | ||
| { |
There was a problem hiding this comment.
[code style] Braces should be in the same line as the if/else statements
|
I decided to go a bit further than has_compositing(). Added few more functions (from my previous changes) and renamed existing one. Yet could be nice to reuse draw_rounded_rect() in xshaping, to not duplicate same func. |
It's all job is to feel proper X11 shape.
Codecov Report
@@ Coverage Diff @@
## master #713 +/- ##
==========================================
- Coverage 66.29% 65.95% -0.34%
==========================================
Files 29 29
Lines 5076 5102 +26
==========================================
Hits 3365 3365
- Misses 1711 1737 +26
Continue to review full report at Codecov.
|
|
What if move draw_rounded_rect() from degrees to direct M_PI / M_PI_2 use since they are exactly half/quater parts? |
|
Did not mean to push full ARGB support here, but noticed too late loss for one changed line, which went to style fix (gg, blindly selected entire block). |
|
Completed ARGB with proper frame stroke limit, so that it doesn't paint behind background. |
|
Thanks for also working on RGBA support! It's been a widely requested feature. Though can you split it into a new PR? It's easier to review and reference in the future when each PR has a smaller scope. Looks like the commits are fairly independent, so all it should take is cherry pick the last 4 commits to a new branch and rewrite the latest style commit.
That's a bit excessive in my opinion, |
|
Hoped to avoid forced push). About color coding - I tried to remember, which other apps I ever used need alpha first in config (configured by xresources). Rofi accepts as Yet dunst docs claim format support with single digit per channel |
nick87720z
force-pushed
the
fix-ugly-frame
branch
from
482b7f6 to
2de1603
Compare
|
Hope PR will not suffer from forced push |
|
@nick87720z Feel free to submit the RGBA patches as a new PR whenever you can - looking forward to having these merged as well |
4 participants
Uh oh!
There was an error while loading. Please reload this page.