Replace repetitive code in SettingsObjectWrapper.cpp by functions?

[Berthold Stoeger]

On Sonntag, 19. November 2017 21:18:11 CET Dirk Hohndel wrote:
> > On Nov 19, 2017, at 4:07 AM, Berthold Stoeger <bstoeger at mail.tuwien.ac.at> 
wrote:
> >>> 1) In getCloudURL() in core/qthelper.cpp, the
> >>> cloud_storate_email_encoded
> >>> member is set, but it seems not to be written to disk.
> >> 
> >> I looked at that code recently and am struggling with the overall logic
> >> of
> >> it. Right now I feel like I need to take a step back and really look at
> >> the
> >> overall logic of how we manage user name and password - because as I read
> >> the code as it is it confuses me (and I think I wrote a good part of
> >> this).
> > 
> > This one is not a bug, just a duplication of code issue. The reason is
> > that
> > cloud_storage_email_encoded is one of those fields that are not written to
> > disk in SettingsObjectWrapper.
> > 
> > I guess the more consistent variant would be calling the method in
> > SettingsObjectWrapper and not manipulating the pref struct directly. But
> > I'm not going to meddle with this.
> 
> I'm perfectly happy to have you work on this code and remove the code
> duplications. My request would be that you try to devise tests for this so
> we can make sure we understand what the functions are supposed to do and
> can verify that they actually do what they are supposed to do.

Ok, will have a look, but not very soon. First I'd have to get an overview on 
the whole cloud-storage functionality and my next week will be very busy. BTW, 
I changed my opinion, these fields (newPassword and emailEncoded) should be 
removed from SettingsObjectWrapper / prefs.h. A few comments in the code seem 
to agree. These are state of the cloud-storage machinery, not user settings/
preferences.
 
> Writing tests isn't a requirement for you to continue down this path - as I
> said, it's a request as I think it would be helpful.
> 
> >>> 2) In uploadFinished() in core/cloudstorage.cpp, the
> >>> cloud_storage_password
> >>> member is set, but it likewise seems not to be written to disk.
> >> 
> >> The password we should only store if that checkbox is ticked.
> > 
> > This one I have no idea - I would have to test it. And I'm not going to
> > touch it unless I have a reliable method of testing it.
> 
> So today we don't store the password unless you tick that box - that was a
> request from some of the testers back when we implemented cloud storage.
> Funnily enough, every time I install Subsurface on a new test machine or
> test VM I stumble across this. I enter credentials, forget to tick the box,
> and then get annoyed the next time I start and the password is missing...
> 
> So the only question here is "is there a situation where we SHOULD store
> the password (assuming the box is ticked) and today we don't?"

This is exactly the point. The usual password saving is via desktop-widgets/
preferences/preferences_network.cpp and mobile-widgets/qmlmanager.cpp, 
respectively. This clearly works.

As I said above, I'm too unfamiliar with the whole cloud-storage feature to 
make any sense of the code in uploadFinished(). One day I will try to find out. 
;)

> > What I got so far:
> > - If the password changed, uploadFinished() sets the new password in prefs
> > and emits the passordChangeSuccesful signal.
> > - This signal is connected to the passwordUpdateSuccesfull(sic) slot of
> > PreferencesNetwork.
> 
> Feel free to rename. Tomaz has done a lot of this code and as a Brazilian
> his English grammar is sometimes entertaining... (I love you, Tomaz).

This wasn't meant as a complaint. I'm a terrible speller myself, even in my 
native language. :P

Berthold