Rewrite the painting of an month item
ClosedPublic

Authored by ognarb on Oct 7 2018, 1:17 PM.

Details

Summary

+ Remove gradiant in background.
+ Simplify the drawing of the rounded rectangle.

Screenshot before:


Screenshot after:

Diff Detail

Repository
R76 PIM: Event Views
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ognarb created this revision.Oct 7 2018, 1:17 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptOct 7 2018, 1:18 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
ognarb requested review of this revision.Oct 7 2018, 1:18 PM
ognarb edited the summary of this revision. (Show Details)Oct 7 2018, 1:23 PM

Could you provide a screenshot for before this changes.
And a screenshot when we select multi days please.
thanks

ognarb edited the summary of this revision. (Show Details)Oct 7 2018, 2:03 PM

Could you provide a screenshot for before this changes.
And a screenshot when we select multi days please.
thanks

Done, I updated my summary.

ognarb updated this revision to Diff 43043.Oct 7 2018, 2:45 PM
ognarb edited the summary of this revision. (Show Details)

Fix rounded corner for beginn and end item

dvratil requested changes to this revision.Oct 8 2018, 5:25 PM

There seems to be a regression compared to the previous version when drawing events that span multiple weeks - in the original version the event is not rounded on week end and week start, while on your screenshot it appears the event has round corners on Sunday and Monday. Can you look into it?

src/month/monthgraphicsitems.cpp
267

const

This revision now requires changes to proceed.Oct 8 2018, 5:25 PM
ognarb edited the summary of this revision. (Show Details)Oct 8 2018, 5:31 PM

There seems to be a regression compared to the previous version when drawing events that span multiple weeks - in the original version the event is not rounded on week end and week start, while on your screenshot it appears the event has round corners on Sunday and Monday. Can you look into it?

Sorry, I saw it too and already fixed it but forgot to update the screenshot. Just did it.

ognarb updated this revision to Diff 43144.Oct 8 2018, 5:36 PM
ognarb marked an inline comment as done.

fix const

dvratil added a comment.EditedOct 8 2018, 5:38 PM

Thanks. With the small radius, it still feels like it's hard to distinguish between an event that ends on Sunday and an event that continues to next week - could you try removing the margin in this case, so that the event seemingly goes out of the screen to make it more obvious that it spans multiple days?

ognarb updated this revision to Diff 43150.Oct 8 2018, 5:57 PM

Fix some margin problem

Some margin problem remain

ognarb added a comment.EditedOct 8 2018, 6:07 PM

Thanks. With the small radius, it still feels like it's hard to distinguish between an event that ends on Sunday and an event that continues to next week - could you try removing them in this case, so that the event seemingly goes out of the screen to make it more obvious that it spans multiple days?

There was not radius at all for Monday and Sunday, but a small margin. So I changed the radius from 2 to 3, and removed the margin for Sunday and Monday. My problem is now that it also removed the margin for the small items. Because begin small item are 'beginItem' and 'endItem'. And it(s not clear anymore, if two items are the same event spaming 2 days or two differentent events.

dvratil added a comment.EditedOct 8 2018, 6:23 PM

The smaller radius and bigger margin looked better, I'd revert to that.

So maybe we could pass some additional flag when creating the MonthGraphicsItem in MonthItem::updateMonthGraphicsItems() - this code is aware of the full event length, so it could simply pass an additional flag saying whether the event continues on next week (or started in the previous week, or both) and the painting code could handle that completely separately, like avoiding the border and margin on the respective side. What do you think?

ognarb added a comment.Oct 8 2018, 6:34 PM

I think it would be easier if we change the meaning and/or name of isBeginItem and isEndItem. So that:

isBeginItem() && isEndItem() => no margin and radius left and right
isBeginItem() && !isEndItem() => no margin and radius left
!isBeginItem() && isEndItem() => no margin and radius right
!isBeginItem() && !isEndItem() => margin and radius left and right
dkurz added a subscriber: dkurz.Oct 8 2018, 6:56 PM

The margin helps to immediately grasp that the event starts/ends on that day, just like the radius. I think there should be a margin iff there is a radius.

In the current code, there is also no border drawn when an event continues in that direction (left for !end, right for !begin). Why did you change that? I think the border on the right side of your event on 21st makes the impression of a gap between the right border of the view and the event. My suggestion:

begin => radius, margin and border on the left side
not begin => no radius, margin or border on the left side

end => radius, margin and border on the right side
not end => no radius, margin or border on the right side

For short: Either all radius, margin and border, or none at all, per side

I hope I did not confuse what begin/end items are in the first place.

I think Denis has explained it better than I did - the margin and borders on 21st and 22nd break the effect of the continuous event - that's what I was concerned about.

dkurz added a comment.Oct 9 2018, 5:55 PM

Another thing I just noticed: You use border width 1 unconditionally, when it was 2 unconditionally before.

I think that border width 1 is better if the border does not carry information, so +1 in case of CategoryOnly or ResourceOnly. However, if the border DOES cary information (ResourceInsideCategoryOutside, CategoryInsideResourceOutside), that information might be harder to grasp. I experimented a little bit and found this most useful:

  • {Category,Resource}Only => border width 1
  • Otherwise => border width 2

However, we should not change too much in a single commit, so I suggest to leave it at "ft" for now and then improve border width in another Diff. If you still choose to ignore "ft" completely, you should remove the static const that is then unused.

ognarb updated this revision to Diff 43588.Oct 14 2018, 3:11 PM

Revert some change

ognarb updated this revision to Diff 43589.Oct 14 2018, 3:19 PM

Fix some code

ognarb added a comment.EditedOct 14 2018, 3:24 PM

Sorry for the late update, I had exams. So I rewrote this differential a little bit and removed some change.

Here is an actual screenshot:

No worries, hope your exams went well :)

Do you think you would be able to get rid of the right-side border on 21st and left-side border on 22nd? That way it should be completely obvious that the event continues.

(sorry, that we are still asking new changes, but since we are finally improving this code, I want to make sure it's really looking smooth and polished)

No worries, hope your exams went well :)

:)

Do you think you would be able to get rid of the right-side border on 21st and left-side border on 22nd? That way it should be completely obvious that the event continues.

The problem is that I need then to render the border manually, and it's a bit more difficult than adding a pen. But I agree it's better without border so I will try to implement it :).

(sorry, that we are still asking new changes, but since we are finally improving this code, I want to make sure it's really looking smooth and polished)

No problem, I also want this part of korganizer to look smooth and polished.

ognarb updated this revision to Diff 43719.EditedOct 16 2018, 9:11 AM
  • remove border for begin and end of weeks

New screenshot:

ognarb updated this revision to Diff 43720.Oct 16 2018, 9:16 AM

Small fix

dvratil added inline comments.Oct 16 2018, 11:25 AM
src/month/monthgraphicsitems.cpp
209

Do you maybe need to draw the background one pixel wider without the border? If you zoom in on the latest screenshot, the right-most pixels on 21st are white (well, light-green, but I suspect that's aliasing or something like that).

ognarb updated this revision to Diff 43729.EditedOct 16 2018, 12:01 PM

I fixed it :)

Screenshot:

dvratil accepted this revision.Oct 16 2018, 1:43 PM

I think it's perfect now, so from my side, this is good to go. I don't think this kind of change belongs to a bugfix release, so I'll push this to master (for 18.12 release).

This revision is now accepted and ready to land.Oct 16 2018, 1:43 PM
This revision was automatically updated to reflect the committed changes.