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

Only filter out <>:"/\|?* while exporting tracks. #5282

Merged
merged 1 commit into from Dec 23, 2019

Conversation

Cyp
Copy link
Contributor

@Cyp Cyp commented Oct 28, 2019

This now preserves most characters in exported track names.

@Spekular
Copy link
Contributor

@Spekular Spekular commented Oct 28, 2019

This looks good to me. Technically it misses a few illegal names on Windows, but they weren't covered before either, aside from ending a filename with a period.

Out of scope for this PR, but it feels like a bad idea to handle this in these classes. Really, all file writing should delegate to a single method that cleans the filename. That would ensure consistent behavior and make sure every fix (like this) applies to every file we save. Plus, it could be a bit larger (to handle Windows's reserved names and OS differences better) without taking up space in the classes that use it.

@Spekular
Copy link
Contributor

@Spekular Spekular commented Oct 28, 2019

Oh, but I don't feel like the INSTALL.txt changes belong in this. I've seen a lot of activity around fixing CI recently so this fix may already be applied or made redundant in some other PR?

@Cyp
Copy link
Contributor Author

@Cyp Cyp commented Oct 28, 2019

I see INSTALL.txt in master does mention submodules. It doesn't mention that cmake can mysteriously fail to generate a Makefile without them, though. Anyway, dropping the INSTALL.txt commit would be fine.

@Cyp
Copy link
Contributor Author

@Cyp Cyp commented Oct 29, 2019

Rebased on latest stable-1.2, and dropped the INSTALL.txt commit…

@PhysSong PhysSong added the needs testing label Nov 16, 2019
@PhysSong
Copy link
Member

@PhysSong PhysSong commented Nov 16, 2019

Is it safe to be merged?

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Nov 21, 2019

It seems like there are some file systems which support limited sets of characters. Should we take care of them?
See https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations for details.

@Cyp
Copy link
Contributor Author

@Cyp Cyp commented Nov 21, 2019

Now also filters 0x00-0x1F and 0x7F. I hope that covers any semi-serious filesystems there that aren't EBCDIC or with a 16-or-less character limit.

@JohannesLorenz JohannesLorenz self-requested a review Dec 12, 2019
@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Dec 12, 2019

@Cyp what do you think about a global common function for that, that also filters out periods at the end, like @Spekular suggested? I ask because, imagine the next time someone will change the filter in RenderManager, they'll surely forget to change it in InstrumentTrack.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Dec 13, 2019

a global common function for that, that also filters out periods at the end

In most cases, we append the file extension at the end. A trailing period is equivalent to the empty file extension(NOT a file without extensions).

@Cyp
Copy link
Contributor Author

@Cyp Cyp commented Dec 13, 2019

It's a global common string (in the last version of the commit), so there shouldn't be anyone changing it in one place but not the other. I don't think there should be any files written without an extension added, so I don't think checking for trailing dots is needed (although it could be added if needed).

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Dec 14, 2019

If we save directories, it may be valuable, but it still seems like a seldom case.

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Dec 14, 2019

What were your reasons to choose that filter? E.g.:

  1. Why allow ! but filter ??
  2. Why allow ; but filter :?

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Dec 15, 2019

@JohannesLorenz The filter reflects restricted characters in major OSes/file systems.

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Dec 15, 2019

OK, then I consider functional and style reviews done. Testing remains.

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Dec 22, 2019

I just tested to save a file aaa<>:"\|?*bbb from the instrument window's save button. It saved as \|?*bbb.xpf, so it

  • did not remove the claimed chars \|?*
  • removed the aaa postfix

Can you please check what happens in your local copy?

@Cyp
Copy link
Contributor Author

@Cyp Cyp commented Dec 23, 2019

Same here, but I think it's unrelated to the commit. The " seems to have special meaning to the save dialogue box. If trying to save to /te"st" in some other program (kwrite), it tries saving to /st (with just one ", it refuses to even try to save).

If naming the track aaa<>:"\|?*bbb and trying to save in the instrument window, it suggests aaabbb as the default filename.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Dec 23, 2019

I checked the regular expression, but I couldn't find any issues with it. I could also confirm the issue with the file dialog, using Qt creator.
I think it's ready to merge.

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Dec 23, 2019

Right, if you don't use " in the dialog, it works.

I'll merge, thanks for the contribution!

@JohannesLorenz JohannesLorenz merged commit d849cc1 into LMMS:stable-1.2 Dec 23, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants