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

List:       kde-pim
Subject:    Re: [Kde-pim] [PATCH] KNotes (some items of the todo list...)
From:       "Gerold J. Wucherpfennig" <gjwucherpfennig () gmx ! net>
Date:       2003-04-25 18:33:27
[Download RAW message or body]

On Friday 25 April 2003 14:56, you wrote:
> [Hm, that's what you get when replying to mails while your modem is still
> downloading mail... sorry for asking the same question three times :-]
>
> On Thursday 24 April 2003 02:11, Gerold J. Wucherpfennig wrote:
> > Here is an update...
> >
> > (bugfix: deleting the selected notes)
> >
> > Please test it and tell me (-> CC to me) if I may commit
>
> Alright, there's a few things to be mentioned:
>   - First of all, to make reading patches easier, please don't inlcude
> those heaps of whitespace changes

Then I could use the patch option to ignore them, no need to change things...

>   - I don't like to have classes named blah and blah2 as this is
> meaningless. And in this case it makes no sense at all to have another
> class as the only thing you added was the
>        setToggleButton( true );
>     call in the constructor. Rather create a KNoteButton and call that
> method on the new object.

That should be ok, no much work...

>   - Members in KNotes classes start with m_

ok

>   - you changed KNote's constructor to pass a KNotesApp object. This should
>     not be done as every KNote should be independend of the application
> using it. I want to make a KNote a plugin later. Also, I can't see *why*
> you did it.
>

I coded the slots in knotesapp, because there was some sample code to
get through all notes. I think I can do it in knotes directly, but I'm not 
sure, I have to take a look into the code first... 

> Now to the semantics:
>
> What's the sense of being able to select a note? Only to be able to kill
> them?

To select a group of notes and kill, show or hide them.
First I wanted to do that with the shift key, but the X samples in kdm & kbd
was too complicated for me, I wanted to add this later if I can figure it out
somehow.

>  Hmm. Not perfectly happy with that. Why not delete the note directly?
> OTOH, if we could find some more useful actions for a set of notes that
> concept might be a good idea.

Have you tried to kill 100 notes like this? Even closing all notes is a 
pain... There isn't even a shortcut for this action.
(I only created the notes for testing purposes,
of course with Ctrl+N and not manually.)
This was my only reason to code it. And if you aren't happy with
that, I don't care. I find it useful and will keep it in my local cvs dir.

>
> There's a problem with hide/show all notes the way you implemented it: hide
> all hides all notes on all desktops. Show all will then show all hidden
> notes on the current desktop. Even more, in your case, show all notes will
> *move all* notes to the current desktop regardless of the state. This is
> definitely not what you want. The solution is to either save the last
> desktop for every note and use this member only in case of ShowAllNotes or,
> that's what I'd like to have, hideAll hides only the notes on the current
> desktop.

I don't have a problem with this and I don't want to save the
desktop number.

> Sorry for being picky, but I hope you get the ideas.

Well I must admit, that I didn't care much about that,
I was happy that I to got it to work. It was ok for me
personally. But instead of committing it I asked you
to give me some impressions. You know, if you have worked
at something you don't see such little errors and it's good if
somebody else reviews it. I didn't even know any more that I
changed the constructor :-x


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