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

List:       kde-devel
Subject:    Re: Patch for KTeaTime
From:       Stefan =?iso-8859-15?q?B=F6hmann?= <ebrief () hilefoks ! org>
Date:       2007-08-23 4:35:57
Message-ID: 200708230635.57465.ebrief () hilefoks ! org
[Download RAW message or body]

First sorry for the delay... I have had some trouble with registration on 
this list and so my mails were held until the list moderator reviewed it. I 
canceled this mails now and so this mail is a little bit confused. Sorry 
for that.


On Tuesday, 21. August 2007 19:28:28 Aaron J. Seigo wrote:
> unless you started with a "clean room" implementation, you can't rightly
> remove the copyrights of others from the code. if your code evolved from
> what was there, you need to retain their copyrights. that they are still
> listed in the KAboutData seems to confirm this. =)

I'm not familiar with the meaning of "clean room" in this matter. Yes - I have 
viewed the original code because first I only wanted to fix KTeaTime for KDE4. 
But an hour later or so I got the impression that it's better to write a new 
KTeaTime from scratch. So yes - I saw the original KTeaTime source but I 
didn't use 
it. I only adopted two class names and maybe one function name.

However. I added the copyrights back to the files.

> that said, you probably want to bounce this "patch" (rewrite, really =) off
> of the maintainers/authors of kteatime rather than to the list here.

You're right but i have had the impression that KTeaTime (and kdetoys in 
general) are unmaintained. Because of that I posted to this list.

But after reading your mail I have written a mail to Martin Willers as well 
(and he also responded to my mail).

> providing some screenshots to them would also probably be welcome (i know i
> appreciate those when such a large changeset comes my way)

They can be found on http://www.hilefoks.org/kteatime/index.html

> btw, i'd strongly suggest using KConfigXT so that you can get rid of all
> that readEntry/writeEntry/connect code used to populate and save the state
> of the config dialog. there is a tutorial on techbase for this and it's
> really, really simple to do.

Thanks for this suggestion. I will read the tutorial and revise the config 
stuff. 



On Wednesday, 22. August 2007 09:37:56 Daniel Teske wrote:
> I just did a quick look through the code. Nothing in depth, but rather just
> reading through it. So this is nitpicking, but well I assume you actually
> will find this helpfull.

Oh yes - every suggestion is helpful. I'm very new to C++ and KDE development 
and have to give up many things I learned in Java in recent years. ;-)
   
> I dind't actually apply the patch, nor did I 
> compile it. I'd like to see a screenshot of the configuration dialog. I
> have the distinct feeling that having both a seconds spinbox and a slider
> doesn't make much sense. And I'm happy that someone is porting kteatime to
> KDE4.

I feel the old "Anonymous Tea" dialog wasn't very nice and I thought my design 
is a little bit nicer. But that is only my personal impression. Because you 
didn't like them (as I take from your next mail mentioned below) I will 
remove the sliders.

> I guess you are missing a call to QApplication::setQuitOnLastWindowClosed()

Thanks a lot - that's it!

> >(static_cast<KMenu*>(contextMenu()))
>
> Are you sure about this cast? contextMenu() returns a QMenu *

Yes - I'm not happy with this cast though and neither with the entire function 
rebuildMenus().  

> And now the big question: Ain't that more usefull as a plasma applet
> instead of a Systemtray application?

That's a legitimate question. In short: yes. 

But notice that my primary intention was only to try repairing KTeaTime for 
KDE4. I didn't try to write a plasmoid because I didn't believe that I would 
learn 
enough to do that. I have some experience with Java but this version of 
KTeaTime is my personal "Hello World" in C++.



On Wednesday, 22. August 2007 22:46:18 Daniel Teske wrote:
> Like I suspected the spinbox and slider option for all the time controls
> doesn't improve on just a spibox. The UI is probably better without the
> sliders.
As said above I thought it would be a nice idea. But I see it wasn't - so 
I will remove the sliders.

> What I noticed where a lot of crashes. I tracked those down to the
> following problem.
>
> Inside qt (qemu.cpp) is this code after activating an action.
>
>     if (QAction *action = qobject_cast<QAction *>(q->sender())) {
>         emit q->triggered(action);
> #ifdef QT3_SUPPORT
>         emit q->activated(q->findIdForAction(action));
> #endif
>

Oh - I understand.

> Easy and correct fix is to not rebuild the menu for just running a tea. We
> just need to disable and enable a few actions.

I have found a better way by using QActionGroup and only check the state of 
menu items for most actions. Now only when the list of teas changed the menu 
will be rebuild.

> And yeah what Aaron said, you can't just delete the copyrights. The code
> still resembles the old one in a few places.

Ok - sorry! I added the copyrights back to the files. 


I now have implemented most of the suggestions I got. Next step is removing 
the sliders and change over to KConfigXT. So an updated patch is on the 
road...


- 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]