> On July 11, 2014, 4:48 p.m., Sebastian Kügler wrote: > > It would be easier to review, if you made separate patches for the renaming > > and the actual changes in the code that juggle around the objects. > > Martin Klapetek wrote: > I did the renaming in the middle of this as I got annoyed by the > confusing names. > > To sum it up - all Rectangles are replaced by one Rectangle and two > Repeaters at the top in DaysCalendar.qml, the rest is just renaming and > sanitizing. > > Sebastian Kügler wrote: > I understand all that, it just doesn't change the fact that now, I have > to look at every changeset first, if it's just renaming or a real code > change. You want others to verify that everything's correct, because if not, > you can just commit it without review and then wait for complaints. We've > decided that that is not how we want to work together. (There's already one > thing gone unnoticed to you, so I think a good review is warranted here, > especially since, in the past, you have suggested changes which are > detrimental to the overall design, suggesting to me that you haven't fully > internalized what the important points of the design are, or that you are > making different calls than I would do. > > In the end, if changes aren't self-contained, you're accepting a > trade-off between your time and the reviewer's. > > David Edmundson wrote: > That was phrased overly negatively, there's no need for that. > > Sebastian Kügler wrote: > So your reaction to that is to give a shipit to code that isn't done > reviewing? > > I wasn't happy with the patch, waiting for a version that does at least > not introduce regressions. To be honest, I find it not cool that you gave a > shipit and Martin pushed the code as-is ignoring my feedback and review. For > one, I don't like to be asked for feedback, then ignored if it doesn't suit, > I have better uses for my time. Also, I wrote the code in question (or at > least did the major rework to get it to its current form), so I can pretend > to know the code, and its culprits. Hearing Martin tell me on the one hand > that every pixel matters, and then ignoring this exact feedback (even > dismissing that there's a regression potential in the first place) meant that > he is either using different criteria for his own and other code, or that he > doesn't understand the issue. That's what I pointed out, and after seeing > what came after that discussion, this impression is even stronger. The review > should not have been shipped for these social reasons (I wasn't done with the > re view, you both chose to override my concerns and just create facts), and for the following technical reasons (which are more important, but still were ignored): > > 1) The code introduces visual regressions. While Martin was keen to point > out that the results are visually exactly the same, and didn't even post a > screenshot to prove it in the first place, it took me less than 10 seconds to > spot a visual regression. Martin has pointed out that it's not visible to > him, but seems to understand the issue introduced now. I was assuming he'd > post a second try, with the opened issues improved. That's exactly what we > have reviews for, and also exactly why this patch should not have been > shipped -- we weren't done reviewing (as is evidenced by the continuation of > this thread around the remaining technical issues). > > 2) The code in this patch makes it harder to fix these overlapping > pixels, so it's a step back in that regard. > > I'm in favour of reverting the patch, and giving it another proper > review, after the issues that have been pointed out (overlapping pixels in > different coloring where they wouldn't overlap before), but also answering > the question how to fix the overlapping pixels (this patch goes in the wrong > direction towards the overlapping junctions, I *think* it can be easily fixed > with the version pre-dating this patch, by shortening the 1px Rectangles > where there'd be junctions). > > Also, the "care to give it a try" is not a corteous thing to do: it is > first making an issue harder to fix, and then shoving the work onto someone > else. > > So, please revert, retry, and let's close this chapter by replacing it > with a good quality patch without visual or maintenance regressions. > > Thanks. > > David Edmundson wrote: > I saw this part of the review saying that you were unable to review it > because it had one minor unrelated change. It wasn't that hard to ignore it, > so I reviewed it for you. > > As for the regression; I should have read your previous review, and it is > my fault for not seeing that. > > I'm not wholly convinced reverting is worth it; the previous version also > had the brighter points on the insides, bottom and right so this is scarcely > a regression because it has it on the top and left too. > > (off topic, Rectangle has a secret property called border.pixelAligned > maybe that will help with either patch)
> To be honest, I find it not cool that you gave a shipit and Martin pushed the > code as-is ignoring my feedback and review. For one, I don't like to be asked > for feedback, then ignored if it doesn't suit The "reviewer" group is Plasma; I didn't ask you specifically. And I'm tired constantly getting negativity and grumpiness from you. > Hearing Martin tell me on the one hand that every pixel matters, and then > ignoring this exact feedback I was talking about pixel-perfect alignment (which we now have). Two different things. You, on the other hand, discarded one of my patches because "it adds one more item", here I lower the number of items by 75 and you don't even bother looking... > While Martin was keen to point out that the results are visually exactly the > same, and didn't even post a screenshot to prove it in the first place I totally did. It's here, you can have a look. > Also, the "care to give it a try" is not a corteous thing to do: it is first > making an issue harder to fix, and then shoving the work onto someone else. I have 0 experience with Canvas, Eike seems to know what he's talking about. What's wrong about asking for help with something he has greater experience than I do? > So, please revert, retry, and let's close this chapter by replacing it with a > good quality patch without visual or maintenance regressions. You know, I'm actually thinking about giving up the maintainership of this. I don't need to keep hearing this over every. single. calendar. patch. that I do. I also have better use of my time. - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119232/#review62142 ----------------------------------------------------------- On July 12, 2014, 1:52 p.m., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119232/ > ----------------------------------------------------------- > > (Updated July 12, 2014, 1:52 p.m.) > > > Review request for Plasma. > > > Repository: plasma-framework > > > Description > ------- > > Currently the grid itself is composed of 88 rectangles that draw all the > lines in a way that two big rect draws the whole two topmost horizontal and > leftmost vertical border lines and then each day rectangle is drawing small > bottom and right rect. > > This patch reduces it to 13 rects only where one rect draws the whole frame > around the grid and then 1px wide/high rects draw the inner lines. Results in > much cleaner & simple code. > > Plus there's a small refactor on the id names so it makes more sense. > > This does not require any additional changes in the applets. > > > Diffs > ----- > > src/declarativeimports/calendar/qml/MonthMenu.qml 5209816 > src/declarativeimports/calendar/qml/MonthView.qml ba3fe95 > src/declarativeimports/calendar/qml/CalendarToolbar.qml cd28702 > src/declarativeimports/calendar/qml/DayDelegate.qml 11f0afa > src/declarativeimports/calendar/qml/DaysCalendar.qml ae9df01 > > Diff: https://git.reviewboard.kde.org/r/119232/diff/ > > > Testing > ------- > > All applets using the Calendar component looks exactly the same and work > perfectly fine with no changes in the applets. > > > File Attachments > ---------------- > > digital-clock before/after > > https://git.reviewboard.kde.org/media/uploaded/files/2014/07/11/9a4f02eb-b06c-4d13-8ea5-94276659fba8__plasma_cal4.png > > > Thanks, > > Martin Klapetek > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
