On April 5, 2012, 10 a.m., Dominik Cermak wrote:
> > 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
>
> Dominik Cermak wrote:
> 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
>
> Dominik Cermak wrote:
> 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)?
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
- Daniele Elmo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104363/#review12181
-----------------------------------------------------------
On March 24, 2012, 10:27 p.m., Dominik Cermak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104363/
> -----------------------------------------------------------
>
> (Updated March 24, 2012, 10:27 p.m.)
>
>
> Review request for Telepathy.
>
>
> Description
> -------
>
> 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...
>
>
> This addresses bug 281929.
> http://bugs.kde.org/show_bug.cgi?id=281929
>
>
> Diffs
> -----
>
> CMakeLists.txt c55fa8431464b62a5426a6cf29319c3f6f5fce6f
> autoconnect.h PRE-CREATION
> autoconnect.cpp PRE-CREATION
> config/telepathy-kded-config.cpp 1fb515f56500407b7b221d1d5310237f8d94ca7f
> config/telepathy-kded-config.ui 0d616c5503b22f37c0ae4d5a94411d0f97b25d38
> telepathy-module.h 05d8c3847587049dcc4c9329a3b4d0b0bfee7d49
> telepathy-module.cpp 955ceec3a8fdaacaf24463ac4e721d0a53086d30
>
> Diff: http://git.reviewboard.kde.org/r/104363/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dominik Cermak
>
>
_______________________________________________
KDE-Telepathy mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-telepathy
