Code review comment for lp:~rpadovani/reminders-app/multiple-accounts

> Hey Riccardo,
>
> nice. Congrats to your first real C++ branch :) Obviously there are a few
> issues to fix. But you know how picky I am. Don't blame yourself for the
> length of the list. I intentionally explained lots of things.
>

Thanks for your support and explanations :-)

> ====
>
> Hmm... Why do you save the username? That just makes it more complex.
>
> Instead of saving the username into the config, I would just save the token.
> This way, on startup we can just load the token from the config, set it to
> EvernoteConnection and not bother dealing with AccountService at all.
> Once the token is set, the username will be automatically refreshed inside the
> UserStore.
>

Well, I thinked is not secure to manage directly accounts without use AccountService, because we save accessToken, but if you say it's ok, I do in this way

> ====
>
> Here's a couple of other things I'd like to have changed:
>
> 134 CameraHelper helper;
> 135 view.engine()->rootContext()->setContextProperty("cameraHelper",
> &helper);
> 136
> 137 +
> view.engine()->rootContext()->setContextProperty("accountPreference", new
> AccountPreference());
>
> This works, but please align it to look the same as the CameraHelper. Apart
> from the visual difference, there is a theoretical issue with your code too:
> If you create a variable without "new", it will be allocated on the stack and
> automatically deleted when the function ends. If you do "new" you will
> allocate the object on the heap and need to delete it yourself. Now, given
> that the program is removed from the memory when the main() ends, this is
> really a theoretical issue here and doesn't really hurt us in practice. But I
> hope you get the idea and see the benefit of keeping things consistent with
> the existing code.
>

Done, thanks for explanation!

> ===
[...]
> As said above, this should be more like:
>
> if (accountPreferences.token) {
> EvernoteConnection.token = accountPreferences.token;
> } else {
> // Do the AccountService stuff...
> }

Ok, I implemented that, but now if accountPreferences.accountToken is not a valid token we have an authentication error, because there is no time to intercept UserStore.username, it is set after else is executed.
So I have no idea on how check if accountPreferences.token is not valid.
This isn't an uncommon user case: login with an account, user removes the account from Online Account, accountToken is no more valid.

> ====
>
> 372 +void EvernoteConnection::clearToken()
> 373 +{
> 374 + if (!EvernoteConnection::instance()->token().isEmpty()) {
> 375 + setToken(NULL);
> 376 + emit tokenChanged();
> 377 + }
> 378 +}
>
> Heh, you're inside EvernoteConnection, you can directly access m_token instead
> of going through EvernoteConnection::instance()->token(). Also, never use NULL
> if a QString parameter is required. And one more: setToken() already does the
> emit tokenChanged(), you're emitting that twice now.
>
> This function could (and should) be as short as:
>
> void EvernoteConnection::clearToken()
> {
> m_token.clear();
> emit tokenChanged();
> }
>
> or:
>
> void EvernoteConnection::clearToken()
> {
> setToken(QString());
> }
>
> I guess the latter one is the better one.
>

Done, thanks for explanations. I'll try to learn C++ step by step :-)

> ====
>
> 56 \ No newline at end of file
>
> Make sure C++ code files always end with a newline. While this is again a
> theoretical issue in our case, it breaks compilation with some compilers, e.g.
> on Solaris.

Ok, thanks, I forced my text editor to add newline at the end of C++ files...

>
> ====
>
> 283 + text: i18n.tr("Accounts")
> 284 + iconName: "contacts-app-symbolic"
> 285 + visible: accounts.count
> 286 + onTriggered: {
> 287 + openAccountPage(true);
> 288 + }
> 289 + }
> 290 +
> 291 + ToolbarButton {
>
> I personally don't think its good to have the button in the toolbar all the
> time, given that in practice you will never ever use it :)
>
> How about putting it into the HUD? David, Dani, Alan, what's your opinion on
> this?

Mhh, you got a point, but I think that in the HUD is too hide. I have not modify this at the moment, but I'll do as soon as someone else give us an opinion!