On Sonntag, 19. November 2017 21:18:11 CET Dirk Hohndel wrote: > > On Nov 19, 2017, at 4:07 AM, Berthold Stoeger <bstoe...@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 _______________________________________________ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface