[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-telepathy
Subject: Re: Review Request: Make display name editable
From: "Daniele Elmo Domenichelli" <daniele.domenichelli () gmail ! com>
Date: 2012-01-04 18:02:35
Message-ID: 20120104180235.23835.42334 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Jan. 4, 2012, 3:52 p.m., Martin Klapetek wrote:
> > 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 ;)
>
> Martin Klapetek wrote:
> I meant top left, not top right, sorry :)
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.
- Daniele Elmo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103628/#review9534
-----------------------------------------------------------
On Jan. 4, 2012, 3:39 p.m., Daniele Elmo Domenichelli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103628/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2012, 3:39 p.m.)
>
>
> Review request for Telepathy.
>
>
> Description
> -------
>
> 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)
>
>
> This addresses bug 284930.
> http://bugs.kde.org/show_bug.cgi?id=284930
>
>
> Diffs
> -----
>
> plugins/butterfly/main-options-widget.h \
> 3b1f264b043540303d8b1e7df846411f78019de8 \
> plugins/butterfly/main-options-widget.cpp \
> 7e4800717673b6ee7d549bae7afbf3126e0a28d4 \
> plugins/gabble/main-options-widget-facebook.h \
> 4653b7304672716a41fa3f3afc3f33fd39daf665 \
> plugins/gabble/main-options-widget-facebook.cpp \
> 7e800bef0c6a1bf99bbfea84d3cb411c0ebac280 \
> plugins/gabble/main-options-widget-googletalk.h \
> b73df1129648dedb084af2d1f8ee056d06fa6bd4 \
> plugins/gabble/main-options-widget-googletalk.cpp \
> ed97bffb0dab256af5ab2a17ad1279094e49092b \
> plugins/gabble/main-options-widget-msn.h \
> 75ccef3324aa07963f3816083133f7cc38f79595 \
> plugins/gabble/main-options-widget-msn.cpp \
> 913a808810b2368850657bc4f76edfa4d7d1f681 \
> plugins/gabble/main-options-widget.h \
> 5443c28414c50945ea169fb585ba08b30d873169 \
> plugins/gabble/main-options-widget.cpp \
> 927bb32f1e29a7e4e7dbed64ed4eb7df56e96014 \
> plugins/haze/aim-main-options-widget.h \
> 2b98548eef266cb1b764a7b6f135d718e8402c1a \
> plugins/haze/aim-main-options-widget.cpp \
> bc19d9ce83d8da6695551125ef83b96aade3a9cf \
> plugins/haze/icq-main-options-widget.h \
> 89574e788c159f7a214d3360a6a6f28edd7684d9 \
> plugins/haze/icq-main-options-widget.cpp \
> b6dcbae0d419c732d9803b02410890774a683708 \
> plugins/haze/msn-main-options-widget.h \
> 56ffc3039f92e9d1912bc396ac85de330b5ebf4f \
> plugins/haze/msn-main-options-widget.cpp \
> 3bab7912896db5fd7d08a5407f4ecf4a615a743a \
> plugins/haze/myspaceim-main-options-widget.h \
> 554fc5100d46b8730a89f15912dd239a87938932 \
> plugins/haze/myspaceim-main-options-widget.cpp \
> 94e475de6f89d5095bbcc9798b144600042e87cb \
> plugins/haze/skype-main-options-widget.h \
> 5e7082ce186301762f9931568ed56c7109f828fa \
> plugins/haze/skype-main-options-widget.cpp \
> 87a3448601b2f338ca9d7361424b7a926c2b14ee \
> plugins/haze/yahoo-main-options-widget.h \
> 72a3b304152885641e91d7e148c1fc586076a8d2 \
> plugins/haze/yahoo-main-options-widget.cpp \
> a3e4f7cabfbd617f185d48a25ae07aacd69e7a1d \
> plugins/idle/main-options-widget.h \
> 1f5120f0244fb3d811fd39bd33354d0b049942d6 \
> plugins/idle/main-options-widget.cpp \
> f8aeb4b7bb777cc9b5973271344d1ebaac78aa52 \
> plugins/rakia/rakia-main-options-widget.h \
> bd1291b8318dbc468b005a54315a9365aaf35b0c \
> plugins/rakia/rakia-main-options-widget.cpp \
> ecb4ee4ef4b23869b3f73077e49a5bd3ed7f035e \
> plugins/salut/salut-main-options-widget.h \
> ab51e1901340ab8630ba19e50b49c77eeb5ecd2d \
> plugins/salut/salut-main-options-widget.cpp \
> 64b41674482c25b77f0ae289213db827dc820323 \
> plugins/sunshine/sunshine-main-options-widget.h \
> fbb6dca508d9bf7b313c4b14dcd1c2097cc772d5 \
> plugins/sunshine/sunshine-main-options-widget.cpp \
> 21a74b76444416072291a929f5f3e10e63577793 \
> src/KCMTelepathyAccounts/abstract-account-parameters-widget.h \
> 0c6cd95db610b7cebebe3e06ca15b446fd7d024d \
> src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp \
> b61e836b77809819a0f6f24f9b9b1bfe2a3af6a6 \
> src/KCMTelepathyAccounts/account-edit-widget.h \
> 5db8119dfae2a31e51fca60daf40b6b083359f6f \
> src/KCMTelepathyAccounts/account-edit-widget.cpp \
> 312a4c913394c39e1b131994fbc0a8d02b84d5df \
> src/KCMTelepathyAccounts/account-edit-widget.ui \
> 0ffd782fce025d430decd0b71dba36e5cf7422f0 \
> src/KCMTelepathyAccounts/parameter-edit-widget.h \
> 0e92bb7df35870778b339fb83e2ddb232e9abb6f \
> src/KCMTelepathyAccounts/parameter-edit-widget.cpp \
> fc666c5c760ae31672fbefc332217ff68a7cd633 src/add-account-assistant.cpp \
> 3f189b440ddf26af4092af6ac11e247838087254 src/edit-account-dialog.cpp \
> 37cb28faacbf63160eaa116924d015d5e59ed1ee src/salut-details-dialog.h \
> 804d61b903bdf8c0946b1960aedac5b63b5b1b97 src/salut-details-dialog.cpp \
> f95826dcaa7b1fda96a695bd5f23ba9a3d4ffa63 src/salut-enabler.h \
> 7d4d640d19509024b9da269cb061ba43d2896dd9 src/salut-enabler.cpp \
> 8cb1e03c9bb41df9f579ab15a319959b61a70e46
> Diff: http://git.reviewboard.kde.org/r/103628/diff/diff
>
>
> Testing
> -------
>
> 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.
>
>
> Screenshots
> -----------
>
> Editable display name
> http://git.reviewboard.kde.org/r/103628/s/403/
>
>
> Thanks,
>
> Daniele Elmo Domenichelli
>
>
[Attachment #5 (text/html)]
<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%2Fdd \
omenichelli%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>
_______________________________________________
KDE-Telepathy mailing list
KDE-Telepathy@kde.org
https://mail.kde.org/mailman/listinfo/kde-telepathy
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic