PR#1148: Backup files policy

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
triplus
Posts: 8439
Joined: Mon Dec 12, 2011 4:45 pm

Re: PR#1148: Backup files policy

Post by triplus » Thu Mar 01, 2018 6:11 pm

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
Posts: 281
Joined: Wed Jun 17, 2015 9:47 pm
Location: Near Paris (France)

Re: PR#1148: Backup files policy

Post by plgarcia » Thu Mar 01, 2018 10:56 pm

triplus wrote:
Thu Mar 01, 2018 6:11 pm
I tested the latest changes and i see there is still a typo in preferences. FCBack instead of FCBak.
Ok I correct that.
triplus wrote:
Thu Mar 01, 2018 6:11 pm
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.
triplus wrote:
Thu Mar 01, 2018 6:11 pm
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
Posts: 281
Joined: Wed Jun 17, 2015 9:47 pm
Location: Near Paris (France)

Re: PR#1148: Backup files policy

Post by plgarcia » Fri Mar 02, 2018 9:48 am

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
Posts: 281
Joined: Wed Jun 17, 2015 9:47 pm
Location: Near Paris (France)

Re: PR#1148: Backup files policy

Post by plgarcia » Fri Mar 02, 2018 10:18 am

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
Posts: 281
Joined: Wed Jun 17, 2015 9:47 pm
Location: Near Paris (France)

Re: PR#1148: Backup files policy

Post by plgarcia » Fri Mar 02, 2018 12:30 pm

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
Posts: 1025
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: PR#1148: Backup files policy

Post by GeneFC » Fri Mar 02, 2018 3:36 pm

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

Gene

triplus
Posts: 8439
Joined: Mon Dec 12, 2011 4:45 pm

Re: PR#1148: Backup files policy

Post by triplus » Fri Mar 02, 2018 4:54 pm

@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
Posts: 281
Joined: Wed Jun 17, 2015 9:47 pm
Location: Near Paris (France)

Re: PR#1148: Backup files policy

Post by plgarcia » Fri Mar 02, 2018 7:07 pm

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!
Last edited by plgarcia on Fri Mar 02, 2018 8:59 pm, edited 2 times in total.

triplus
Posts: 8439
Joined: Mon Dec 12, 2011 4:45 pm

Re: PR#1148: Backup files policy

Post by triplus » Fri Mar 02, 2018 7:47 pm

plgarcia wrote:
Fri Mar 02, 2018 7:07 pm
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
Posts: 281
Joined: Wed Jun 17, 2015 9:47 pm
Location: Near Paris (France)

Re: PR#1148: Backup files policy

Post by plgarcia » Fri Mar 02, 2018 8:03 pm

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!
Last edited by plgarcia on Sun Mar 04, 2018 10:55 am, edited 1 time in total.

Post Reply