Review Request 119232: Rewrite the Calendar component grid + small refactor

Mon Jul 14 11:46:42 UTC 2014

> 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 review, 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140714/102effe5/attachment-0001.html>