Add options to draw indicator as polygon - #258
Conversation
|
This is SO COOL. I'll review and merge soon! |
58198de to
d19b9bd
Compare
Raymo111
left a comment
There was a problem hiding this comment.
Great work! This was a static review, just a couple of code points to fix. Once they're updated I'll do a full test and then approve+merge!
a4a6bcf to
8c22426
Compare
8c22426 to
4920faf
Compare
Raymo111
left a comment
There was a problem hiding this comment.
Good changes, couple more. Please add more commits instead of amending so it's easier for me to review, thanks!
| "--bar-total-width" | ||
| # Polygon indicator | ||
| "--polygon-sides" | ||
| "--polygon-offset" |
|
|
||
| .TP | ||
| .B \-\-polygon\-sides | ||
| Draw the indicator as a regular polygon instead of a circle. |
There was a problem hiding this comment.
Should document the required arg (number of sides, in addition to current info here)
| if (sscanf(arg, "%d", &polygon_sides) != 1) | ||
| errx(EXIT_FAILURE, "polygon-sides must be a number\n"); | ||
| if (polygon_sides < 3) | ||
| errx(EXIT_FAILURE, "polygon-sides must be greater then 2 or 0\n"); |
| case 405: | ||
| arg = optarg; | ||
| if (sscanf(arg, "%lf", &polygon_rotation) != 1) | ||
| errx(1, "polygon-rotation must be a number\n"); |
| case 406: | ||
| arg = optarg; | ||
| if (sscanf(arg, "%d", &polygon_highlight) != 1) | ||
| errx(1, "polygon-highlight must be a number\n"); |
Raymo111
left a comment
There was a problem hiding this comment.
Review — Request changes
Clean, self-contained feature and the screencaps look great. A few real defects plus the merge conflict to clear first. Most severe first:
Merge blocker — rebase needed. GitHub reports this as CONFLICTING / DIRTY; the branch is stale against master and must be rebased before it can merge.
1. [Medium] --polygon-sides=0 is fatally rejected despite being the documented "draw a circle" value. The guard is if (polygon_sides < 3) errx(...) with message "must be greater then 2 or 0", but 0 < 3 triggers the fatal errx, so explicitly passing --polygon-sides=0 kills the process. Should be if (polygon_sides != 0 && polygon_sides < 3).
2. [Medium] bash completion advertises a flag that doesn't exist. i3lock-bash adds --polygon-offset, but the actual getopt long option (and the i3lock-zsh / i3lock.1 docs) is --polygon-rotation. The completion suggests a non-existent option and omits the real one. Rename to --polygon-rotation. (The PR description uses the old name too.)
3. [Low] draw_polygon() truncates vertex coordinates to int — int x = radius * cos(theta) — introducing up to ~1px asymmetry per vertex, worst at small --radius. Cairo takes doubles everywhere else; use double.
4. [Low, visual] Polygon ring isn't path-closed. It returns to the first vertex with an explicit cairo_line_to rather than cairo_close_path; fill auto-closes fine, but the stroked ring renders start/end as two line caps instead of a mitered join, leaving a small notch at vertex 0. Use cairo_close_path.
Nits: inconsistent error handling — case 404 uses errx(EXIT_FAILURE, ...), 405/406 use errx(1, ...), and out-of-range --polygon-highlight warns-and-continues while out-of-range --polygon-sides is fatal; pick one convention. Typo "greater then 2" (also in the man page) → "than". The counterclockwise -input_position % polygon_sides yields a negative index but is harmless (fed through periodic cos/sin) — a comment would help.
Recommendation: Request changes — rebase to clear the conflict, then fix the --polygon-sides=0 rejection and the --polygon-offset→--polygon-rotation completion mismatch (both real defects); the double-precision and close_path items are quick polish. Worth landing after that.
Description
I needed a hexagon unlock indicator for my config, thought it turned out pretty neat and was worth sharing. This adds three new config options.
--polygon-sides=nenables drawing a regular polygon instead of a circle for the indicator. It defaults to0, which draws a circle, and can be any integer greater then2.--polygon-offset=dspecifies a rotation in degrees to apply to the polygon. At zero offset the first vertex is at(ind_x+radius,ind_y).--polygon-highlight={0,1,2}defines the highlight mode while typing.0highlights a random edge on each keystroke.1highlights sequential edges walking clockwise around the indicator.2walks counter clockwise. The edge to highlight is based on the buffer position, so backspacing characters reverses the direction of rotation.The value of
--radiusis the center to vertex distance of the polygon.--line-*,--inside-*, and--ring-*are all respected. Separators are not drawn.Screenshots/screencaps
Release notes
Notes: Added config options to draw indicator as a polygon.