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

List:       kde-devel
Subject:    Re: Patch for KTeaTime
From:       Stefan =?iso-8859-1?q?B=F6hmann?= <ebrief () hilefoks ! org>
Date:       2007-09-06 22:01:03
Message-ID: 200709070001.04383.ebrief () hilefoks ! org
[Download RAW message or body]

Hello,

I updated the patch on http://www.hilefoks.org/kteatime/kteatime.diff


> Hi Stefan,
>
> this patch looks very good. Good job and congratulations. You mentioned
> that you are very new to C** development. So I'm curious how did you learn
> C++, because obviously you got a lot of things right.
>
Many thanks. Yes I'm new to C/C++ development and it's nice to hear that I 
didn't make to many mistakes....  especially because I have experience with 
Java and compared to this language I have many more things to notice in C++. 
But with this knowledge in mind, one or two good C++ books and many excellent 
sourcecode (aka KDE) that people like me can read for free it's easier as I 
expected.


> I think this patch as is, could go (almost) in. So doyou want me to sumbit
> it, or would want to apply for an svn account?
>I think kteatime isn't enough for an svn account so I'm thankful when you   
>
commit it. But please have a look into CMakeLists.txt and Messages.sh because 
I'm not sure if I have done everything correctly.


> Anyway the last few comments:
> I still think in the long term kteatime would be more usefull as a
> plasmoid.
I agree with it. As I noticed I didn't believe that I can do this and, the old 
release-plan in mind, I only wanted a working kteatime in KDE 4.

I think until KDE 4.1 someone (and maybe myself) has implemented a nice 
replacement as plasmoid.

However. For my personal use I see no reason for a plasmoid because I don't 
want kteatime on the desktop (I cannot see it there) and also I don't want 
a "kicker applet" that needs more space then kteatime now needs.

But I'm sure there are other users who wants that... 


> And on the code:
> Slots are normal functions. So you can just call them, no need to use "emit
> runTea" or "emit reapintTrayIcon" just call them. And for those two
> functions you don't actually need to declare them as slots. (Because you
> aren't connecting to them anyway.) emit is just syntactic sugar for humans,
> but what emit means is that the code is emitting a signal. So use emit only
> with signals.
Ah - ok.


> The Teamodel and Settings ctor should take a *const* reference. instead of
> just a reference.
Of course. I think now it is correct.


> The only other thing I'm not sure about is that position restore code you
> hace. I'm not sure wheter negative postions are really invalid.
>
My intention is to ensure that the dialogs always accessible. Basically 
negative values are ok as long as the dialogs are not purely outsite the 
screen.  
For example this situation can happen when people switch there monitor 
resolution (because they're using an external monitor on their laptop) or 
something similar. So I think it's principly not a bad idea. My 
implementation isn't bulledproof but avoids hidden windows in such a 
situation.

In my patch I didn't remove this function but when you think it should be - 
I'll do it... (or do it yourself before committing).


Finally many thanks for suggestions, help and the time you're (and of course 
the other people) spent to read my code.


-Stefan

 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]