Open GoogleCodeExporter opened 9 years ago
Hello, thank you very much for the contribution! This is the first open source
code
contribution, exciting stuff!
Here's some comments.
1. Auto refresh
I like the idea, but if there's a timer always running, how will this affect
CPU and
battery usage?
The timer should be stopped when the application is in the background or after
inactivity so the phone can enter sleep mode and minimise battery usage. It
should
only update and draw if it's in the foreground (and then it could also update
once
when it's brought to the foreground).
2. RSK "Hide"
Rather than renaming the existing "Refresh" string as "Hide", it should be
added as
a new string to all the loc files. That way the old string is still there if we
need
it again, and the translators can see the new one at the end.
3. Auto shutdown of all tasks
Auto shutdown of all tasks -- not sure about this, perhaps it should be an
option.
I'll have a think about it and ask some other users. (Another idea could be to
close
the connection, if this is possible, when exceeding the quota, but some apps
may
reconnect automatically.)
Here's some minor code review things:
* Only include once: #include <APGCLI.H>
* Automatic variables should be initialised instead of assigned,
e.g. TInt aAppCount = 0; -> TInt aAppCount(0);
* Put a space around operators,
e.g. iTasksClosed=EFalse; -> iTasksClosed = EFalse;
* Preincrement rather than postincrement,
e.g. for(TInt i=0;i<aAppCount;i++)
->
for(TInt i(0); i < aAppCount; ++i)
* Move this to the .cpp: const TInt KPeriodicTimerInterval(100000);
and add a comment e.g. // 1/10 second
* Define TBool iTasksClosed=EFalse; in the header and initialise in the constructor.
* No need for #include <APGCLI.H> in DataQuotaAppView.h.
* Rename EDataQuotaRefresh as EDataQuotaHide
Original comment by hugovk@gmail.com
on 2 May 2010 at 8:50
Hi hugovk. Thanks for the replay. I do agree the close all tasks should have an
option. For some users (as myself obviously) it may be considered as a very
basic
functionality but it may be annoying to others. Note that closing the
connection will
not help as many apps will reconnect as you said. Keeping that in mind the auto
refresh should always be active when this option is enabled. It is a very good
idea
thought to disable it on the background when the option is not enabled to save
some
battery. Please leave a comment or e-mail me at dafisalos@gmail.com if you need
any help.
Original comment by dafisa...@gmail.com
on 3 May 2010 at 6:11
Of course, you're right, the auto-refresh is needed when the option is enabled,
otherwise it can't do the auto-shutdown.
I did some testing with Nokia Energy Profiler, and it didn't seem to use too
much
battery when enabled. Though perhaps the interval could be safely increased
from
1/10 second to 1 second?
Please could you implement the option and attach a new patch? Thank you!
(PS, I've added you to the project as a contributor and put you as the owner
for
this issue.)
Original comment by hugovk@gmail.com
on 3 May 2010 at 7:11
Original issue reported on code.google.com by
dafisa...@gmail.com
on 1 May 2010 at 8:56Attachments: