Lowering number of saved revisions below 1 crashes program

[morags]

• Manuskript 0.7.0
• Windows 10 Pro

Under Settings -> Revisions: Lowering any of the counters below "1" causes an immediate crash.

[gedakc]

Thanks @morags for reporting this issue. I have confirmed the crash using Manuskript 0.7.0 on GNU/Linux so the issue applies to all OSes.

The console output when decreasing "revisions per minute for the last 10 minutes" from 1 to 0 is as follows:

$ manuskript
Debug: Web rendering engine used: QWebView
Running manuskript version 0.7.0.
Note: No translator found or loaded for locale en_CA.
Loading: /home/gedakc/tmp/manuskript-test.msk
Detected file format version: 1. Zip: False.
Project /home/gedakc/tmp/manuskript-test.msk loaded.
Traceback (most recent call last):
  File "/home/gedakc/workspace/manuskript/bin/../manuskript/settingsWindow.py", line 360, in revisionsSettingsChanged
    opt["rules"][10 * 60] = 60 / self.spnRevisions10Mn.value()
ZeroDivisionError: division by zero
Project manuskript-test.msk saved.
$

[RiderExMachina]

I can try my hand at this one. Assign it to me and I'll see what I can come up with this week.

[gedakc]

@RiderExMachina I tried to assign this issue to you. Unfortunately only the issue creator and @olivierkes and myself are listed as choices. I was unable to add your name directly.

Please feel free to develop a fix for this issue.

[RiderExMachina]

Fair enough, I'll be unofficially assigned.

[RiderExMachina]

@gedakc Manuskript wouldn't crash for me on my Ubuntu system, so I have no idea if my fix works or not. Would you be able to test 435e6b5 and see if there is any change?

[gedakc]

I'm not sure how to apply the commit listed in the previous message. If you know how (without manually making the changes myself) then please let me know.

Otherwise a patch file can be created and uploaded into this issue. To create a patch file the command should be something like:

git format-patch origin/develop -n --stdout > filename.patch

From a quick look at the code the changes should prevent the divide-by-zero crash.

Have you tested what happens when typing in text?
For example does the saving of revisions stop when the revision value is zero?

Also when creating a patch it is good practice to mention the issue number in the git comment. If you use something like Closes issue #381 then the issue will be automatically closed when your patch or PR is applied. See GitHub: Closing issues using keywords.

Using a short descriptive title also helps with patch review and tracking down regressions. For example a title like Prevent crash when saved revisions set to zero indicates what the patch / PR actually does.

[RiderExMachina]

@gedakc I understand, I just did not want to claim anything in the comment that I didn't know for 100% certainty resolved this issue.

I unfortunately cannot create a patch file right now (it would be several hours until I would be able to), but there should be a couple of ways to get the file.

Easiest way would be to download the raw settingsWindow.py file and drop it in your current local folder, but you could also clone my repository in a different folder or use git fetch and git checkout to checkout my changes.

That should be something like the this:

git fetch https://github.com/RiderExMachina/manuskript.git dev
git checkout RiderExMachina/manuskript-dev FETCH_HEAD

[gedakc]

Thanks @RiderExMachina for the tips on how to get the patch/file. It will likely be tomorrow before I get a chance to look further into this issue.

[RiderExMachina]

Okay, I just tested it on a different computer (on which I confirmed crashes without the fix). The program no longer crashes, but we need to figure out a way to actually have it so that 0 is a viable option. I'll work on that further later but for now will create a PR so we can not have baseless crashing.

[gedakc]

Thanks @RiderExMachina for trying to find a solution to this issue.

I tested the code in PR #384, and it does indeed prevent the crash.

One problem I see occurs when I reduce a revision setting to "0". The "0" appears in the Settings UI as would be expected. However when I exit the Settings window and reenter Edit -> Settings then the revision reverts to the previous value (1 in my case). This represents inconsistent behaviour.

Perhaps a better way is to consider a different approach to preventing the crash. We might consider limiting the range of values for each revision in the GUI. For example we could prevent values below 1. Further we might also limit the upper bound of how many revisions are saved in a given time frame. For example it doesn't make sense in my mind to make a revision save every second.

What are your thoughts on preventing the zero value in the UI?

[RiderExMachina]

@gedakc

What are your thoughts on preventing the zero value in the UI?

My thought to preventing issues would be to have an if/else branch for the settings. if the number is set to zero it doesn't divide and should theoretically work as intended (save no drafts for the period of time specified). However, at this current moment I don't know which value does what in the code, so I would need to either reverse-engineer it or ask someone who knows more about that particular spot more about how to proceed.

[gedakc]

That sounds like a reasonable approach too.

Unfortunately the person who knows the code base best has not been in contact for a long time. This means that it is up to the person creating the fix to learn the area of the code base in order to create a proper fix. Ideally the proposed fix should be well tested before being posted for review.

[RiderExMachina]

@gedakc

Understood. I'll toy with the code this weekend and see if I can come up with a good fix. Do you think it would be okay to merge the current PR for a temporary solution until then or would you prefer not to?

[gedakc]

My preference is to not merge the current PR because the fix introduces inconsistent behaviour. Also the cause of the crash is not something that writers are encountering every day. My guess is that most writers probably don't play around with the revisions settings.

EDIT: Additionally the title of the commit in the PR is vague. "Attempt to avoid crash" is not specific. Avoid which crash? Is it an attempt because the code hasn't been tested? What will later developers think when they see this commit?

[RiderExMachina]

That is fair. And yes, the original commit message of "attempt to avoid crash" was because at the time it hadn't been tested. I understand that the "crash" in question is vague, so that is probably for the best.

[gedakc]

Lowering the number of saved revisions below 1 is now prevented with the merge of PR #655. Hence closing this issue as resolved.