<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/104363/">http://git.reviewboard.kde.org/r/104363/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 5th, 2012, 10 a.m., <b>Daniele Elmo Domenichelli</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;">Perhaps I didn't explain it correctly, I didn't mean to remove the enum completely, but just to read and write the "full" version
Anyway it's not a big deal it is not used much... but you should definitely have 3 const variables containing the string and use them instead of using QLatin1String(...) everywhere</pre>
</blockquote>
<p>On April 7th, 2012, 2:22 p.m., <b>Dominik Cermak</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'm pretty much confused...
AFAIK (but please correct me if I'm wrong) enums are integer only. To save it as a string in the config file I would need to do a conversion for reading and writing (enum -> string, string -> enum).
So using the enum as well as the strings only brings one more step:
for enum only (save as int) or strings only:
1. read value from config
2. compare to decide what to to
for strings and enum:
1. read value from config
2. convert to corresponding enum value
3. compare to decide what to do</pre>
</blockquote>
<p>On April 7th, 2012, 2:29 p.m., <b>Dominik Cermak</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;">Anyway it missed feature freeze for 0.4 so maybe just discard this one and redo it for 0.5 (handling different settings per account, solving the "2 relogins needed" problem and so on)?</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;">We are still in "soft freeze" that means, afaik, that rewiews pending can still be merged, so no problem in merging this if you finish it before the "hard freeze"
Yes, enums are "named" integer, inside the code it makes sense to use enums, but in the config file it does not make sense to me to save the integer, because the config file is supposed to be editable manually and an enum would make it difficult to understand
Another problem in doing that (even though it's quite unlikely to happen here) is that you have an enum
{ disabled, manual, enabled } you will have disabled=0 manual=1 enabled=2. If you save the number in your config file and then for some reason we decide that we need to add one or to reorder them, the values will change and when the user runs the program, the value read from the old config will be associated to the new value and therefore it will have another meaning...
With a string it takes some more steps, but you are sure that, no matter how you reorder the stuff inside, the associated value is always the same. Writing and reading config files is not a bottleneck where we have to optimize the code as much as we can, so I see no problem in doing it.
Anyway using just strings inside can be good as well, but just make them constant somewhere instead of writing them every time you use them, so that if we want to change the string, we don't have to do it everywere in the code, but just in one place. Moreover we could have other "manual" strings around the code, so a variable like autoLoginManual is a lot more expressive
</pre>
<br />
<p>- Daniele Elmo</p>
<br />
<p>On March 24th, 2012, 10:27 p.m., Dominik Cermak 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 Dominik Cermak.</div>
<p style="color: grey;"><i>Updated March 24, 2012, 10:27 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;">It's a simple approach to have this feature implemented.
It uses the ConnectsAutomatically property and the AutomaticPresence.
In the kcm you can enable/disable this feature (ConnectsAutomatically is set) and on every presence change the new presence is set as the AutomaticPresence.
This way mission-control cares for setting the saved presence as soon as possible.
Note: Disabling it takes only effect after the second login, I suspect something with the change notification between kcm and kded isn't working (that signal kcm should send and kded receive). So kded sets the property only after starting...</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=281929">281929</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>CMakeLists.txt <span style="color: grey">(c55fa8431464b62a5426a6cf29319c3f6f5fce6f)</span></li>
<li>autoconnect.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>autoconnect.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>config/telepathy-kded-config.cpp <span style="color: grey">(1fb515f56500407b7b221d1d5310237f8d94ca7f)</span></li>
<li>config/telepathy-kded-config.ui <span style="color: grey">(0d616c5503b22f37c0ae4d5a94411d0f97b25d38)</span></li>
<li>telepathy-module.h <span style="color: grey">(05d8c3847587049dcc4c9329a3b4d0b0bfee7d49)</span></li>
<li>telepathy-module.cpp <span style="color: grey">(955ceec3a8fdaacaf24463ac4e721d0a53086d30)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104363/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>