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

List:       kopete-devel
Subject:    Re: [kopete-devel] Usability issues in Chat History dialog
From:       Martijn Klingens <klingens () kde ! org>
Date:       2007-02-26 10:44:27
Message-ID: 200702261144.27891.klingens () kde ! org
[Download RAW message or body]

Hi Johannes,

I cannot comment on most of the code since I don't know the History plugin 
very well (I would need to famliarize myself with it too), so I hope someone 
else can comment on parts of.

But some general comments to your mail:

On Saturday 24 February 2007 16:24, Johannes Jowereit wrote:
> first of all, thanks for your encouraging answers. I have now written
> a patch (applies to Kopete version 0.12.3) that solves at least some
> of the issues I wrote about:

Please note that Kopete 0.12 (KDE 3.5 version) is in maintenance mode, and 
only bugfixes are allowed. Some of your changes are perfect, but others 
probably belong in KDE 4's Kopete.

Speaking of which, do you think you'll be able to work on Kopete for KDE 4 as 
well? We could use more developers there...

Now, on to your changes:

> - The search line above the date list view has been removed.

Is it gone entirely or replaced with something else?

> - The status label has been removed and the progress bar is now placed
> at the bottom-left corner of the date list view (like in the file
> open dialog), where it doesn't take much space.

I probably need to see this before I can comment. It sounds like a good idea 
though.

> - The messages now have a header, even if it is not very pretty at the
> moment.

This is a new feature and probably a no-go for KDE 3.5, especially given that 
it introduces new strings.

For KDE 4 it's a good idea.

Some notes and thoughts though:

* You ask at some places whether i18n is needed. E.g.:

+  // FIXME: Does this have to be i18n'ed?
+  recipientsHTML += QString("<td>%3%1 (%2)</td>").arg(...);

  The answer is "yes", because in right-to-left languages (Hebrew and Arabic)
  the string may look like '(%2) %1%3' or so. It needs a translator comment to
  explain the meaning of %1-3 though or nobody can translate it ;)

* In that same line of code you do

   QString( "%1 %2 %3" ).arg( A ).arg( B ).arg( C );

  Don't do that. Using arg() like this is slower, but worse, if A contains the
  text "%1", it will replace that %1 with B, and %2 with C. This is the Qt 2
  way of doing things, in Qt 3 and Qt 4 it should be:

   QString( "%1 %2 %3" ).arg( A, B, C );

* Should we use the chatwindow style here too, to make it look consistent?

> - The i18n issues have been fixed; all dates are now localized.

That should be safe for KDE 3.5. i18n changes that are fixes are allowed.

> - The search button is removed and has been replaced by a timer which
> starts the search after a typing pause of 500ms.

I don't think that's a good idea. I personally don't want to wait half a 
second, and at other times there's a half-second delay when I don't want to 
search yet.

If the search can be made lightweight, make it truly incremental. Otherwise, 
keep the search button.

In any case, this is too sensitive for bugs, please only do this for KDE 4.

> Additionally, I have fixed two bugs I found during testing:
> - The search now only begins when all messages have been loaded.
> Before, the progress bar did some crazy things when searching while
> the messages were loaded.

Good idea.

> - If you change the contact with the contact combo box while messages
> are loaded, the date list view will no longer be polluted with the
> messages of the contact selected before.

Also great :)

> In the UI, the combo boxes and the search box can now be accessed via
> keyboard shortcuts (accelerators).

That's a string change, but I very much would like to see this in 3.5. Matt, 
can this be done for 3.x?

> The message is now not only displayed in the message viewer when the
> corresponding item is clicked, but also when it is selected e.g. by
> keyboard (it now reacts simply on the selectionChanged signal).

Great!

> There are also some problems I couldn't solve. Maybe you can help me
> with it:
> - I commented out the code in line 374-381 (non-patched version) and
> replaced it by putting the account in the header. However, the
> original code does some checks I don't really understand. Is my code
> sufficent?

I'm not sure what it does either. Did you try 'svn annotate' to see what 
revision added this and find the corresponding 'svn log' message?

> - I couldn't get Qt to let the size of the date list view fixed, so
> now it is too big and resizes when the window is resized. How can this
> be fixed?

QSizePolicy::Fixed? But I'm not sure if hardcoding is a good idea. A splitter 
might be better.

> - I wanted to include a link to chat with the person whose messages
> are displayed. In KMail, the link had a form like "im:<cryptical
> string>", but I couldn't figure out how to get that in Kopete. Or
> does this only work for contacts that are listed in the address book?

The cryptical string is the ID from the address book. But within Kopete you 
don't need to make this indirection. I think the chat window already does 
this. Take a look at that. But this really screams for sharing the code 
between history and chat window.

> - How are chats with more than one person handled? (I don't have an
> account on which testing this is possible.) I don't think any special
> code for this was included in the original version of the history
> plugin, so I assume I don't have to care about it. Or is this a
> missing feature?

It's *badly* missing. Kopete makes a mess of history of multi-person chats.

I have some chat logs with 3-person chats, but when viewing them in History I 
don't see who wrote what, history assumes it's all written by one person :S

The data on disk is correct, it's the display that's broken, so it can be 
fixed for my existing logs btw.

> I hope my patch helps you to improve the history plugin. In any case,
> it is great contributing something to KDE. Please help me a bit by
> answering my questions.

See above.

I now have a request for you:

Can you please send a couple of smaller patches that each address one or a few 
issues? This cumulative patch is hard to breakdown in the individual fixes.

In particular, I would like to see a patch for the i18n issues, the 
accelerators, the 2 bug fixes you found during testing, and the 
selectionChanged part.

Those can probably all go into KDE 3.5 right away and make the remaining patch 
a fair bit shorter.

Thanks for the work you've done!

-- 
Martijn

I want to get in shape, but the gym is two flights up.
_______________________________________________
kopete-devel mailing list
kopete-devel@kde.org
https://mail.kde.org/mailman/listinfo/kopete-devel
[prev in list] [next in list] [prev in thread] [next in thread]