On Tue, 29 May 2018 10:13:33 -0400 Mike Blumenkrantz <michael.blumenkra...@gmail.com> said:
> While it is true that a mail was sent by JackDanielz related to this patch, > the elapsed time between that mail and this revert is roughly 1.5 hours on > a Saturday afternoon during a federal holiday weekend. If inactivity from a > patch author is going to be used as reasoning for a revert, I think that > the time period given should be a reasonable one. it's the severity of the break. a totally blank screen. impossible to recover from without going to a text console and finding a better working revision. when a break is severe enough then a revert asap is what needs doing with people already finding it and complaining after an hour or so. > As stated in the original thread, testing for this patch occurred on a > number of machines, all with a similar quirk: modesetting occurs at the > start of every session. This happened to work around the issue exhibited in > some sessions. It's a bit unlucky, but that's the sort of result that can > be expected when there is no unit testing for the component being modified. see below... > Questioning the review process in your case seems like a bit of a slippery > slope considering that the patch was submitted over 3 weeks ago with you as > a reviewer, and it was mentioned more than once in another ticket that you > were active in at that time. i didn't notice it... but my point is that it's quite severe and review didn't find it. but a review should involve trying the code out and giving it a test for possible failure modes. what kind of review is going on? is it just a scan "oh that looks good to me?"... if so what is the point if it's not catching problems by at least trying it out (decently)? this also goes for just compiling the code. whilst i was checking some older revisions i found a commit that went through review, was approved AND pushed... and it didn't compile because it was missing a ) - not even in an ifdef for some specific os or display system. it never compiled. sure. a few commits after was another reviewed patch that fixed this. what kind of review is going on that lets basic stuff like this through (you and cedric were not involved with this one)? this was a few weeks ago. > Had the commit log for this revert been only "this patch causes this > regression" with a simple description of the issue, it's unlikely that this > mail would exist. Instead, however, it seems like this revert was being > used to bash both patch review and testing--something which makes little > sense considering that you declined to attempt either when it was > explicitly requested of you. I don't think it's fair for you to > (indirectly) criticize Cedric for trying to do the work that you did not do. the person reviewing and approving takes on the responsibility to ensure quality. me being involved or not is not relevant to the quality of the review (testing) that was done. it's a general thing - test things. i find commits that break simple things that should have been caught with fairly basic testing fairly often. when i do i make noise about it to remind people to improve their testing. > I can understand your mindset in writing this commit log and related mail, > but I don't think statements like these are beneficial to us as a community > or the project as a whole. well you explained what about testing made it not trigger, though that would only explain i believe starting e afresh on a new xserver for example, and never restarting it... or never testing e in x... it triggered instantly on both my desktop and 2 laptops (screen resolutions did not need a reconfigure on all 3... and one is multi-screen). any restart of e (ctrl+alt+end) should bring this out. on restart the screen is already in the final config state and thus no change will happen thus no configure event. i restart e when i'm testing things at least twice (once to try the newly compiled code for the first time, and then again to check shutdown and re-exec work). i still maintain that testing here was poor. a restart (in x) would show the issue right away (no reconfigure of screen). since i believe this is the case, my take is that testing was inadequate for sure. it appeared immediately on 3 of my machines after rebuilding and ctrl+alt+end immediately. no exceptions. review should involve testing the code in question. decently. so ok - it was run... but not enough. testing needs to be far better. totally "it doesn't work at all" kinds of changes need to not slip through if at all possible. > On Sat, May 26, 2018 at 4:01 PM Carsten Haitzler <ras...@rasterman.com> > wrote: > > > raster pushed a commit to branch master. > > > > > > http://git.enlightenment.org/core/efl.git/commit/?id=4715c099ecb1e9a5c7de5dab883560e198704c72 > > > > commit 4715c099ecb1e9a5c7de5dab883560e198704c72 > > Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com> > > Date: Sun May 27 04:52:03 2018 +0900 > > > > Revert "ecore-evas-x: set draw_block until the window receives a > > configure event" > > > > This reverts commit 7b80038fa7b54cff27b463382283211727aaf104. > > > > JackDanielz asked nicely, but this hasn't been reverted. As this > > totally breaks enlightenment (it's black) and this happens on > > everythng I've tested (1 laptop, desktop and Xephyr) I'm calling this > > patch a dud. > > > > Now... what kind of review is going on here? This hasn't been tested. > > What kind of review doesn't build + run things? > > > > for the reasons of poor review and massive horribler fully complete > > desktop like breakage ... this gets reverted as master should not be > > broken like this. > > --- > > src/modules/ecore_evas/engines/x/ecore_evas_x.c | 27 > > ++----------------------- > > 1 file changed, 2 insertions(+), 25 deletions(-) > > > > diff --git a/src/modules/ecore_evas/engines/x/ecore_evas_x.c > > b/src/modules/ecore_evas/engines/x/ecore_evas_x.c > > index 47baeff5ff..3d88e9aae3 100644 > > --- a/src/modules/ecore_evas/engines/x/ecore_evas_x.c > > +++ b/src/modules/ecore_evas/engines/x/ecore_evas_x.c > > > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > enlightenment-devel mailing list > enlightenment-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- Carsten Haitzler - ras...@rasterman.com ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
