<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103628/">http://git.reviewboard.kde.org/r/103628/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 4th, 2012, 3:52 p.m., <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">By looking at the screenshot, it's not really clear what is that klineedit and what is it actually for. I quickly scrolled through the code and I saw no click-message either, so I assume that the text will simply appear there (even letter by letter)? I'd like to see some label there. Also the icon looks wrong. I know you said it looks wrong on the right too, but currently it looks just randomly placed, it doesn't align with the horizontal edges/visual guidelines. How about moving the icon on the top right, making it bigger and putting big "Jabber" next to it (maybe bold? though that's generaly bad) and another line below it saying something like "You're about to create a jabber account". Then make the display name part of the form below, but label it with something like "Account name".
And don't forget that we're in freeze right now ;)</pre>
</blockquote>
<p>On January 4th, 2012, 3:53 p.m., <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I meant top left, not top right, sorry :)</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The lineedit has a "Placeholder Text" that at the moment shows "Displayed Name", so that's what the user will see when he adds a new account, then yes, the text will appear letter by letter. The placeholder also appears if the string is empty. Moreover if the user leaves it empty he will receive an error. When the account is added, whatever the user chose is shown in the main window.
You cannot set the text "You're about to create a jabber account", because the same widget is used both for creating and editing the account
In my opinion it must be clear to the user that the display name does _not_ belong to the form below, since is a personal preference and not a parameter to connect to the account (that's why I used the frame). Moreover, the org.freedesktop.Telepathy.Account interface supports setting the icon property, so in the future we could let the user choose the icon or do something like kopete that lets you choose the colour of the icon, useful to distingush when you have several accounts using the same protocol (i.e. jabber).
Anyway, I'd leave the UI fixing for later (and possibly to someone else, since I really suck at that) and focus this review on the "core" of the display name stuff.
</pre>
<br />
<p>- Daniele Elmo</p>
<br />
<p>On January 4th, 2012, 3:39 p.m., Daniele Elmo Domenichelli wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Telepathy.</div>
<div>By Daniele Elmo Domenichelli.</div>
<p style="color: grey;"><i>Updated Jan. 4, 2012, 3:39 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch makes the display name editable and let each plugin generate a "default" display name, based on the parameters set.
- Default name is used if display name is empty or if it is the previous default name.
- If the current display name _contains_ the old default display name, only the default part is replaced. For example, if old default name is foo@bar.com and the display name is Foo <foo@bar.com> when the user changes something and the new default display name is foo@buz.com, the new display name becomes Foo <foo@buz.com> (This happens realtime while the user is typing)
- Otherwise the user set display name is just left unchanged.
Also all the plugins were updated. For accounts without a specific plugin, the display name is set automatically using the "account" parameter if it exists. If it does not exist the user will have to add it manually, but this doesn't seem a big issue to me...
Branch here: http://quickgit.kde.org/?p=clones%2Ftelepathy-accounts-kcm%2Fddomenichelli%2Ftelepathy-accounts-kcm.git&a=shortlog&h=refs/heads/displayname
(Note: Of course this patch won't be merged until master is re-opened)
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Created an account and modified the display name.
Edited an account and modified the display name.
Created salut account and edited using the dialog.
Edited salut account.
Tested most of the plugins.
More random tests.
</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=284930">284930</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plugins/butterfly/main-options-widget.h <span style="color: grey">(3b1f264b043540303d8b1e7df846411f78019de8)</span></li>
<li>plugins/butterfly/main-options-widget.cpp <span style="color: grey">(7e4800717673b6ee7d549bae7afbf3126e0a28d4)</span></li>
<li>plugins/gabble/main-options-widget-facebook.h <span style="color: grey">(4653b7304672716a41fa3f3afc3f33fd39daf665)</span></li>
<li>plugins/gabble/main-options-widget-facebook.cpp <span style="color: grey">(7e800bef0c6a1bf99bbfea84d3cb411c0ebac280)</span></li>
<li>plugins/gabble/main-options-widget-googletalk.h <span style="color: grey">(b73df1129648dedb084af2d1f8ee056d06fa6bd4)</span></li>
<li>plugins/gabble/main-options-widget-googletalk.cpp <span style="color: grey">(ed97bffb0dab256af5ab2a17ad1279094e49092b)</span></li>
<li>plugins/gabble/main-options-widget-msn.h <span style="color: grey">(75ccef3324aa07963f3816083133f7cc38f79595)</span></li>
<li>plugins/gabble/main-options-widget-msn.cpp <span style="color: grey">(913a808810b2368850657bc4f76edfa4d7d1f681)</span></li>
<li>plugins/gabble/main-options-widget.h <span style="color: grey">(5443c28414c50945ea169fb585ba08b30d873169)</span></li>
<li>plugins/gabble/main-options-widget.cpp <span style="color: grey">(927bb32f1e29a7e4e7dbed64ed4eb7df56e96014)</span></li>
<li>plugins/haze/aim-main-options-widget.h <span style="color: grey">(2b98548eef266cb1b764a7b6f135d718e8402c1a)</span></li>
<li>plugins/haze/aim-main-options-widget.cpp <span style="color: grey">(bc19d9ce83d8da6695551125ef83b96aade3a9cf)</span></li>
<li>plugins/haze/icq-main-options-widget.h <span style="color: grey">(89574e788c159f7a214d3360a6a6f28edd7684d9)</span></li>
<li>plugins/haze/icq-main-options-widget.cpp <span style="color: grey">(b6dcbae0d419c732d9803b02410890774a683708)</span></li>
<li>plugins/haze/msn-main-options-widget.h <span style="color: grey">(56ffc3039f92e9d1912bc396ac85de330b5ebf4f)</span></li>
<li>plugins/haze/msn-main-options-widget.cpp <span style="color: grey">(3bab7912896db5fd7d08a5407f4ecf4a615a743a)</span></li>
<li>plugins/haze/myspaceim-main-options-widget.h <span style="color: grey">(554fc5100d46b8730a89f15912dd239a87938932)</span></li>
<li>plugins/haze/myspaceim-main-options-widget.cpp <span style="color: grey">(94e475de6f89d5095bbcc9798b144600042e87cb)</span></li>
<li>plugins/haze/skype-main-options-widget.h <span style="color: grey">(5e7082ce186301762f9931568ed56c7109f828fa)</span></li>
<li>plugins/haze/skype-main-options-widget.cpp <span style="color: grey">(87a3448601b2f338ca9d7361424b7a926c2b14ee)</span></li>
<li>plugins/haze/yahoo-main-options-widget.h <span style="color: grey">(72a3b304152885641e91d7e148c1fc586076a8d2)</span></li>
<li>plugins/haze/yahoo-main-options-widget.cpp <span style="color: grey">(a3e4f7cabfbd617f185d48a25ae07aacd69e7a1d)</span></li>
<li>plugins/idle/main-options-widget.h <span style="color: grey">(1f5120f0244fb3d811fd39bd33354d0b049942d6)</span></li>
<li>plugins/idle/main-options-widget.cpp <span style="color: grey">(f8aeb4b7bb777cc9b5973271344d1ebaac78aa52)</span></li>
<li>plugins/rakia/rakia-main-options-widget.h <span style="color: grey">(bd1291b8318dbc468b005a54315a9365aaf35b0c)</span></li>
<li>plugins/rakia/rakia-main-options-widget.cpp <span style="color: grey">(ecb4ee4ef4b23869b3f73077e49a5bd3ed7f035e)</span></li>
<li>plugins/salut/salut-main-options-widget.h <span style="color: grey">(ab51e1901340ab8630ba19e50b49c77eeb5ecd2d)</span></li>
<li>plugins/salut/salut-main-options-widget.cpp <span style="color: grey">(64b41674482c25b77f0ae289213db827dc820323)</span></li>
<li>plugins/sunshine/sunshine-main-options-widget.h <span style="color: grey">(fbb6dca508d9bf7b313c4b14dcd1c2097cc772d5)</span></li>
<li>plugins/sunshine/sunshine-main-options-widget.cpp <span style="color: grey">(21a74b76444416072291a929f5f3e10e63577793)</span></li>
<li>src/KCMTelepathyAccounts/abstract-account-parameters-widget.h <span style="color: grey">(0c6cd95db610b7cebebe3e06ca15b446fd7d024d)</span></li>
<li>src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp <span style="color: grey">(b61e836b77809819a0f6f24f9b9b1bfe2a3af6a6)</span></li>
<li>src/KCMTelepathyAccounts/account-edit-widget.h <span style="color: grey">(5db8119dfae2a31e51fca60daf40b6b083359f6f)</span></li>
<li>src/KCMTelepathyAccounts/account-edit-widget.cpp <span style="color: grey">(312a4c913394c39e1b131994fbc0a8d02b84d5df)</span></li>
<li>src/KCMTelepathyAccounts/account-edit-widget.ui <span style="color: grey">(0ffd782fce025d430decd0b71dba36e5cf7422f0)</span></li>
<li>src/KCMTelepathyAccounts/parameter-edit-widget.h <span style="color: grey">(0e92bb7df35870778b339fb83e2ddb232e9abb6f)</span></li>
<li>src/KCMTelepathyAccounts/parameter-edit-widget.cpp <span style="color: grey">(fc666c5c760ae31672fbefc332217ff68a7cd633)</span></li>
<li>src/add-account-assistant.cpp <span style="color: grey">(3f189b440ddf26af4092af6ac11e247838087254)</span></li>
<li>src/edit-account-dialog.cpp <span style="color: grey">(37cb28faacbf63160eaa116924d015d5e59ed1ee)</span></li>
<li>src/salut-details-dialog.h <span style="color: grey">(804d61b903bdf8c0946b1960aedac5b63b5b1b97)</span></li>
<li>src/salut-details-dialog.cpp <span style="color: grey">(f95826dcaa7b1fda96a695bd5f23ba9a3d4ffa63)</span></li>
<li>src/salut-enabler.h <span style="color: grey">(7d4d640d19509024b9da269cb061ba43d2896dd9)</span></li>
<li>src/salut-enabler.cpp <span style="color: grey">(8cb1e03c9bb41df9f579ab15a319959b61a70e46)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103628/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/103628/s/403/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/01/04/snapshot35_400x100.png" style="border: 1px black solid;" alt="Editable display name" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>