Created attachment 208266 [details] v1 - Switch to new upstream - Drop redundant option descriptions - Drop redundant DOCS dependencies - Connect tests to the framework - Respect PREFIX != LOCALBASE for compton.conf
Alexey, are you de facto maintainer of this port? If so review would be appreciated but, as usual, subject to timeouts.
Jesper, can you check if x11/compton-conf still works? JT, can you check if x11/lumina-core still works?
I'm not just de-facto maintainer, I actually use this port. Given the size of the proposed patch and amount of changes, please give me some time to properly review it.
(In reply to Jan Beich from comment #2) Well some compton-config names has been deprecated, and some set by compton-conf are already gone, but after a short test the most relevant settings have retained their names, or have sane defaults. If you take a test your self, keep in mind that you must restart compton after applying settings in compton-conf. See https://github.com/lxqt/compton-conf/issues/18
Created attachment 208270 [details] v1.1 Cosmetic changes (TIMESTAMP, WWW, patch comments, commit message)
(In reply to Jan Beich from comment #2) I dont think there should be any issues on the Lumina-Core front.
(In reply to Alexey Dokuchaev from comment #3) > I'm not just de-facto maintainer, I actually use this port. I also use this port and have been for years (but maybe not as long as you). Unfortunately, MAINTAINER field doesn't support more than 1 email and FreeBSD.org bugzilla instance doesn't support generic review flag. > Given the size of the proposed patch and amount of changes Most risky changes come from upstream, the rest can be reviewed in ~10 minutes (ignoring time on rationalizing stuff you don't like). According to https://repology.org/project/compton/versions many Linux distributions have switched yshui's fork, even NetBSD. For one, "--vsync none" no longer lies after v5 per https://github.com/yshui/compton/commit/e2182bb00bb4 which made me realize my config relied on a buggy behavior. ;) > please give me some time to properly review it. Did you miss "subject to timeouts" in comment 1? Make sure to provide some feedback by the next Saturday. Blatant stalling for time will be ignored as "cookie licking".
> According to [Repology] many Linux distributions have switched yshui's > fork, even NetBSD. Fair enough. It would be nice if we could avoid the PORTEPOCH bump but I guess we cannot. :( > (ignoring time on rationalizing stuff you don't like) Well, that's a bit dismissive being said upfront. Anyway, the patch looks good for the most parts, except for couple of nits below: > - Drop redundant option descriptions Actually, as much as I'm a fan of default option descriptions, I felt that they were not descriptive enough for this port, e.g. see ports r433464 for DRM and OpenGL. Same for PCRE support: default wording ("Use ...") does not really explain what kind of "use", but if now it spans beyond the blacklisting, I guess we can live with default generic, vague PCRE_DESC. > - Connect tests to the framework Having tests is nice, but I don't understand the point of leaking the framework-level stuff to user-exposed OPTIONS. Why not simply hook the tests under TEST_TARGET ("make test") and remove the option? > Did you miss "subject to timeouts" in comment 1? No I didn't, why? > Blatant stalling for time will be ignored as "cookie licking". I'm not sure what that's supposed to mean, but it sounds rude and insulting. Go lick your "cookie" yourself.
Created attachment 208534 [details] v1.2 Changes since previous version: - Disable DRM option by default per startup warning [ 10/23/19 18:26:27.091 vsync_init WARN ] The DRM vsync method is deprecated, please don't enable it. - Make TEST option unconditional (In reply to Alexey Dokuchaev from comment #8) >> - Drop redundant option descriptions > Actually, as much as I'm a fan of default option descriptions, I > felt that they were not descriptive enough for this port, e.g. see > ports r433464 for DRM and OpenGL. Same for PCRE support: default > wording ("Use ...") does not really explain what kind of "use", but > if now it spans beyond the blacklisting, I guess we can live with > default generic, vague PCRE_DESC. Current OPENGL_DESC is as ambiguous as upstream. "VSync method" is no longer supported as --vsync is now boolean while "backend" is just an implementation detail for "2D rendering the whole screen". option('opengl', type: 'boolean', value: true, description: 'Enable features that require opengl (opengl backend, and opengl vsync methods)') Users are supposed to refer to documentation to figure out where and how OpenGL, PCRE, etc. are used. Trying to tweak options *before* familarizing with the software is footshooting. >> - Connect tests to the framework > Having tests is nice, but I don't understand the point of leaking > the framework-level stuff to user-exposed OPTIONS. Why not simply > hook the tests under TEST_TARGET ("make test") and remove the > option? `test` was part of non-default option in order to avoid `-Dunittest=true`. Given `--unittest` is harmless if unused always building it seems OK.
FWIW, the next version will rename compton to picom. See review D22137 for downstream POV.
A commit references this bug: Author: jbeich Date: Wed Oct 30 13:29:56 UTC 2019 New revision: 516059 URL: https://svnweb.freebsd.org/changeset/ports/516059 Log: x11-wm/compton-yshui: add new port Actively maintained fork of Compton (X11 compositor). Will be renamed to Picom during the next update. PR: 241216 Tested by: jsm, q5sys Reviewed by: danfe (previous version) Changes: head/x11-wm/Makefile head/x11-wm/compton/Makefile head/x11-wm/compton-yshui/ head/x11-wm/compton-yshui/Makefile head/x11-wm/compton-yshui/distinfo head/x11-wm/compton-yshui/files/patch-Makefile head/x11-wm/compton-yshui/files/patch-src_c2.h head/x11-wm/compton-yshui/files/patch-src_log.c head/x11-wm/compton-yshui/pkg-descr head/x11-wm/compton-yshui/pkg-plist
As suggested upstream I've left Compton and its users as is. https://github.com/yshui/picom/issues/228#issuecomment-547563344