flameshot icon indicating copy to clipboard operation
flameshot copied to clipboard

1 line Bug Fix - April 23 commit prevents using : in ISO 8601 extended time formats

Open jr81 opened this issue 7 years ago • 8 comments

While #196 makes a case for changing the time default, the associated 17ac6f7a352f136b7470b66f536d372861fc6d8d completely prevents using colons often used in ISO Extended Time formats.

It also creates an unexpected user experience when colons can not be entered and are changed to dashes before the user's eyes.

As colons are not a problem in Linux file names, and all my image files have them ;-), I suggest keeping the new default, but allowing users to enter colons by undoing the second part of 17ac6f7a352f136b7470b66f536d372861fc6d8d as follows:

https://github.com/lupoDharkael/flameshot/blob/94daa4fecdfaf6188853625d4798b4c5804a20a3/src/utils/filenamehandler.cpp#L48

48>     res = res.replace(QLatin1String("/"), QStringLiteral("⁄"));

Thank you for such a well designed utility!

jr81 avatar Nov 27 '18 22:11 jr81

I don't think that's a bug, it's by design. Even tough colons can be used on Unix systems, they're generally not recommended.

Also, the patch would be a regression for Windows systems, a OS conditional should be used instead.

Do you have any use case where you really need to use a colon?

AlfredoRamos avatar Nov 27 '18 23:11 AlfredoRamos

@jr81 As you know, file name contains colon is invalid under Windows system (NTFS partition).

holazt avatar Nov 28 '18 03:11 holazt

@AlfredoRamos

Bug vs Feature: I didn't think it was by design because #196 only advocated changing the default, not prohibiting the use of colons under all circumstances.

#196 actually created a Linux regression, because colons legally entered and stored in ~/.config/Dharkael/flameshot.ini prior to #196, were clobbered during path expansion after #196.

Colons not recommend under Linux: I actually Googled "colons in Linux file names" before submitting my issue, but didn't find any compelling arguments against.

Of course colons are treated specially by some commands, for example scp ./my:file:with:colons target_host:/tmp/my:new:file:with:colons would fail without the leading ./, but then if you're are using scp from the command line, you probably know this or at least know how to Google it. IMHO, well designed GUI apps should allow and properly handle all valid OS file names.

Windows regression: I don't see how this would create a Windows regression, because dashes stored in flamshot.ini would still be honored. Yes, it would allow a user to create file names that work under Linux but not Windows, but Linux people are well aware of this and should we really force Windows restrictions on all operating systems? That's why many of us left Windows for Linux. ;-)

OS conditional: Great solution! But if we are prohibiting :, what about the other invalid chars Windows chars? i.e. " < > * / \ ? What's our purpose?

Need: Yes I have thousands of image, scanned receipts, video, notes, and pdf files that are preceded with an ISO style format. e.g. 2018-11-28 01:30 My business receipt.png. I could have easily worked around this with a script, but I'm trying to contribute to this fine project.

@ZetaoYang

Colons invalid under Windows: Yes I realize colons are invalid under Windows as are " < > * / \ ?.

I started with DOS 1.0 where we had A:, B:, but no C:! My first drive C: was $7,000 (= $20k today) for 20MB, not GB. You're talking to a really old guy who literally bought the very first IBM PC off the first truck shipped to Missouri. It's really fun to see all this progress and work with talented people like you!

The 81 in @JR81 is the year of my treasured vintage computer, not me! LOL

jr81 avatar Nov 28 '18 10:11 jr81

But if we are prohibiting :, what about the other invalid chars Windows chars? i.e. " < > * / \ ? What's our purpose?

Fair enough.

I agree with your arguments except with the usage of colons in Linux, because file names should be normalized for all possible systems, for compatibility, but that's just my opinion.

I see your point, removing the replacement of the colon does not affect other systems, because it's already fixed in the date format, I was under the impression you wanted to revert the date format too and you only put the replacement as example.

AlfredoRamos avatar Nov 29 '18 20:11 AlfredoRamos

This is my fault for the false impression.

My line suggesting "keeping the new default" should have been a seperate paragraph. I missed it myself when I was looking to see if I had actually included it.

jr81 avatar Nov 30 '18 20:11 jr81

Be careful because the valid character set for filenames does not depend on the operating system alone, but also on the type of filesystem where you are saving the file. Look at #295 (file created by Linux on an NTFS partition used in a dual-booted system). Also saving files from Linux to a FAT-32 formatted USB pen drive (the normal case) can have the same problems.

I agree that simply removing colons is not a nice way to solve it, but the matter is not so easy. I would

  • give a warning if the user uses a filename that can create a problem (any of the "prohibited") characters is present)
  • never use such a char in automatically generated file names by default

but of course, this is just an opinion.

Rmano avatar Jan 15 '19 09:01 Rmano

@Rmano Your consideration is very thoughtful and detailed. Yes, there is the special situation:

file created by Linux on an NTFS partition used in a dual-booted system

holazt avatar Jan 15 '19 09:01 holazt

This would be great if closed, not being able to have normal dates as filenames is cumbersome, having to use an external script to provide the correct filename

cwrau avatar Aug 13 '24 20:08 cwrau

This became a much bigger deal than I wanted so I gave up on it long ago. Still using my script. Thanks for your consideration. Closing issue.

jr81 avatar Sep 09 '24 23:09 jr81