Differential D2878

Krita freezes upon file recovery - KDE Bug 367610
AcceptedPublic
Actions

Authored by pkouame on Sep 28 2016, 7:20 AM.

Details

Reviewers
rempt
Summary

The auto-save recovery dialog seems to hang after a document recovery. The recovered document loads, but the app's user interface is frozen ( (most menu items grayed out).

Fixed a few problems:

  1. Turns out, signaling KisApplication::onAutoSaveFinished(int result) from within the recovery dialog is problematic and causes some sort of hang. Unecessarily complex. Now I just let the dialog exec out normally and KisApplication::checkAutosaveFiles() goes on its meery way.
  1. Consolidated some logic to eliminate the now unecessary slot call to onAutoSaveFinished.
  1. The dialog itself was never released

Not sure why this is only happening on macOS though.

My environment:
krita master 3.0.2 beta git (b9c7f56)
ext_qt qt5.7 rebuilt and patched
xcode 8
macOS Sierra
macPro 3,1

Test Plan

Tested these scenarios:

  • one to many files to recover - discard
  • one to many files to recover - ok
  • one to many files to recover - cancel
  • generate an auto-save by kill -abrt
  • simple new file
  • simple open

EDIT: additional tests just performed

  • verified timed autosave
  • changed settings to 1 minute
  • verified new timing

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
pkouame updated this revision to Diff 6967.Sep 28 2016, 7:20 AM
pkouame retitled this revision from to Krita freezes upon file recovery - KDE Bug 367610.
pkouame updated this object.
pkouame edited the test plan for this revision. (Show Details)
pkouame added a reviewer: rempt.
pkouame set the repository for this revision to R37 Krita.
pkouame added a project: Krita: Releases (Release works 3.0.2).
rempt accepted this revision.Sep 28 2016, 9:23 AM
rempt edited edge metadata.

Looks reasonable -- I've added two comments. Could you please apply for a developer account so you can push the patch yourself? You can give me as a reference.

https://community.kde.org/Infrastructure/Get_a_Developer_Account

libs/ui/KisApplication.cpp
510

Best remove this.

627

Since this is a member variable, it probably should also be set to 0 after deleting

This revision is now accepted and ready to land.Sep 28 2016, 9:23 AM
pkouame added a comment.Sep 28 2016, 2:55 PM

cool. comments noted.

BTW, I also noticed that thumbnails for auto-save kra files aren't created consistently on crashes. Found that a while ago also. Exacerbated now while I "stress" test with multiple auto-saves. So let me look into that quickly - while i have the hood open ...

rempt added a comment.Sep 28 2016, 5:34 PM

That might be because autosave files don't have thumbnails or mergedimage previews -- not saving those, saves time.

pkouame added a comment.Sep 28 2016, 6:28 PM

Dunno, maybe it's just me but the attached snap just looks weird. Don't you think? May be we have some default thumbnail to display in that situation.

Also, if one does opt to recover a previously unamed file, one gets a "Unamed - .krita-PID-document_0-autosave.kra".
Natural reaction is to quickly cmd-S (Save) which does what it's told and saves the "hidden" file.
A clean restart and you get into recovery scenario again.
Just not intutive to me, maybe we should force a Save As... in such cases. Worth another look.

Last thing - shouldn't we conceal the location of recovery material a bit more. Right now it defaults to one's $HOME. Other systems tuck these things away in hidden directories. Maybe a configurable setting?

Ok. I think I'll micro-patch. So my comments/opinions above notwithstanding, I'll deal with the crash here and file an issue for othe UI/UX related ones.

Let me fill out a few kde forms...;-)

pkouame updated this revision to Diff 6977.Sep 28 2016, 8:06 PM
pkouame edited edge metadata.
pkouame removed R37 Krita as the repository for this revision.

accepting review comments

pkouame edited the test plan for this revision. (Show Details)Sep 28 2016, 8:10 PM

Revision Contents
Changeset List

PathPackages
Mlibs/ui/KisApplication.h (2 lines)
Mlibs/ui/KisApplication.cpp (64 lines)

Diff 6977

View Options

libs/ui/KisApplication.h

Loading...
View Options

libs/ui/KisApplication.cpp

Loading...