Set default suffix in "Save as..." file save dialog #2230

Merged
merged 1 commit into from Dec 14, 2016

Projects

None yet

4 participants

@zonkmachine
Member

When you "save as..." without adding suffix manually, LMMS will add a suffix but not test if the file "name + suffix" exists and will gladly write over an existing file.
This is true on Ubuntu Mate at least.
This patch adds suffix mmp/mmpz as default to VersionedSaveDialog when called in saveProjectAs() which fixes the issue.
When I searched the issue I got the impression that this could be mostly a Linux thing but I have not possibility to test on any other OS.

@zonkmachine zonkmachine changed the title from Set default suffix to Set default suffix in "Save as..." file save dialog Aug 2, 2015
@zonkmachine
Member

saveas

Save action after fix.

@zonkmachine zonkmachine changed the title from Set default suffix in "Save as..." file save dialog to [WIP] Set default suffix in "Save as..." file save dialog Aug 2, 2015
@zonkmachine
Member

After a cup of sufficiently strong coffee I realise that this should be fixed on the other save dialogues too so I'll expand on this pull request.

@curlymorphic
Contributor

@zonkmachine Nice work, and good catch on the other save dialogs

Are you able to test windows builds using wine?

Hopefully we are not too far way from releasing test builds where full testing under windows will take place.

@zonkmachine
Member

@curlymorphic Thanks!

Are you able to test windows builds using wine?

No. It's a miracle that I manage to compile anything at all. Building under, or for, Wine are too many alien elements for me right now.

I've caught some of the cases now but I don't quite know what to do when the suffix is selected after the file dialogue calls exec()

@zonkmachine
Member
zonkmachine commented Aug 2, 2015 edited

To sum up the work so far.

✔️ Save As... Fixed
✔️ Export... Fixed
✔️ Export MIDI... Fixed (But MIDI export is disabled...)
✔️ Save preset... Fixed

@zonkmachine
Member

Since we do the work for the .mpt outside of the sfd.exec() the default suffix is added so we need to remove it first. I'll go shopping for a better method but for now .mmp and .mmpz works.

@zonkmachine
Member

This should maybe be done in VersionedSaveDialog() but I can't wrap my grey matter around it. As an example I give you this instead.
( .mpt ) It works and the odds that you ever come across it is tiny. You need to first want to save a template file and also want to write over a present file or accidentally type the name of a present file... all this without typing in the suffix manually, then this message box will execute and since it's done after versionedSaveDialog() has finished it won't take you back to the save dialog but to the project window.

@tresf
Member
tresf commented Aug 7, 2015

@zonkmachine,

Here's an old commit where I tried to tackle a somewhat similar problem 7a03353 (yeah, it's ugly)

@zonkmachine
Member

tackle a somewhat similar problem

As I understand it that part is for avoiding adding the suffix if the user has already typed it in. I basically just tossed a message box in there after the output file name is done to test for duplicates.
I'm uncertain of how this works on other platforms and I'm curious to see if there is a better way to do it within the file dialogue exec() and not after.

@tresf
Member
tresf commented Aug 8, 2015

I'm uncertain of how this works on other platforms

Since we're using Qt's dialog, it should be identical across platforms.

I'm curious to see if there is a better way to do it within the file dialogue exec() and not after.

Agreed.

@zonkmachine
Member

Perhaps... http://stackoverflow.com/questions/9285575/how-can-i-get-a-qfiledialog-to-prompt-for-overwrite-in-qt-4

No. We set QFileDialog::AcceptSave already in versionedFileDialog, among the first things actually.

I'm curious to see if there is a better way to do it within the file dialogue exec() and not after.

Agreed.

Maybe handle it under #1834 then and just settle for a more temporary fix for now?

@tresf
Member
tresf commented Aug 17, 2015

@zonkmachine if overwrite prompting is going to continue to be a problem moving forward, can we permanently patch in this prompt directly in the FileDialog class?

@zonkmachine
Member

Looking into it.

@zonkmachine
Member

can we permanently patch in this prompt directly in the FileDialog class?

Nope. I can't wrap my head around that one.

@tresf tresf added the bug label Sep 18, 2015
@tresf tresf added this to the 1.2.0 milestone Sep 18, 2015
@zonkmachine
Member

@tresf I don't know how to proceed with this one. The PR still works and I've updated it but haven't done anything more to it.

@zonkmachine if overwrite prompting is going to continue to be a problem moving forward, can we permanently patch in this prompt directly in the FileDialog class?

Add to #1834 and fix later?

@tresf tresf commented on an outdated diff Aug 10, 2016
src/gui/MainWindow.cpp
{
- fname += ".mpt";
+ // Remove the default suffix
+ fname.remove( "." + suffix );
+ if( !sfd.selectedFiles()[0].endsWith( ".mpt" ) )
+ {
+ if( QFile( fname + ".mpt" ).exists() )
+ {
+ QMessageBox mb;
+ mb.setWindowTitle( tr( "Save project" ) );
+ mb.setText( fname + tr( ".mpt already exists. "
@tresf
tresf Aug 10, 2016 Member

Probably shouldn't translate the .mpt portion

@tresf
Member
tresf commented Aug 10, 2016 edited

In short, yes, this technique is our only technique. I find people's answer on this to be utter bullshit. Native common dialogs handle these things all the time and Qt should too. </endrant>.

Anyway, yes, this is the right approach. Only criticism I have is we should probably consolidate as much of this logic into a single, static (or perhaps not static if it makes sense) in a helper function (reduces complexity as well as the number of language translations as well). FileDialog is a good place to start assuming they all use it anyway.

@zonkmachine
Member

FileDialog is a good place to start assuming they all use it anyway.

Yes, they all use FileDialog or VersionedSaveDialog which is derived from FileDialog .
I looked at this before and my answer is the same. I don't know how to do this. I'm not experienced enough with working with Classes. There is a function in there FileDialog::clearSelection() which at least gives a hint at how to extend it but it isn't enough.

@tresf
Member
tresf commented Aug 18, 2016

I looked at this before and my answer is the same. I don't know how to do this. I'm not experienced enough with working with Classes.

This time specifically I was speaking about a single, static function to reduce the redundancy in code, e.g. a helper to make a centralized location to test if (fileExists) { ... } etc.

@Umcaruje
Member

@zonkmachine what's the status on this? Can this still be done for 1.2 or should we move the milestone?

@zonkmachine
Member

what's the status on this?

The status is that the code works and is up to date but adds some redundancy and I don't know how to remedy that.

@zonkmachine
Member

@zonkmachine what's the status on this?

Done! Maybe test it a bit first though... 😉

@zonkmachine zonkmachine changed the title from [WIP] Set default suffix in "Save as..." file save dialog to Set default suffix in "Save as..." file save dialog Dec 13, 2016
@zonkmachine
Member

@tresf I've moved the redundant code to a function in versionedFileDialog. Tested and I'm happy with it. Merge?

@tresf
Member
tresf commented Dec 14, 2016

@zonkmachine I'll have to defer the decision to merge to someone else. The squash erased the context of my original questions and I don't have time to review thoroughly.

If you are happy with it please feel free to merge on your own.

@zonkmachine
Member

The squash erased the context of my original questions

:trollface: A bit of an oops there... I'll merge this then.

@zonkmachine zonkmachine merged commit e1c598b into LMMS:master Dec 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine zonkmachine deleted the zonkmachine:savefileassavesover branch Dec 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment