[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] [PATCH] KAgenda L&F changes
From:       Cornelius Schumacher <schumacher () kde ! org>
Date:       2003-07-23 9:53:28
[Download RAW message or body]

On Wednesday 23 July 2003 01:13, Tim Jansen wrote:
> On Monday 21 July 2003 02:12, Tim Jansen wrote:
> > I'd like to commit the attached patch.
> > It does the following:
> > - complete redesign of the agenda item look with custom renderer
> > http://www.tjansen.de/blog/pics/after2.png
>
> Ok to commit?

I have looked at the patch and I like it :-). This is a great improvement.

Some nitpicking and some more serious issues:
- KOAgendaItem::updateIcons(): Instead of "if(mIncidence->isReadOnly()) 
mIconReadonly = true; else mIconReadonly = false;" you could simply write 
"mIconReadOnly = mIncidence->isReadOnly()", same for the other lines like 
that. Does it make sense at all to use all this variables. Wouldn't it be 
easier to access the values by mIncidence and the appropriate functions in 
the painting code?
- You should put the opening brace of a function on a new line (see e.g. 
conditionalPaint() )
- Please don't use tabs (some of the KWordWrap calls contain tabs).
- Why do you rename the config file group for the category colors? This means 
that the user loses hist custom category color settings, right?
- All-day events shouldn't never have a time header.
- Wouldn't it look better to center the text on the item, at least when it 
isn't wrapped? Especially with multiple day all-day events it looks odd when 
the text is on the 
- When the highlight color is very dark, the text becomes invisible when the 
item is selected. The text of the item should be switched to white, if the 
background color becomes to dark to provide sufficient contrast.
- When the highlight color is very similar to the color of the event (e.g. 
because of a category color), it becomes impossible to tell, if an event is 
selected or not. I think we need another way to indicate selection of an 
event than only changing the color.
- Todos shouldn't have an end time shown.
- I'm not sure that I agree with you on the pixel spacing between items. 
Without this spacing it looks better in my opinion. Vertically there is no 
such spacing anyway. Without the spacing it looks like all fits perfectly 
together, it reminds me of LEGO ;-), and it's still no problem to see where 
one event end and another starts because of the thin line at the border of 
the item.

-- 
Cornelius Schumacher <schumacher@kde.org>

_______________________________________________
kde-pim mailing list
kde-pim@mail.kde.org
http://mail.kde.org/mailman/listinfo/kde-pim
kde-pim home page at http://pim.kde.org/
[prev in list] [next in list] [prev in thread] [next in thread]