Move default autoaccept dir

[sphaerophoria]

Closes #5375

Alright this one ended up being much more complicated than I thought, handling upgrade of settings is non-trivial. That being said I think I came up with an okay solution.

On first run with the change the user sees

if they previously set the autoaccept direcotry

Then for each friend that they previously enabled upgrade on they see

That's the major idea here, rest of the changes are in the commit message (pasted here for convenience)

feat(transfer): Change default autoaccept directory

• Implements clean upgrades to the new defaults by requesting user
  approval to use new defaults over previously set values
• Changes default global autoaccept directory to appDataDir()/transfers
• Changes each default user autoaccept directory to
  appDataDir()/transfrers/hashed_friend_public_key
• Adds buttons in the about friend dialog and general settings dialog to
  open download directory
• Decouples friend autoaccept from autoaccept dir
  • Interaction with global autoaccept is still broken but that will
    be resolved in Replace global autoaccept with per-conversation autoaccept prompt #5376
• Adds ability to reset friend autoaccept dir to default in the about
  friend dialog
• Images are now saved to the per-user transfer directory instead of the global
  images folder

Screenshots of new UI elements

Given that this touches the same UI elements as #5373 the UI elements may not merge cleanly, in that case just reassign whichever goes in second to me and I can update them.

Tests I ran before pushing

• Global and User auto accept set

  • Do not accept global auto accept upgrade
    • Do not accept user auto accept upgrade
      • Downloads still go to user download dir
    • Accept user auto accept upgrade
      • Downlaods go to global download dir / hashed id
        • This is reasoanble behavior since the new default user download dir is supposed to be relative to the global one
  • Accept global auto accept upgrade
    • Do not accept user auto accept upgrade
      • Downloads go to user download dir
    • Accept user auto accept upgrade
      • Downloads go to new global download dir / hashed id

• Global set and User not set

  • Do not accept global upgrade
    • User upgrade not prompted
      • Files downloaded to global download dir
        • This interation is a little undesirable but will be cleaned up in Replace global autoaccept with per-conversation autoaccept prompt #5376
    • Enable autoaccept for user
      • Files downloaded to global download dir / hashed id
        • This interation is a little undesirable but will be cleaned up in Replace global autoaccept with per-conversation autoaccept prompt #5376
  • Accept global upgrade
    • User upgrade not prompted
      • Files downloaded to global download dir
        • This interation is a little undesirable but will be cleaned up in Replace global autoaccept with per-conversation autoaccept prompt #5376
    • Enable autoaccept for user
      • Files downloaded to global download dir / hashed id
        • This interation is a little undesirable but will be cleaned up in Replace global autoaccept with per-conversation autoaccept prompt #5376

• New Program Run (no qtox.ini)

  • No prompts
  • Default global autoaccept dir set to new global download dir
  • Same interactions as above

• Open directory in friend about page now opens user download dir

• Reset resets friend download dir to global dir / hashed id

• Download dir follows global dir change if default/reset

Tests can be run by opening a user profile after downgrading qtox, this should reset behavior so you can experience the upgrade logic again :) (with the side affect of disabling autoaccept for all users)

---

This change is 

[sudden6]

Reviewed 12 of 22 files at r1.
Reviewable status: 0 of 1 LGTMs obtained

---

src/persistence/profile.cpp, line 767 at r1 (raw file):

}

QString Profile::hashedFriendId(const QString& ownerStr) const

Please document what this function does. Especially the part that the generated hash is dependent on our own ToxPk. Since this function is used on ToxPk only it would make sense to pass a ToxPk instead of QString.

---

src/persistence/settings.h, line 695 at r1 (raw file):

    int themeColor;

    static QMutex bigLock;

why remove these from the header?

---

src/persistence/settings.cpp, line 62 at r1 (raw file):

namespace {
const QString globalSettingsFile = "qtox.ini";

QStringLiteral?

---

src/persistence/settings.cpp, line 66 at r1 (raw file):

QMutex bigLock{QMutex::Recursive};
QThread* settingsThread{nullptr};
char const* autoAcceptUpgradeMessage =

These messages must be translateable, use QString and tr(...)

---

src/persistence/settings.cpp, line 73 at r1 (raw file):

    "New Default: %1\n"
    "Old Value: %2";
char const* friendAutoAcceptUpgradeMessage =

same

---

src/persistence/settings.cpp, line 427 at r1 (raw file):

