Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix export - double dialog windows on writing over existing file #3526

Merged
merged 2 commits into from May 30, 2017

Conversation

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented May 1, 2017

If writing over an existing file on export you are prompted twice. See comment here:
#3516 (comment)
Issue introduced here: #2230

@Umcaruje I had this fix lying around already but hadn't tested it properly. It looks like a fix but needs more testing.

@zonkmachine zonkmachine requested a review from Umcaruje May 1, 2017
@zonkmachine zonkmachine added this to the 1.2.0 milestone May 1, 2017
@zonkmachine zonkmachine mentioned this pull request May 1, 2017
@Hey-Holokin
Copy link

@Hey-Holokin Hey-Holokin commented May 2, 2017

Export showed 2 overwrite dialogs before checkout. After checkout, 1 overwrite dialog is shown. Pull request works as expected.

@@ -1416,19 +1417,18 @@ void Song::exportProject( bool multiExport )
// Get first extension from selected dropdown.
// i.e. ".wav" from "WAV-File (*.wav), Dummy-File (*.dum)"
suffix = efd.selectedNameFilter().mid( stx + 2, etx - stx - 2 ).split( " " )[0].trimmed();
exportFileName.remove( "." + suffix );

This comment has been minimized.

@tresf

tresf May 2, 2017
Member

Two observations about remove(...)...

  1. By defaul it's case insensitive by default which can cause issues on Mac and Windows, since they're both case-insensitive (yes, Mac is too!)
  2. This will remove all instances of e.g. .wav, so there are some edge cases where my.wav.file.wav might get snagged, no?
@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented May 2, 2017

By defaul it's case insensitive by default which can cause issues on Mac and Windows, since they're both case-insensitive (yes, Mac is too!)

Fixed... I think.

This will remove all instances of e.g. .wav, so there are some edge cases where my.wav.file.wav might get snagged, no?

Yes. Will think. Think not easy. Sad.

Copy link
Member

@Umcaruje Umcaruje left a comment

Tested this out, can confirm it fixes the issue 👍

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented May 21, 2017

@zonkmachine can we merge or do you have something to add to this PR?

@qnebra
Copy link

@qnebra qnebra commented May 27, 2017

Tested locally, fixed double export dialog issue for me.

@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented May 28, 2017

@zonkmachine can we merge or do you have something to add to this PR?

I'm not near my build machine so I can't work on this right now but I'm happy with a merge. @tresf had made some points above that I also can't test or do anything about right now.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented May 30, 2017

Ok no objections to a merge for 3 days, merging this. If there are any issues, they can be opened on the tracker.

@Umcaruje Umcaruje merged commit 9bdc011 into LMMS:stable-1.2 May 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine zonkmachine deleted the zonkmachine:exportfixstable branch May 30, 2017
@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented May 30, 2017

If there are any issues, they can be opened on the tracker.

Cool! I'll test this later on and there are still questions from @tresf unanswered. Will get to that.

@tresf
Copy link
Member

@tresf tresf commented May 30, 2017

There are still some logical problems. Linux can have .foo and .FOO side-by-side (valid albeit confusing) whereas Windows and MacOS cannot, so that logic is a bit presumptuous and these edge-cases will be very confusing in the very rare event they occur.

There's also the double-extension problem, where .remove() makes no guarantee that the extension is at the end of the file. Again, very rare edge-case but when it happen to someone it won't be obvious. For example ocean.waves.wav would become oceanes.wav. :)

PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017
…S#3526)

* Fix export - double dialog windows on writing over existing file

* Case sensitivity
PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017
…S#3526)

* Fix export - double dialog windows on writing over existing file

* Case sensitivity
@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Jan 8, 2019

@tresf I'm looking at fixing the two issues you mention above. I haven't found a way to force any actual bug to appear though.

The case sensitivity issue should be fixed by:

diff --git a/src/core/Song.cpp b/src/core/Song.cpp
index ba2659a..6e9b382 100644
--- a/src/core/Song.cpp
+++ b/src/core/Song.cpp
@@ -1414,7 +1414,11 @@ void Song::exportProject( bool multiExport )
                                // Get first extension from selected dropdown.
                                // i.e. ".wav" from "WAV-File (*.wav), Dummy-File (*.dum)"
                                suffix = efd.selectedNameFilter().mid( stx + 2, etx - stx - 2 ).split( " " )[0].trimmed();
-                               exportFileName.remove( "." + suffix, Qt::CaseInsensitive );
+                               Qt::CaseSensitivity cs = Qt::CaseSensitive;
+#if defined(LMMS_BUILD_APPLE) || defined(LMMS_BUILD_WIN32) || defined(LMMS_BUILD_WIN64)
+                               cs = Qt::CaseInsensitive;
+#endif
+                               exportFileName.remove( "." + suffix, cs );
                                if ( efd.selectedFiles()[0].endsWith( suffix ) )
                                {

However. I intend to fix the problem of using remove() by using QFileInfo().completeBaseName() to 'remove' the suffix and I suspect that also takes care of the other issue. QFileInfo() doesn't mention case sensitivity anywhere so I guess it's all taken into account internally.

@tresf
Copy link
Member

@tresf tresf commented Jan 8, 2019

FYI, LMMS_BUILD_WIN32 is always true for LMMS_BUILD_WIN64, only 32 is needed. If QFileInfo works as described (even when the target file doesn't yet exist) it sounds like a plan.

@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Jan 13, 2019

If QFileInfo works as described (even when the target file doesn't yet exist)

Actually no. It operates on existing files. I'll come up with something else...

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jan 15, 2019

I think we can address the remove() issue by using chop() with endsWith().
However, I've found another issue: the remove() tries to ..wav or similar because suffix starts with ..

@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Jan 16, 2019

Thanks for chipping in! I'll look into chop() with endsWith().
I tried exportFileName.truncate( exportFileName.lastIndexOf( suffix, -1, cs ) ); in different ways but bumped into what I think is the same issue you mention above with remove(). I'm not treating dots with suffixes consistently so I'll have to look closer into this.

@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Jan 26, 2019

Also just bumped into QTBUG-59401, an issue with dots in the path and setDefaultSuffix().

@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Jan 26, 2019

Here is the original Qt issue I tried to solve in #2230 with probing the suffix. https://bugreports.qt.io/browse/QTBUG-11352
I think I referred to it earlier but that info got lost when I rebased the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.