Move splash instantiation outside if statement fully terminates threads on close.
Needs RevisionPublic

Authored by ictwod on Mar 31 2020, 7:12 PM.

Details

Reviewers
aacid
Summary

Moving the splash instantiation outside of the if statement checking whether or not "Show Splash" = false or nosplash was given in the command line allows k3b to close fully and terminate all threads it creates.

Originally if nosplash or "Show Splash" = false was configured, although k3b opens without a splash screen, it is unable to fully terminate on close. This leaves threads created by k3b running after closing.

BUG: 419105

FIXED-IN:
Kubuntu: 19.10
KDE Applications: 20.07.70
KDE Frameworks Version: 5.69.0
KDE Plasma: 5.16.5
Qt Version: 5.12.4

Test Plan
  1. Open k3b in the command line with the argument --nosplash, then click 'X' on the k3b window (closing it). K3b should close and all threads should terminate. Test with ps ux | grep k3b
  1. Open k3b and uncheck 'Show splash screen' in Miscellaneous Settings, close k3b. Then open k3b in the command line (with no arguments), then click 'X' on the k3b window (closing it). K3b should close and all threads should terminate. Test with ps ux | grep k3b

Diff Detail

Repository
R467 K3b
Lint
Lint Skipped
Unit
Unit Tests Skipped
ictwod requested review of this revision.Mar 31 2020, 7:12 PM
ictwod created this revision.
ictwod retitled this revision from Moved splash instantiaion outside of if statement. Nosplash k3b now fully terminates on close. to Move splash instantiaion outside if statement fully terminates threads on close..Mar 31 2020, 7:31 PM
ictwod edited the summary of this revision. (Show Details)
ictwod edited the summary of this revision. (Show Details)Mar 31 2020, 7:56 PM
ictwod retitled this revision from Move splash instantiaion outside if statement fully terminates threads on close. to Move splash instantiation outside if statement fully terminates threads on close..Mar 31 2020, 9:05 PM
aacid added a comment.Apr 5 2020, 5:00 PM

oh wow, this is actually pretty bad, not your fix/workaround, but the bug itself, i know i didn't think it would be a Qt bug, but from seeing your code and playing with it a bit it may actually be.

aacid added a comment.Apr 5 2020, 5:14 PM

Can you confirm this actually fixes the problem for you?

I like it a bit better because:

  • doesn't create the Splash when it's not needed
  • removes the processEvents call which in general is a bit nasty and "shouldn't be needed"
  • Uses the pointer to function method for close instead of the version with quotes
diff --git a/src/k3bapplication.cpp b/src/k3bapplication.cpp
index 76e764f77..379c53091 100644
--- a/src/k3bapplication.cpp
+++ b/src/k3bapplication.cpp
@@ -49,6 +49,7 @@
 
 #include <QCommandLineParser>
 #include <QDebug>
+#include <QTimer>
 
 
 K3b::Application::Core* K3b::Application::Core::s_k3bAppCore = 0;
@@ -91,12 +92,9 @@ void K3b::Application::init( QCommandLineParser* commandLineParser )
     else {
         m_mainWindow->show();
     }
-    
-    processEvents();
-    
+
     if (splash) {
-        splash->hide();
-        QMetaObject::invokeMethod( splash, "close", Qt::QueuedConnection );
+        QTimer::singleShot(0, splash, &QWidget::close);
     }
 
     qRegisterMetaType<KSharedConfig::Ptr>( "KSharedConfig::Ptr" );
ictwod added a comment.Apr 6 2020, 6:45 AM

It does work for me. However, can you explain the changes a bit further? Why did you comment out processEvents()? What does the functional call do in this context?

aacid added a comment.Apr 19 2020, 5:46 PM

It does work for me. However, can you explain the changes a bit further? Why did you comment out processEvents()? What does the functional call do in this context?

Oh sorry, i totally missed your answer.

processEvents in general is bad design or a workaround for bad design.

The thing about events is they are supposed to be that, things that someone sends and you process when you get them. Manually calling processEvents says "oh i know someone sent an events and i need to process it now because otherwise bad things will happen", which is just bad.

Since you mention that my patch also works for you, i'll commit that one.

Your patch was not bad, but the one i did is a bit better for the reasons mentioned in the previous commit.

I hope that doesn't discourage you to keep working on more patches!

aacid requested changes to this revision.Apr 19 2020, 10:11 PM

Please close this since my other fix just landed

This revision now requires changes to proceed.Apr 19 2020, 10:11 PM