[Merged] PR#1148: Backup files policy

[triplus]

I tested the latest changes and i see there is still a typo in preferences. FCBack instead of FCBak. The input field to add and format date stamp i guess is a nice addition. Users using more backup files can easily determine when a backup file was created. I purposely exceeded the -99 limit and got a pop-up informing me the save failed.

I did detect some issues though. User can put just about anything in the input field. That by itself is not a bad thing. But when that happens maximum number of backup files setting won't be respected anymore. In addition with default date stamp it can happen two save operations happen faster and therefore -1 is for example appended to the second file. That again will result in exceeding the maximum number of backup files setting. I am guessing you should change this logic and always do:

FileName + Stamp + Number + FCBak. And don't care about the user set Stamp part anymore. Just use the Number when checking if maximum number of backup files was reached or not. Basically like it works with the default solution. Now if this will introduce some of the "issues" back. You where after to resolve and feel the default option has. I don't know. But maximum number of backup files setting should i guess be detected more reliably.

[plgarcia]

I tested the latest changes and i see there is still a typo in preferences. FCBack instead of FCBak.

Ok I correct that.

The input field to add and format date stamp i guess is a nice addition. Users using more backup files can easily determine when a backup file was created. I purposely exceeded the -99 limit and got a pop-up informing me the save failed.

Ok I correct that. I must say that I did not imagine that more than 99 backups could have been requested.
There is no more limit but the system limits.
The message you saw is the kind of message that happen when there is a problem and the save did not go to the end.

I did detect some issues though. User can put just about anything in the input field. That by itself is not a bad thing. But when that happens, maximum number of backup files setting won't be respected anymore. In addition with default date stamp it can happen two save operations happen faster and therefore -1 is for example appended to the second file. That again will result in exceeding the maximum number of backup files setting. I am guessing you should change this logic and always do:

FileName + Stamp + Number + FCBak. And don't care about the user set Stamp part anymore. Just use the Number when checking if maximum number of backup files was reached or not. Basically like it works with the default solution. Now if this will introduce some of the "issues" back. You where after to resolve and feel the default option has. I don't know. But maximum number of backup files setting should i guess be detected more reliably.

Ok the issue is because of the freedom we gave the user to be able to identify what backup is linked to what document. I change the filter to identify the backups.
The user will be able to put anything as format. A "." will be replaced by "-". So between the base name of the file, and the extension, there will be not ".". If there is a “.”, it does not concern the same project. If there is no “.”, it can only concern this project. When there is a duplicate due to rapid saves for example, or to the format chosen by the user that can be a simple string, the a -1 -2 ... will be added.

That should solve completely the problems you identify, and make the checks simple. No need to count the characters as I tried to do, what became unsuccessful obviously with ta last opening given.

Let me some time to test before I publish this change. I will give you a go to test again.

Thanks for the time taken!

[plgarcia]

This is it.
This solves definitively the problem identifying the archives.

I added an extra function: If the format ends by “.” and “-“, a number is always added.
If it ends with a space the space is removed and the number added.
Therefore a format that contains only 1 space puts therefore only the number.
The –number is added only if needed (ie an archive with the name already exists). That means when the format does not discriminate enough every save (save in the same second or constant format for example).

[plgarcia]

I do not understand what are the errors detected by tests during build, and not with all builds. May be due to the environment the compilations and test have been run!

I have no error on my Linux and Windows systems, neither when building nor when runtest!

But this concerns regex, and I added regex to check the formats of the names of the archives!

[plgarcia]

I just changed str::regex by boost::regex. May be a conflict with some libaries.
I wait for complete travis build to see if the error still exists.

[GeneFC]

This continuing whack-a-mole bug fixing surely indicates that this change should be targeted at version 0.18.

While the basic idea seems OK, there has not been a lot of clamoring for the change. And it is likely that very few people have looked at the changes. It is quite to be expected that numerous suggestions for changes or improvements will happen once this new naming system is released.

Adding this to 0.17 just before release means that any deficiencies will be "baked in" to the stable version of FC for the next year or so.

I know plgarcia has put a lot of work into this, but it seems prudent to hold off from including in the 0.17 release.

Sorry.

Gene

[triplus]

@Gene

This actually is how it is supposed to be done. Proposal -> Testing -> Feedback -> Acknowledging it -> Fixing detected bugs ...

@plgarcia

I tested the latest changes. I did a few basic tests and detecting the maximum number of backup files set in preferences now seem to be detected reliably. I used a combination of date and random string and that didn't cause any issues too. The typo is corrected. Maybe if detecting the .FCBak extension would be case insensitive. That would further improve reliability.

All in all as this is optional feature and you acknowledge the feedback and resolved the detected issues. You have my vote for merging it before FreeCAD 0.17 is released. Note that your determination plays a role too. Otherwise the answer would simply be we are in feature freeze phase already and that would be that. Now as for whatever happens next. We will just have to accept the outcome and that is that.

[plgarcia]

Thank you.
It is not a complex code.
The difficulty came from the request to make it optional, and to include variability in the format. My first proposal was working perfectly and the checks were very simple.
As long I did not decide to rework the code and define a robust way to identify the different backups there was no solution.

The code stays simple, and I do not think with all the tests I did myself, and the test done by triplus there is big risk taking this PR.
Writing code is part of my job, but managing projects and delivery is also part of my job, so I understand both points of view.

I remind that the problem I encountered is that I was losing my changes because not warned when a problem while saving occurred. Google problem seems to be solved, but what happens when disk full, or whatever problem on the FS.

To help to make a decision here is status and clearly what brings this PR:

• Adding some optional features to define naming of the archives,

• In all cases solves the problems detected in the present version:
  • Handles the delay when modifying or deleting files (as Google drives was doing on windows),
  • Explicitly reports to the user when save does not come to its end, (messages added to be translated)
  • Handles correctly the number of archives even when changed in configuration.

I can add case insensitive test for the extension, it is almost risk free!

[triplus]

Here is a general remark for professional non embedded software.

I likely wouldn't lecture too much about taking the professional approach. When my feature would be pending in a feature freeze phase. A feature that could and up doing exactly what you said software should never do. Especially as there is no known general and oppressing issue in default implementation. The issue you detected was reported and now fixed in other software.

What i am trying to say is FreeCAD approach likely already is on a high professional level. In addition we sometimes go the extra mile. Like for example investing the effort to get a feature in while in feature freeze phase. Please don't hold that against us.

[plgarcia]

I was not to argue sorry. I remove this remark.

By professional I did not mean FreeCAD is not, or the approach is not professional. I think even the contrary, there are people reviewing, release freeze etc. I feel it is really on a good track!