                            .arg(newDefaultTransfersDir)
                            .arg(oldAutoAcceptDir)
                            // Unfortunately the best we can do to display some sort of

I think we save the nickname somewhere, because even if a friend is offline the correct name is displayed, but I'm also not sure how to get it here.

I really don't like displaying just the ToxPk here, because that's not very informative. I'd prefer something like " (ToxPk)"

---

src/persistence/settings.cpp, line 1480 at r1 (raw file):

    auto it = friendLst.find(id.getKey());
    if (it == friendLst.end())

braces

---

src/persistence/settings.cpp, line 1516 at r1 (raw file):

    auto it = friendLst.find(id.getKey());
    if (it == friendLst.end())

braces

---

src/persistence/settings.cpp, line 1519 at r1 (raw file):

        return QString();

    if (it->autoAcceptDir.isEmpty())

braces

---

src/persistence/upgradablesetting.h, line 39 at r1 (raw file):

public:
    /**
     * Helps construct our UpgradableSetting without a lengthly constructor

typo lengthy?

---

src/persistence/upgradablesetting.h, line 118 at r1 (raw file):

        UpgradableSetting setting;
        /// Tracking variable to ensure that the caller has set all mandatory fields before building
        /// the UpgradableSetting. Only use at time of writing is to assert on the build() fundtion

typo function

---

src/persistence/upgradablesetting.h, line 124 at r1 (raw file):

    /**
     * @brief Requests upgrade of setting to the user. This will only be
     * prompted once per settingand only if the previous value has already

typo setting and

---

src/persistence/upgradablesetting.h, line 127 at r1 (raw file):

     * been set
     * @param[in] message Message to display
     * @return true if the upgrade was successful

function returns void

---

src/persistence/upgradablesetting.cpp, line 26 at r1 (raw file):

bool UpgradableSettingDetail::doUpgradeRequest(QString const& message)
{
    auto response = QMessageBox::question(nullptr, "Upgrade Request", message);

use tr(...) to make the string translatebable

---

src/widget/friendwidget.cpp, line 293 at r1 (raw file):

                                          tr("Choose an auto accept directory", "popup title"),
                                          oldDir);
    if (!newDir.isEmpty())

braces

---

src/widget/about/aboutfriendform.cpp, line 88 at r1 (raw file):

{
    const QString dir = getAutoAcceptDir(about->getAutoAcceptDir());
    if (dir.isNull())

braces

[sphaerophoria]

Reviewable status: 0 of 1 LGTMs obtained

---

src/persistence/profile.cpp, line 767 at r1 (raw file):

Previously, sudden6 wrote…

Please document what this function does. Especially the part that the generated hash is dependent on our own ToxPk. Since this function is used on ToxPk only it would make sense to pass a ToxPk instead of QString.

I used a QString for a couple reasons

1. In the avatar function we've already converted the ToxPk to a QString
2. At the time of hashing on settings init we don't have our PK in ToxPk form, it seems silly to convert to a ToxPk just to immediately translate it back to a QString.

Will document

---

src/persistence/settings.h, line 695 at r1 (raw file):

Previously, sudden6 wrote…

why remove these from the header?

There's a pattern ingrained in my head to try to keep as much out of the header as possible, I was adding a couple constants and thought it made more sense for these to be scoped to the translation unit instead of being leaked into the header. This way if we need to add more static vars you only need to recompile one translation unit instead of all of them.

It's not a big deal, I can put them back if you'd like

---

src/persistence/settings.cpp, line 66 at r1 (raw file):

Previously, sudden6 wrote…

These messages must be translateable, use QString and tr(...)

If you don't mind I'd prefer to leave them as const chars here and then QString + tr them below when we use them. That way we don't have the extra allocations just sitting around doing nothing.

---

src/persistence/settings.cpp, line 427 at r1 (raw file):

Previously, sudden6 wrote…

I think we save the nickname somewhere, because even if a friend is offline the correct name is displayed, but I'm also not sure how to get it here.

I really don't like displaying just the ToxPk here, because that's not very informative. I'd prefer something like " (ToxPk)"

Yeah I was really unhappy with this. I thought the same thing but couldn't find where it gets set from. I looked around and could only find it in Core::getPeerName. Maybe my navigation is bad though, do you know where we store it?

Another option is to delay the upgrade until the first retrieval of the value. That way it's much more likely that we'll be fully initialized.

---

src/persistence/upgradablesetting.h, line 39 at r1 (raw file):

Previously, sudden6 wrote…

typo lengthy?

Yup

[sudden6]

I'm not sure if it's a good idea to prompt the user on every settings change to move the directory. IMO it would suffice to present a dialog which says "The qTox default save directory has moved to ..." or similar once on upgrading.

[sphaerophoria]

I'm not sure if it's a good idea to prompt the user on every settings change to move the directory. IMO it would suffice to present a dialog which says "The qTox default save directory has moved to ..." or similar once on upgrading.

Yeah i agree usability sucks especially if people have changed defaults on many contacts. However, I think the defaults currently are in a state where it will be quite common for a user to have changed them. In this case I think we should have some way of saying "hey we know you probably didnt want your downloads sitting in your home directory, but weve put them somewhere else now, would you like to reset to our new defaults or did you actually want to put those files there?"

Id personally prefer more prompts once than being in the wrong state forever

[sphaerophoria]

Reviewable status: 0 of 1 LGTMs obtained

---

src/persistence/settings.cpp, line 62 at r1 (raw file):

Previously, sudden6 wrote…

QStringLiteral?

Done.

---

src/persistence/settings.cpp, line 66 at r1 (raw file):

Previously, sphaerophoria wrote…

If you don't mind I'd prefer to leave them as const chars here and then QString + tr them below when we use them. That way we don't have the extra allocations just sitting around doing nothing.

Done, left as a const char* here and added tr below

---

src/persistence/settings.cpp, line 73 at r1 (raw file):

Previously, sudden6 wrote…

same

Done. Same as above

---

src/persistence/settings.cpp, line 1480 at r1 (raw file):

Previously, sudden6 wrote…

braces

Done.

---

src/persistence/settings.cpp, line 1516 at r1 (raw file):

Previously, sudden6 wrote…

braces

Done.

---

src/persistence/upgradablesetting.h, line 39 at r1 (raw file):

Previously, sphaerophoria wrote…

Yup

Done.

---

src/persistence/upgradablesetting.h, line 118 at r1 (raw file):

Previously, sudden6 wrote…

typo function

Done.

---

src/persistence/upgradablesetting.h, line 124 at r1 (raw file):

Previously, sudden6 wrote…

typo setting and

Done.

---

src/persistence/upgradablesetting.h, line 127 at r1 (raw file):

Previously, sudden6 wrote…

function returns void

Done.

---

src/persistence/upgradablesetting.cpp, line 26 at r1 (raw file):

Previously, sudden6 wrote…

use tr(...) to make the string translatebable

Done.

---

src/widget/friendwidget.cpp, line 293 at r1 (raw file):

Previously, sudden6 wrote…

braces

Done.

---

src/widget/about/aboutfriendform.cpp, line 88 at r1 (raw file):

Previously, sudden6 wrote…

braces

Done.

[sphaerophoria]

Reviewable status: 0 of 1 LGTMs obtained

---

src/persistence/profile.cpp, line 767 at r1 (raw file):

Previously, sphaerophoria wrote…

I used a QString for a couple reasons

1. In the avatar function we've already converted the ToxPk to a QString
2. At the time of hashing on settings init we don't have our PK in ToxPk form, it seems silly to convert to a ToxPk just to immediately translate it back to a QString.

Will document

Done.

---

src/persistence/settings.cpp, line 1519 at r1 (raw file):

Previously, sudden6 wrote…

braces

Done.

[sphaerophoria]

@sudden6 What do you think about a single messagebox popup when you log in with checkboxes for each user?

Upgrade flow would be

• Open qTox
• Possibly prompt for default download dir change (if it's non-null)
• Login
• Possibly prompt for user default autoaccept dirs change
  • Messagebox contains a checkbox for each user, each row would have the username, new dir, and old dir.
  • Messagebox does not show up if no users have their autoaccept dir set

[sudden6]

Reviewed 19 of 22 files at r3.
Reviewable status: 0 of 1 LGTMs obtained

---

src/persistence/settings.cpp, line 427 at r1 (raw file):

Previously, sphaerophoria wrote…

Yeah I was really unhappy with this. I thought the same thing but couldn't find where it gets set from. I looked around and could only find it in Core::getPeerName. Maybe my navigation is bad though, do you know where we store it?

Another option is to delay the upgrade until the first retrieval of the value. That way it's much more likely that we'll be fully initialized.

I have to research myself

---

src/widget/form/chatform.cpp, line 341 at r3 (raw file):

    const Settings& settings = Settings::getInstance();
    auto pk = f->getPublicKey();

const? 2x?

[sudden6]

I think the checkbox thing is great, maybe we can use a list where the top element allows to enable/disable all?

[sphaerophoria]

I'll play around with it and let you know what I think. Seems likely that an enable/disable all will be there :).

[sphaerophoria]

I've amended my PR, but haven't had time to test it thoroughly... Mostly looking for feedback on how to make this dialog not suck.

It's a lot closer to reasonable, but I'm not really sure what to do to make it more reasonable. I'll fiddle with it a little more tomorrow but if someone with a little more Qt experience has time to check out upgradablesetting.cpp and tell me what I'm doing wrong that would be much appreciated.

I'm not even sure what I don't like (other than the checkbox alignment), just the whole thing looks bad to me.

[sphaerophoria]

@sudden6 on a second look I must have been quite out of it when I looked for username before... It's very apparently in the constructor of Friend(). Looks like as long as the core is up we can get our friend name with Core::getInstance()->getPeerName(friendPk) so don't worry about looking for it yourself :).

[sudden6]

I would replace the ToxID with the username and make the ToxID tooltip only.

[sphaerophoria]

This is what I have so far. I think I'm willing to commit that, anyone else have any thoughts?

[sphaerophoria]

Alright I've pushed what I had above. Changes from last time

• UpgradableSetting has been fairly overhauled
  • Shows a modal dialog if there's 1 item
  • Shows a list with the changes if there's 2 or more
  • Should get something similar to https://user-images.githubusercontent.com/1069811/46920910-bbae8700-cfa9-11e8-92ea-1f93723c6572.png with 2+ items, and something similar to https://user-images.githubusercontent.com/1069811/46656627-57339800-cb63-11e8-8253-04fe32f4b9a4.PNG if there's only 1
• Fixed up the wrong directory being used when a user manually accepted a file transfer

[sudden6]

Does the list turn into a scrollable list if there are many friends? I don't see any code for that.

[sphaerophoria]

Does the list turn into a scrollable list if there are many friends? I don't see any code for that.

That's a pretty big oversight on my part...

[sphaerophoria]

Fixed. New layouts

Thanks for the feedback @sudden6 :).

[sudden6]

Reviewed 19 of 23 files at r5.
Reviewable status: 0 of 1 LGTMs obtained

---

src/widget/form/settings/generalform.cpp, line 261 at r5 (raw file):

    auto newMaxSizeMB = bodyUI->maxAutoAcceptSizeMB->value();
    auto newMaxSizeB = std::lround(newMaxSizeMB * 1024 * 1024);
    

whitespace

[sudden6]

Code mostly looks good, need to do a bit of testing though.

Also we should merge this PR after #5397, because in that PR I removed the "Tox" organisation, which changes the appdatadir for qTox.

[sphaerophoria]

Reviewable status: 0 of 1 LGTMs obtained

---

src/widget/form/chatform.cpp, line 341 at r3 (raw file):

Previously, sudden6 wrote…

const? 2x?

Done.

---

src/widget/form/settings/generalform.cpp, line 261 at r5 (raw file):

Previously, sudden6 wrote…

whitespace

Done.

[sphaerophoria]

@sudden6 as far as I understand there's no reason for #5397 to block this since the "Tox" organization didn't change any directories. I use the Settings directory getters which were untouched

[sudden6]

oh, then I think this PR should use the paths provided by QStandardPath so we create the file transfer folder in the correct place from the beginning.

[sphaerophoria]

oh, then I think this PR should use the paths provided by QStandardPath so we create the file transfer folder in the correct place from the beginning.

I don't see why it would be any harder to move it later with everything else vs now. As far as I know we haven't committed to using QStandardPath and there are commented reasons as to why we do things they way we do. I would vote for not mangling the two concepts and just move these paths with everything else if we ever do change the paths in the Settings::get<>DirPath calls. If we do it now any differences between the two could cause some weird filesystem layouts that will be harder to fix later.

[sudden6]

If we move it later, we have to write update code again (and annoy the users). Also, I don't want to move the settings file, just create the transfers/* file tree in the correct place from the beginning.

I'm not sure of the exact case for windows, but on linux qTox would store data in .config/qTox which is wrong. By using AppDataLocation it would be .local/share/qTox/ which is where it's supposed to be.

[sphaerophoria]

Sorry I've been out of it for a bit, did we ever end up changing the storage paths? If so I can update this PR to use the correct ones. I have one more PR In the works that depends on this one so I haven't pushed it, but if this one is unblocked I can finish up the feature :).

[anthonybilinski]

@sphaerophoria storage paths have now been merged on the path_rework branch. Could you point your PR to that as the base branch (and rebase if required), then we can merge this before path upgrade code is done?

[sphaerophoria]

Sorry, I haven't forgot about this.

I rebased onto the path_rework branch but couldn't test my changes because I coudln't create a profile on my box. Not sure if that's expected ATM or is a windows bug. I've been meaning to pull out my linux box to take another try but haven't had time.

[anthonybilinski]

It was a cross-platform bug of not creating the new directories before trying to access them. A fix has been pushed to path_rework.