Fixed calligra crashing when opening remote document
ClosedPublic

Authored by niccolove on Jan 23 2019, 12:49 PM.

Diff Detail

Repository
R8 Calligra
Branch
print-remote-files (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7716
Build 7734: arc lint + arc unit
niccolove created this revision.Jan 23 2019, 12:49 PM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptJan 23 2019, 12:49 PM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald Transcript
niccolove requested review of this revision.Jan 23 2019, 12:49 PM
niccolove updated this revision to Diff 50098.Jan 23 2019, 12:52 PM
  • Fixed typos
niccolove edited the test plan for this revision. (Show Details)Jan 23 2019, 12:53 PM

Could you please describe what you have done and why

Sure.
Calligra was crashing because the method "slotFilePrint" called "rootView()" that tried to return the first elements of "d->rootViews". The problem is that such list was empitya, as SetRootDocument was never called: that method is called in openDocument but not in openDocumentInternal. I therefore added that function and it now works. But, when slotLoadCompleted is called (that is, after slotFilePrint), there's a check to open a new window if there's already a non-blank document. Since the root document was already set, it opened a new window with the same document. I therefore added the conditions of the two documents being different.
I hope I've been clear, this is one of my first commits here :-/

At this point my only worry is that the setRootDocument in openDocumentInternal makes the check for opening a new window in slotLoadCompleted useless. Moving the whole if statement from slotLoadCompleted to openDocumentInternal also works, and could avoid to replace the root document instead of opening a new window. It makes more sense, now that I think of it.

Yes that description helped a lot, and you are doing great. Keep up the good work :)

One concern though - how often and when is openDoumentInternal called. I'm a bit afraid that we just add some functionality to a place that might be used for something else

niccolove updated this revision to Diff 50100.Jan 23 2019, 1:16 PM
  • Moving the if from slotLoadCompleted to openDocumentInternal.

I've seen that openDoumentInternal is called every time a remote document is used as input. I'm not sure if it's also used in different scenarios.

Please check with loading a normal document,. The same concerns now applies to slotLoadCompleted - especially since you have now removed functionality from it.

Also please check in all of our applications as some might reimplement virtual methods

I tried differents files (locale and remote, opening and printing), and everything works okay. Other applications work normally, but calligrasheets still crashes when trying to print a remote (odt) file - but I can see the file is downloaded correctly, so it looks like a different problem (calligrasheets manages printing?).
About removing features from slotLoadCompleted, it is important to notice that it is only called by openDocumentInternal itself, so that should not be a problem.

I think the problem is that slotFilePrint() is called (in KoApplication::start()) before the document is actually loaded.
Probably we need to start printing in a slot connected to KoMainWindow::loadCompleted() instead.
And to be safe, we also need to handle loadCanceled().
And the same for exportToPdf, I think.

Probably we need to start printing in a slot connected to KoMainWindow::loadCompleted() instead.
And to be safe, we also need to handle loadCanceled().

I agree with that.

I'm not sure on how to do that, do you have any tip?
openDocumentInternal is called by openDocument, which is called in KoApplication just before checking the arguments. When it sees the print argument, it calls slotFilePrint. openDocumentInternal downloads the document in async. Should it be made not async, or is there another way to make it wait until the other process ends? Also, "slotLoadCanceled" is already implemented, or were you talking about something else?

So just don't call slotFilePrint connect loadCompleted to slotFilePrint. When openDocumentInternal downloads the document it fires signal of finish, just connect to slot of opened it.

I'm not sure on how to do that, do you have any tip?
openDocumentInternal is called by openDocument, which is called in KoApplication just before checking the arguments. When it sees the print argument, it calls slotFilePrint. openDocumentInternal downloads the document in async. Should it be made not async, or is there another way to make it wait until the other process ends? Also, "slotLoadCanceled" is already implemented, or were you talking about something else?

Well niccolove, you have really put your hand into a bee-hive :)
The answer from anthony can be used as a test to see if the the princip works, but does not address all situations.

