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

Save dialogue - Glitchy filename test #5193

Open
zonkmachine opened this issue Sep 16, 2019 · 15 comments
Open

Save dialogue - Glitchy filename test #5193

zonkmachine opened this issue Sep 16, 2019 · 15 comments
Assignees
Labels
bug needs takeover The original author is looking for someone else to continue this pull request
Milestone

Comments

@zonkmachine
Copy link
Member

As mentioned here, there is a remaining problem with the save dialogue.

Quote by @tresf

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. :)

Basically, we test for already existing files to avoid saving over them. We also test for existing file minus suffix as you can enter just the name and let LMMS/Qt add the suffix but this earlier didn't test for the proposed name 'with' suffix so, as an example, saving the project 'test' when 'test.mmp' was present would save over test.mmp (See #2230).
The remaining issue is that all occasions of the suffix is removed from the filename when testing. Not only the suffix proper on the end.

Related pull requests: #2230 #3526 #4768

@zonkmachine zonkmachine added this to the 1.2.1 milestone Sep 16, 2019
@zonkmachine zonkmachine self-assigned this Sep 16, 2019
@zonkmachine
Copy link
Member Author

Meanwhile, the original issue I tried to solve has seem to have been at least partially fixed... 🐳
I can recreate the bug on LinuxMint-17.3 but not on LinuxMint-18.3 . The original issue pointed to a possible bug in Qt and it seem to have been fixed in the meantime.

I'm looking into the possibility of reverting, in order:
#4768 419321d
#3526 9bdc011
#2230 e1c598b

#2230 has changes in it that should be kept but it may be least messy to revert it in it's entirety and pull those again separately.

I will try and update my machine and test the issue first to see the state of it before proceeding.


In order to test the issue on lmms 1.1.3 or older.

Create an issue "test.mmp"
Export as "test.wav"
Once more export as "test.wav" and note how you are prompted with a warning that the file exists.
Export as just "test". The software should catch that "test.wav" exists as it's going to complete the name.

The case above is if you go along with the project name as your render name. If you instead type in a new name altogether for the rendered project the issue seem to be there still.

@PhysSong PhysSong modified the milestones: 1.2.1, 1.2.2 Oct 10, 2019
@JohannesLorenz
Copy link
Contributor

@zonkmachine When do you have time to solve this?

@tresf
Copy link
Member

tresf commented Dec 29, 2019

"May be messy" sounds like "should be master". Perhaps a better question is... is this needed for 1.2?

@tresf
Copy link
Member

tresf commented Jan 11, 2020

Two weeks, no answer. Re-milestoning.

@MrTopom
Copy link
Contributor

MrTopom commented Aug 29, 2023

May I take this one ?

@zonkmachine
Copy link
Member Author

Yes indeed but I would caution against it. I think your looking at a rewrite rather than troubleshooting. I was the one to mess this up and I failed to fix it in stable-1.2. After many changes in master it's beyond what I can handle but I'll be happy to test a PR.

@MrTopom
Copy link
Contributor

MrTopom commented Aug 29, 2023

So assign it to me and I'll have a try.

@MrTopom
Copy link
Contributor

MrTopom commented Aug 30, 2023

I took some time reading the different issues and comments related to this issue and this is one idea to implement.
I present it before coding it (is seems ok).

[Updated]
if fileName's suffix is the selected dialog one then test if file exists
else
if fileName's suffix is part of other possible suffix in the dialog then keep this suffix and test if file exists
else
add the dialog's suffix to the file and test if file exists

This would force the creation of file with valid suffix.

@MrTopom
Copy link
Contributor

MrTopom commented Aug 30, 2023

As the usage of mmpz or mmp is based on configuration, we could have the filedialog only proposing the possible format and then the file dialog would have always only one possible format on a selected format line.

@zonkmachine
Copy link
Member Author

This would force the creation of file with valid suffix.

OK. That at least sounds familiar. I'll look into this after the weekend and try and replicate the issue/s.

As the usage of mmpz or mmp is based on configuration, we could have the filedialog only proposing the possible format and then the file dialog would have always only one possible format on a selected format line.

But that is only the default file format. You can always change format later on actually saving.
(I'm not sure I understand you completely here)

@MrTopom
Copy link
Contributor

MrTopom commented Aug 31, 2023

New Strategy :
if filename has a prefix available in the dialog then test if exists
else force the selected dialog suffix and test if exists

Seems to simple but logical for me.

@MrTopom
Copy link
Contributor

MrTopom commented Aug 31, 2023

This would force the creation of file with valid suffix.

OK. That at least sounds familiar. I'll look into this after the weekend and try and replicate the issue/s.

Yes it would be good to have a test case cause I was not able to reproduce for the moment on Linux Ubuntu.

I think the Save has no issue and the export should be addressed with #5744.

@JohannesLorenz
Copy link
Contributor

@MrTopom We want to close 1.3 and thus, we could need a fix for this issue soon. Do you want to write a PR (in this case we could support you), or should someone else do it?

@Rossmaxx
Copy link
Contributor

@JohannesLorenz i believe the OP is inactive.

@Rossmaxx Rossmaxx added the needs takeover The original author is looking for someone else to continue this pull request label Aug 10, 2024
@DomClark
Copy link
Member

How about showing the save dialog after, rather than before, the export dialog? The reason we sometimes have to change the extension is that we show the save dialog before we know what the extension should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs takeover The original author is looking for someone else to continue this pull request
Projects
None yet
Development

No branches or pull requests

7 participants