#24908: Validate saved files and alert user

[krasko78]

Resolves: #24908

Initial commit to demonstrate the idea and start refining it.

• [X] I signed the CLA
• [X] The title of the PR describes the problem it addresses
• [X] Each commit's message describes its purpose and effects, and references the issue it resolves
• [X] If changes are extensive, there is a sequence of easily reviewable commits
• [X] The code in the PR follows the coding rules
• [X] There are no unnecessary changes
• [X] The code compiles and runs on my machine, preferably after each commit individually
• [ ] I created a unit test or vtest to verify the changes I made (if applicable)

[krasko78]

Do we want to limit this to Windows users only?

[RomanPudashkin]

Do we want to limit this to Windows users only?

I'd rather keep it for all OS

[DmitryArefiev]

@krasko78 Can you restart builds please? Linux build is missing

[Jojo-Schmitz]

Non-staff-members can't do that

[Jojo-Schmitz]

But there is an issue with the translatable strings, like subsequent trailing whitespace, which the Linux build stumbles accross

Also a code style issue

[krasko78]

@Jojo-Schmitz How can I do the code style validation from inside Qt Creator or before I commit?

[Jojo-Schmitz]

The latter, running uncrustify locally, but I rather let GitHub CI let it do for me 😉

[krasko78]

@bkunda @RomanPudashkin @cbjeukendrup @Jojo-Schmitz @wizofaus Here is the warning dialog for review:

[krasko78]

Gah, no, I still don't like it... But let hear everyone out and I'll edit it accordingly. How about this one:

[wizofaus]

@Jojo-Schmitz How can I do the code style validation from inside Qt Creator or before I commit?

If you're on Windows I have a batch file that can both check and fix all modified files, though you need to install a specific version of uncrustify. I hate having to wait for the github CI job to fail the PR.

[Jojo-Schmitz]

There's not much to wait, it fails quite fast

[krasko78]

It is building fine now and I am publishing the PR. Please make sure you've looked at the latest changes. Thanks.

[bkunda]

Thanks for tackling this problem @krasko78! Great to have a solution that will prevent this kind of file corruption.

My only comment has to do with the UI copy and dialog design. To better align it with our existing corruption dialogs, and to simplify the messaging, I'd prefer to see the following:

Then when "Show details" is clicked, the error message appears below (and the dialog expands accordingly):

As I understand it, when a score file gets filled with 0s, it's no longer usable and in all cases needs to be re-saved. In this case, the messaging should be much simpler, and we should just give the user a button that triggers the OS save dialog ("Save a copy").

For bonus points, the "musescore.org" hyperlinked text should point directly to the relevant forum: https://musescore.org/en/forum/6

Let me know if this makes sense!

[cbjeukendrup]

@bkunda That is not 100% accurate. I will explain the problem in more detail later today (right now omw to an exam 🙂)

[krasko78]

Btw, should we say "corrupted" or "corrupt"? I believe we should use "corrupt" when we want the adjective. Native English speakers please advise.

[krasko78]

@cbjeukendrup What a loooooooooooooooong exam you are having. 😃 Hope it finishes soon. Good luck! 😃

[cbjeukendrup]

Ah, sorry that I didn't post an update here, but we're having some conversations about this issue via our internal channels. Of course we will let you know when there is an outcome.

[bkunda]

@krasko78 after quite a few discussions with my colleagues, I've updated the dialog copy as follows:

We would just use this one dialog in all instances where the file read check fails (I.e. for both the check of the temporary and definitive files).

Obviously the "Save as..." button would trigger the OS save dialog. And the "Try again" should simply re-trigger the user's initial save attempt. If the read check fails, then this dialog would again re-appear.

Hope this makes sense! Pls let me or @cbjeukendrup know if you have any questions!

@DmitryArefiev @RomanPudashkin FYI

[krasko78]

Thanks for the update @bkunda. Where should the musescore.org link on the dialog take the user exactly? To the "Support and bug reports" forum maybe?

[bkunda]

Thanks for the update @bkunda. Where should the musescore.org link on the dialog take the user exactly? To the "Support and bug reports" forum maybe?

Yes to this page: https://musescore.org/en/forum/6

[krasko78]

@bkunda Without the word "File" between the error code and the file name?

[cbjeukendrup]

I'd say it doesn't matter much; that error string should have the sole purpose of facilitating troubleshooting. So it should stay succinct, untranslated, and as descriptive as possible. For example, this error string should help us distinguish whether the check failed before or after moving the file. Also, in this context, I believe "corrupted" is more appropriate than "corrupt".

[krasko78]

@cbjeukendrup OK, no "File" and "corrupted". Just to confirm: everybody is aware of the latest decision: we do not move the temp file over the original file, instead we now copy it, then we check the original file for corruption - if it is ok, we delete the temp file and are good. If it is corrupted, we then check the temp file. In the error message in the dialog we will see the original file's name if the temp file is ok and the temp file's name if the temp file is corrupt.

[cbjeukendrup]

Ah I wasn't fully aware of that, but that sounds good to me!

[krasko78]

There we go! I've reordered the 3 buttons because MSS insisted on putting the Cancel button on the right side. For consistency, I didn't argue with MSS.

[Jojo-Schmitz]

Button order differs depending on platform

[krasko78]

Right. And looking at WinLayout, MacLayout and LinuxLayout in buttonboxmodel.h, I see that for all platforms Cancel (RejectRole) will be to the right of any custom buttons (CustomRole). So I should probably give the two save buttons the AcceptRole, ApplyRole or ContinueRole in order to get the button order as in bkunda's screenshot (at least on non-Windows systems).

[Jojo-Schmitz]

The ButtonBoxGroup widget gets it right automacally

[cbjeukendrup]

Using ButtonBox directly is not really an option here, because interactive()->error(…) is used. Setting the button roles is indeed the best way to get the order right.

[krasko78]

@DmitryArefiev @RomanPudashkin @cbjeukendrup Just confirming that no one is waiting for something from me here.