The current implementation needs quite a lot of change to work. These are the conditions as I see them:

  1. openDocument() initiates loading and returns true if it finds the file, just return false if the file do not exist.
  2. The actual loading is asynchronous if the file is remote.
  3. It is possible to specify multiple files to load. (Each file will be shown in it's own main window.)
  4. For writing to pdf, a file name can be specified on the command line.

These are the steps I think is needed:

  1. Create slot(s) in KoApplication to handle the different resullts; print, pdf and cancelled.
  2. The signal connections must be done before openDocument() is called. (So that local files also works.)
  3. Add KoDocument as an arg to the KoMainWindow::loadCompleted() signal, so KoApllication knows which dcoument has been loaded.
  4. For pdf, find a way to handle filename.
niccolove updated this revision to Diff 50519.Jan 29 2019, 9:58 PM
  • Made slotFilePrint depend on signal loadCompleted to avoid it running before remote file being complete
niccolove updated this revision to Diff 50520.Jan 29 2019, 9:59 PM
  • Removed unnecessary newlines
niccolove updated this revision to Diff 50542.Jan 30 2019, 9:38 AM
  • Added slots in KoApplication to handle pdf and print.

What have I Done:

  • Added slots in KoApplication to manage pdf and print. Cancel is already KoMainWindow::slotLoadCanceled with an error popup, what should a KoApplication::slotLoadCanceled do?
  • Added connection for pdf print and moved it before document opening (local files works)
  • I added the KoMainView to the loadCompleted signal, as a new one might be created in KoMainWindow::slotLoadCompleted, and the slots needs the new one to call slotFilePrint or exportToPdf.
  • Filename is passed to slot using a lambda, like specified on the slots and signals qt documentation

Things seem working. Should I do anything else?

regarding the lambda connection - nice to see that you use the version with a "still alive" argument - but I fear this requires a qt version higher than what we currently support - could you please check

niccolove updated this revision to Diff 50551.Jan 30 2019, 12:15 PM
  • Adding pdf filename as propriety of KoApplication

This solution is Qt <5.12 friendly

This solution is Qt <5.12 friendly

What you mean?

libs/main/KoApplication.h
154

It should be in KoApplicationPrivate then use as others d->pdfFileName

This comment was removed by niccolove.

Regarding the completed slot, the doc says:
--print Only print and exit
--export-pdf Only export to PDF and exit

To exit, you need to know when all jobs have finished, so if some failed you need to track that.
Also, the connection to job in slotExportToPdf() is wrong (not your fault) as it will just quit while there may be other jobs running.

niccolove updated this revision to Diff 50552.Jan 30 2019, 12:59 PM
  • Moved QString pdfFileName to KoApplicationPrivate
niccolove marked an inline comment as done.Jan 30 2019, 1:28 PM

I have some doubts:

  • Anything related to "and exiting" does not seem to work. mainWindow->slotFileQuit() does not seem working.
    • --benchmark-loading-show-window should also exit using mainWindow->slotFileQuit(), but it doesn't.
    • Adding mainWindow->slotFileQuit() after a print job does nothing.
    • Calligra does not seem to exit after pdf job, even though job end is connected to mainWindow->slotFileQuit(). (removing the connect does not seem to change anything)
  • The exit after exporting to pdf job seems right, since you should not be able to export multiple files to a single pdf anyway, and you can't print AND export to pdf in the same command.
  • The print command will open a mainWindow for every printing job, so running mainWindow->slotFileQuit() after mainWindow->slotFilePrint() would bring to having no windows open at the end of all jobs.

Yeah, well there are some half done designs in here :(

The benchmark just does not work with remote files. Don't know if it should or if it should be blocked. Let's leave it for now.

If you do not specify a pdf filename, it will use the same name as in the loaded file just changing the extension to .pdf, so it is possible to export multiple files.
I have sort of assumed that print and exportToPdf commands was meant to be hands free, to be able to print a number of files wo a lot of mouse work.
Popping up dialogs (print dialog or error dialogs) defies that goal, so imho these things should be handled differently.
I recognize that this fix has balooned and we are getting into difficult areas, so I propose:
Let's do without exit atm, just remove the job connection in slotExportToPdf() and add a TODO to implement exit later.

niccolove updated this revision to Diff 50557.Jan 30 2019, 2:39 PM
  • Removed connection between job and exiting; added todo
danders accepted this revision.Jan 31 2019, 10:51 AM

Yes, close.
When export to pdf, the mainwindow is not shown and since we don't exit, the app will hang.
You need to invert the check in KoApplication line 460 so the window is shown.

Also please fix the trailing spaces.

And then: ok to land!

This revision is now accepted and ready to land.Jan 31 2019, 10:51 AM
niccolove updated this revision to Diff 50590.Jan 31 2019, 11:35 AM
  • Made window show on pdf print and removed trailing spaces
niccolove updated this revision to Diff 50836.Feb 4 2019, 12:02 PM
  • Made windows show even without export-pdf flag
niccolove updated this revision to Diff 50837.Feb 4 2019, 12:04 PM
  • fixed whitespace, sorry

Ok, it's accepted so please commit/push/land

I do not have a developer account. I requested one some days ago with no answer yet.

Dan can you please reply - I'm fine with the account but I can't reply to Ben as I am not able to send email

Dan can you please reply - I'm fine with the account but I can't reply to Ben as I am not able to send email

Yes, I have, and just answered again in case something went wrong first time.
Let's see if something happens soon.

niccolove updated this revision to Diff 51617.Feb 13 2019, 6:34 PM

BUG:358581

This revision was automatically updated to reflect the committed changes.

It seems like it landed. Please tell me if I screwed up anything.

Thanks, I'm checking it out now...
Just a couple of things:
Keywords needs to be on a separate line to have effect, see https://community.kde.org/Infrastructure/Git/Hooks
Also FIXED-IN is nice to add.

I've updated and closed the bug, so no problem.