screenshot file naming logic is bugged

[axd1967]

This has been rejected as a bug in #1180 , yet I call on any developer that ever touches that code area, to review the code.

The code in question is following

https://github.com/Stellarium/stellarium/blob/23078c5912cd61129f72e1f4ba7489af38330299/src/StelMainView.cpp#L1680-L1685

This code contains internal logic errors as a result of a sloppy choice of magic numbers / hardcoded related constants.

Why limit to 10000? why not 10_0001? or 10_000_000? There is no immediate need for this limit. The full unsigned 32bit range is fine (although a time based range such as Unix time is a superior choice that would not break the existing implementation).

Meanwhile, the code is internally contradicting itself (counting to 100000 but limiting to a field width of 3 digits will cause sorting issues). The code will forcibly (and silently) overwrite an existing screenshot once rolling past 100000 . a while loop over an unsigned int makes this problem not go away, but far less likely (even if that also is Y2K reasoning).

The logic is also smelly: when the user moves some of the screenshot files, gaps are opened in the file names which will be filled by chronologically later screenshots, but alphabetically earlier (gap fillers). This will cause subtle sorting problems. (For me an indication that the use of an index is smelly. Call it an opinion...).

todo

• [ ] raise the limit or get rid of the limit and continue from the largest existing number retrieved from the directory, bearing in mind that an (unlikely, yet predictable) overflow can occur. unless all numbers are in use, the routine will find the next usable file name (but again, this is a waste of processing, see Unix time instead)
• [ ] use a field width that matches the counter (10 digits when going for an unsigned 32bit solution)
• [ ] for readability, make these constants rather than literals
• [ ] if still needed, optimize the expression shotDir.filePath() + "/" + screenShotPrefix as new variable (as it is invariant wrt j). this might probably be further optimized by including the suffix + "." + screenShotFormat and using a parameter to insert the found index.

A follow-on solution is to replace the numbering with Unix time, while searching for a solution (64 bit? ) for Epochalypse.

[axd1967]

Additionally, I discovered in https://github.com/Stellarium/stellarium/issues/1180#issuecomment-699004723 that the index suffix is always added, also when invoking the screenshot function from scripting.

An improvement is to allow the caller to skip the free index seeking logic (eg a flag or new argument generate_index = true ?) as the index might no longer make sense.

Edit added: the index '000' (or maybe higher) is added only when overwrite=false.

Edit added: the same time stamp logic is used as default in the screen capture feature of KUbuntu ("spectacle").

[axd1967]

for_wishlist

[chrisx8]

@alex-w, what do you think if we just name screenshots by the current time, like stellarium-yyyyMMdd-hhmmss-ms?

Proposed change

A simple implementation of this could be like so:

// name screenshot files with current time
QString currentTime = QDateTime::currentDateTime().toString("yyyyMMdd-hhmmss-ms");
QFileInfo shotPath = QFileInfo(shotDir.filePath() + "/" + screenShotPrefix + currentTime + "." + screenShotFormat);

This would replace this code segment: https://github.com/Stellarium/stellarium/blob/966f54b393b619a7e9d6886289d31fdb94c1835a/src/StelMainView.cpp#L1714-L1719

Reasons

I think the current implementation is not a good idea. Here are some issues that I can think of:

• It can be very slow when there are lots of files in the screenshots directory.
• Deleting one (of many) screenshot and taking a new one will break the logical sort order of screenshot files. For example, if we have screenshots 001, 002, and 003, and we delete 002. This is fine because 001 and 003 is in order, but what if we take another screenshot? It'll be named 002, even though it's newer than all previous screenshots, hence breaking logical sort order.

IMO, The time-based naming is probably a better choice here, because:

• Time-based naming for screenshots is standard on a majority of desktop and mobile operating systems.
• Does not require checking files in screenshot directory (hence better performance) or storing a counter of screenshots (hence more reliable).

Please let me know what do you think!

[alex-w]

I disagree. You may add it as an additional option to naming screenshot files (include updating the GUI), but not just replace the current method.

[gzotti]

It may indeed be good to detect the greatest used number and add there. And maybe allow a time-based strategy as alternative.

[alex-w]

Please check the latest stable version of Stellarium: https://github.com/Stellarium/stellarium/releases/latest