Am Dienstag, 6. Juni 2006 14:52 schrieb Georg Baum:
> Jean-Marc Lasgouttes wrote:
>
> > Do we need extensive testing of batch-commands processing, for
> > example?
>
> I am not sure whether it is possible to test all code paths. I would be
> confident if somebody could review all cases where lyx_exit() is used,
the
> rest should be safe. My idea behind the lyx_exit change was that we need
to
> use lyx_gui::exit if we exit after we entered the GUI event loop.
I checked that myself again and remove one from teh signal handler. I am
applying the attached patch now to trunk. If you ask me the corresponding
1.4 patch could go in, too.
Georg
Log:
Fix crash on exit (bug 2549) by correct usage of QApplication
* src/lyx_cb.C
(quitLyX): lyx_gui::exit takes now an argument
* src/frontends/{gtk,xforms}/lyx_gui.C
(lyx_gui::parse_init): rename to lyx_gui::exec and call LyX::exec2
(void lyx_gui::exit): add exit status argument
* src/frontends/qt{3,4}/lyx_gui.C
(cleanup): new function for pointer cleanup
(lyx_gui::parse_init): rename to lyx_gui::exec and call LyX::exec2,
turn static variables into automatic variables
(void lyx_gui::exit): add exit status argument
(start): Use cleanup()
(exit): ditto
* src/frontends/lyx_gui.h
(parse_init): remove
(exec): new
(exit): Take exist status argument
* src/lyx_main.[Ch]
(LyX::priv_exec): split into LyX::priv_exec and LyX::exec2
* src/lyx_main.C
(lyx_exit): New, choose the right exit function
(showFileError): call lyx_exit
(LyX::queryUserLyXDir): ditto
(LyX::init): ditto
(LyX::priv_exec): ditto
(LyX::priv_exec): Replace want_gui by lyx_gui::use_gui
(LyX::priv_exec): replace lyx_gui::parse_init by lyx_gui::exec and
exec2
(LyX::init): Replace gui argument by lyx_gui::use_gui
Index: src/lyx_cb.C
===================================================================
--- src/lyx_cb.C (Revision 14032)
+++ src/lyx_cb.C (Arbeitskopie)
@@ -216,7 +216,7 @@ void quitLyX(bool noask)
Alert::warning(_("Unable to remove temporary directory"), msg);
}
- lyx_gui::exit();
+ lyx_gui::exit(0);
}
Index: src/frontends/gtk/lyx_gui.C
===================================================================
--- src/frontends/gtk/lyx_gui.C (Revision 14032)
+++ src/frontends/gtk/lyx_gui.C (Arbeitskopie)
@@ -100,7 +100,7 @@ int getDPI()
} // namespace anon
-void lyx_gui::parse_init(int & argc, char * argv[])
+void lyx_gui::exec(int & argc, char * argv[])
{
new Gtk::Main(argc, argv);
@@ -112,6 +112,8 @@ void lyx_gui::parse_init(int & argc, cha
// must do this /before/ lyxrc gets read
lyxrc.dpi = getDPI();
+
+ LyX::ref().exec2(argc, argv);
}
@@ -152,8 +154,9 @@ void lyx_gui::start(string const & batch
}
-void lyx_gui::exit()
+void lyx_gui::exit(int /*status*/)
{
+ // FIXME: Don't ignore status
Gtk::Main::quit();
}
Index: src/frontends/qt3/lyx_gui.C
===================================================================
--- src/frontends/qt3/lyx_gui.C (Revision 14032)
+++ src/frontends/qt3/lyx_gui.C (Arbeitskopie)
@@ -79,6 +79,10 @@ using std::string;
extern BufferList bufferlist;
+// FIXME: wrong place !
+LyXServer * lyxserver;
+LyXServerSocket * lyxsocket;
+
namespace {
int getDPI()
@@ -90,11 +94,15 @@ int getDPI()
map<int, shared_ptr<socket_callback> > socket_callbacks;
-} // namespace anon
+void cleanup()
+{
+ delete lyxsocket;
+ lyxsocket = 0;
+ delete lyxserver;
+ lyxserver = 0;
+}
-// FIXME: wrong place !
-LyXServer * lyxserver;
-LyXServerSocket * lyxsocket;
+} // namespace anon
// in QLyXKeySym.C
extern void initEncodings();
@@ -104,7 +112,6 @@ extern bool lyxX11EventFilter(XEvent * x
#endif
#ifdef Q_WS_MACX
-extern bool macEventFilter(EventRef event);
extern pascal OSErr
handleOpenDocuments(const AppleEvent* inEvent, AppleEvent* /*reply*/,
long /*refCon*/);
@@ -153,22 +160,23 @@ namespace lyx_gui {
bool use_gui = true;
-void parse_init(int & argc, char * argv[])
+
+void exec(int & argc, char * argv[])
{
// Force adding of font path _before_ QApplication is initialized
FontLoader::initFontPath();
- static LQApplication app(argc, argv);
+ LQApplication app(argc, argv);
#if QT_VERSION >= 0x030200
// install translation file for Qt built-in dialogs
// These are only installed since Qt 3.2.x
- static QTranslator qt_trans(0);
+ QTranslator qt_trans(0);
if (qt_trans.load(QString("qt_") + QTextCodec::locale(),
qInstallPathTranslations())) {
- app.installTranslator(&qt_trans);
+ qApp->installTranslator(&qt_trans);
// even if the language calls for RtL, don't do that
- app.setReverseLayout(false);
+ qApp->setReverseLayout(false);
lyxerr[Debug::GUI]
<< "Successfully installed Qt translations for locale "
<< QTextCodec::locale() << std::endl;
@@ -182,7 +190,7 @@ void parse_init(int & argc, char * argv[
// These translations are meant to break Qt/Mac menu merging
// algorithm on some entries. It lists the menu names that
// should not be moved to the LyX menu
- static QTranslator aqua_trans(0);
+ QTranslator aqua_trans(0);
aqua_trans.insert(QTranslatorMessage("QMenuBar", "Setting", 0,
"do_not_merge_me"));
aqua_trans.insert(QTranslatorMessage("QMenuBar", "Config", 0,
@@ -192,7 +200,7 @@ void parse_init(int & argc, char * argv[
aqua_trans.insert(QTranslatorMessage("QMenuBar", "Setup", 0,
"do_not_merge_me"));
- app.installTranslator(&aqua_trans);
+ qApp->installTranslator(&aqua_trans);
#endif
using namespace lyx::graphics;
@@ -204,6 +212,8 @@ void parse_init(int & argc, char * argv[
lyxrc.dpi = getDPI();
LoaderQueue::setPriority(10,100);
+
+ LyX::ref().exec2(argc, argv);
}
@@ -245,9 +255,7 @@ void start(string const & batch, vector<
qApp->exec();
// FIXME
- delete lyxsocket;
- delete lyxserver;
- lyxserver = 0;
+ cleanup();
}
@@ -263,18 +271,16 @@ void sync_events()
}
-void exit()
+void exit(int status)
{
- delete lyxsocket;
- delete lyxserver;
- lyxserver = 0;
+ cleanup();
- // we cannot call qApp->exit(0) - that could return us
+ // we cannot call QApplication::exit(status) - that could return us
// into a static dialog return in the lyx code (for example,
// load autosave file QMessageBox. We have to just get the hell
// out.
- ::exit(0);
+ ::exit(status);
}
Index: src/frontends/qt4/lyx_gui.C
===================================================================
--- src/frontends/qt4/lyx_gui.C (Revision 14032)
+++ src/frontends/qt4/lyx_gui.C (Arbeitskopie)
@@ -76,6 +76,10 @@ using std::string;
extern BufferList bufferlist;
+// FIXME: wrong place !
+LyXServer * lyxserver;
+LyXServerSocket * lyxsocket;
+
namespace {
int getDPI()
@@ -86,11 +90,15 @@ int getDPI()
map<int, shared_ptr<socket_callback> > socket_callbacks;
-} // namespace anon
+void cleanup()
+{
+ delete lyxsocket;
+ lyxsocket = 0;
+ delete lyxserver;
+ lyxserver = 0;
+}
-// FIXME: wrong place !
-LyXServer * lyxserver;
-LyXServerSocket * lyxsocket;
+} // namespace anon
// in QLyXKeySym.C
extern void initEncodings();
@@ -145,50 +153,21 @@ bool LQApplication::macEventFilter(Event
#endif
-namespace {
-
-LQApplication * app = 0;
-
-}
-
-
namespace lyx_gui {
bool use_gui = true;
-void parse_init(int & argc, char * argv[])
-{
- /*
- FIXME : Abdel 29/05/2006 ([EMAIL PROTECTED])
- reorganize this code. In particular make sure that this
- advise from Qt documentation is respected:
-
- Since the QApplication object does so much initialization, it
- must be created before any other objects related to the user
- interface are created.
-
- Right now this is not the case. For example, the call to
- "FontLoader::initFontPath()" below is doned before the QApplication
- creation. Moreover, I suspect that a number of global variables
- contains Qt object that are initialized before the passage through
- parse_init(). This might also explain the message displayed by Qt
- that caused the hanging:
-
- QObject::killTimer: timers cannot be stopped from another thread
- */
+void exec(int & argc, char * argv[])
+{
// Force adding of font path _before_ QApplication is initialized
FontLoader::initFontPath();
-#ifdef Q_WS_WIN
- static QApplication win_app(argc, argv);
-#else
- app = new LQApplication(argc, argv);
-#endif
+ LQApplication app(argc, argv);
// install translation file for Qt built-in dialogs
// These are only installed since Qt 3.2.x
- static QTranslator qt_trans(0);
+ QTranslator qt_trans(0);
if (qt_trans.load(QString("qt_") + QTextCodec::locale(),
qInstallPathTranslations())) {
qApp->installTranslator(&qt_trans);
@@ -206,7 +185,7 @@ void parse_init(int & argc, char * argv[
// These translations are meant to break Qt/Mac menu merging
// algorithm on some entries. It lists the menu names that
// should not be moved to the LyX menu
- static QTranslator aqua_trans(0);
+ QTranslator aqua_trans(0);
aqua_trans.insert(QTranslatorMessage("QMenuBar", "Setting", 0,
"do_not_merge_me"));
aqua_trans.insert(QTranslatorMessage("QMenuBar", "Config", 0,
@@ -228,6 +207,8 @@ void parse_init(int & argc, char * argv[
lyxrc.dpi = getDPI();
LoaderQueue::setPriority(10,100);
+
+ LyX::ref().exec2(argc, argv);
}
@@ -269,10 +250,7 @@ void start(string const & batch, vector<
qApp->exec();
// FIXME
- delete lyxsocket;
- delete lyxserver;
- lyxserver = 0;
- delete app;
+ cleanup();
}
@@ -288,18 +266,16 @@ void sync_events()
}
-void exit()
+void exit(int status)
{
- delete lyxsocket;
- delete lyxserver;
- lyxserver = 0;
+ cleanup();
- // we cannot call qApp->exit(0) - that could return us
+ // we cannot call QApplication::exit(status) - that could return us
// into a static dialog return in the lyx code (for example,
// load autosave file QMessageBox. We have to just get the hell
// out.
- ::exit(0);
+ ::exit(status);
}
Index: src/frontends/xforms/lyx_gui.C
===================================================================
--- src/frontends/xforms/lyx_gui.C (Revision 14032)
+++ src/frontends/xforms/lyx_gui.C (Arbeitskopie)
@@ -83,6 +83,7 @@ namespace {
/// quit lyx
bool finished = false;
+int exit_status = 0;
/// estimate DPI from X server
int getDPI()
@@ -152,7 +153,7 @@ namespace lyx_gui {
bool use_gui = true;
-void parse_init(int & argc, char * argv[])
+void exec(int & argc, char * argv[])
{
setDefaults();
@@ -197,6 +198,8 @@ void parse_init(int & argc, char * argv[
lyxrc.dpi = getDPI();
LoaderQueue::setPriority(10,100);
+
+ LyX::ref().exec2(argc, argv);
}
@@ -313,12 +316,14 @@ void start(string const & batch, vector<
// FIXME: breaks emergencyCleanup
delete lyxsocket;
delete lyxserver;
+ ::exit(exit_status);
}
-void exit()
+void exit(int status)
{
finished = true;
+ exit_status = status;
}
Index: src/frontends/lyx_gui.h
===================================================================
--- src/frontends/lyx_gui.h (Revision 14032)
+++ src/frontends/lyx_gui.h (Arbeitskopie)
@@ -46,9 +46,6 @@ std::string const sans_font_name();
/// return a suitable monospaced font name (called from non-gui context too !)
std::string const typewriter_font_name();
-/// parse command line and do basic initialisation
-void parse_init(int & argc, char * argv[]);
-
/**
* set up GUI parameters. At this point lyxrc may
* be used.
@@ -60,7 +57,12 @@ void parse_lyxrc();
* batch commands, and loading the given documents
*/
void start(std::string const & batch, std::vector<std::string> const & files,
- unsigned int width, unsigned int height, int posx, int posy);
+ unsigned int width, unsigned int height, int posx, int posy);
+
+/**
+ * Enter the main event loop (\sa LyX::exec2)
+ */
+void exec(int & argc, char * argv[]);
/**
* Synchronise all pending events.
@@ -70,7 +72,7 @@ void sync_events();
/**
* quit running LyX
*/
-void exit();
+void exit(int);
/**
* return the status flag for a given action. This can be used to tell
Index: src/lyx_main.C
===================================================================
--- src/lyx_main.C (Revision 14032)
+++ src/lyx_main.C (Arbeitskopie)
@@ -107,12 +107,23 @@ string cl_system_support;
string cl_user_support;
+void lyx_exit(int status)
+{
+ // FIXME: We should not directly call exit(), since it only
+ // guarantees a return to the system, no application cleanup.
+ // This may cause troubles with not executed destructors.
+ if (lyx_gui::use_gui)
+ lyx_gui::exit(status);
+ exit(status);
+}
+
+
void showFileError(string const & error)
{
Alert::warning(_("Could not read configuration file"),
bformat(_("Error while reading the configuration file\n%1$s.\n"
"Please check your installation."), error));
- exit(EXIT_FAILURE);
+ lyx_exit(EXIT_FAILURE);
}
@@ -204,14 +215,21 @@ void LyX::priv_exec(int & argc, char * a
{
// Here we need to parse the command line. At least
// we need to parse for "-dbg" and "-help"
- bool const want_gui = easyParse(argc, argv);
+ lyx_gui::use_gui = easyParse(argc, argv);
lyx::support::init_package(argv[0], cl_system_support, cl_user_support,
lyx::support::top_build_dir_is_one_level_up);
- if (want_gui)
- lyx_gui::parse_init(argc, argv);
+ // Start the real execution loop.
+ if (lyx_gui::use_gui)
+ lyx_gui::exec(argc, argv);
+ else
+ exec2(argc, argv);
+}
+
+void LyX::exec2(int & argc, char * argv[])
+{
// check for any spurious extra arguments
// other than documents
for (int argi = 1; argi < argc ; ++argi) {
@@ -224,10 +242,10 @@ void LyX::priv_exec(int & argc, char * a
// Initialization of LyX (reads lyxrc and more)
lyxerr[Debug::INIT] << "Initializing LyX::init..." << endl;
- init(want_gui);
+ init();
lyxerr[Debug::INIT] << "Initializing LyX::init...done" << endl;
- if (want_gui)
+ if (lyx_gui::use_gui)
lyx_gui::parse_lyxrc();
vector<string> files;
@@ -278,13 +296,13 @@ void LyX::priv_exec(int & argc, char * a
bool success = false;
if (last_loaded->dispatch(batch_command, &success)) {
quitLyX(false);
- exit(!success);
+ lyx_exit(!success);
}
}
files.clear(); // the files are already loaded
}
- if (want_gui) {
+ if (lyx_gui::use_gui) {
// determine windows size and position, from lyxrc and/or session
// initial geometry
unsigned int width = 690;
@@ -296,10 +314,10 @@ void LyX::priv_exec(int & argc, char * a
}
// if lyxrc returns (0,0), then use session info
else {
- string val = LyX::ref().session().loadSessionInfo("WindowWidth");
+ string val = session().loadSessionInfo("WindowWidth");
if (!val.empty())
width = convert<unsigned int>(val);
- val = LyX::ref().session().loadSessionInfo("WindowHeight");
+ val = session().loadSessionInfo("WindowHeight");
if (!val.empty())
height = convert<unsigned int>(val);
}
@@ -307,10 +325,10 @@ void LyX::priv_exec(int & argc, char * a
int posx = -1;
int posy = -1;
if (lyxrc.geometry_xysaved) {
- string val = LyX::ref().session().loadSessionInfo("WindowPosX");
+ string val = session().loadSessionInfo("WindowPosX");
if (!val.empty())
posx = convert<int>(val);
- val = LyX::ref().session().loadSessionInfo("WindowPosY");
+ val = session().loadSessionInfo("WindowPosY");
if (!val.empty())
posy = convert<int>(val);
}
@@ -318,7 +336,7 @@ void LyX::priv_exec(int & argc, char * a
} else {
// Something went wrong above
quitLyX(false);
- exit(EXIT_FAILURE);
+ lyx_exit(EXIT_FAILURE);
}
}
@@ -430,7 +448,7 @@ void LyX::printError(ErrorItem const & e
}
-void LyX::init(bool gui)
+void LyX::init()
{
#ifdef SIGHUP
signal(SIGHUP, error_handler);
@@ -441,9 +459,6 @@ void LyX::init(bool gui)
signal(SIGTERM, error_handler);
// SIGPIPE can be safely ignored.
- // Disable gui when easyparse says so
- lyx_gui::use_gui = gui;
-
lyxrc.tempdir_path = package().temp_dir();
lyxrc.document_path = package().document_dir();
@@ -478,7 +493,7 @@ void LyX::init(bool gui)
// Check that user LyX directory is ok. We don't do that if
// running in batch mode.
- if (gui) {
+ if (lyx_gui::use_gui) {
if (queryUserLyXDir(package().explicit_user_support()))
reconfigureUserLyXDir();
} else {
@@ -507,7 +522,7 @@ void LyX::init(bool gui)
lyxerr[Debug::INIT] << "Reading layouts..." << endl;
LyXSetStyle();
- if (gui) {
+ if (lyx_gui::use_gui) {
// Set up bindings
toplevel_keymap.reset(new kb_keymap);
defaultKeyBindings(toplevel_keymap.get());
@@ -540,7 +555,7 @@ void LyX::init(bool gui)
// close to zero. We therefore don't try to overcome this
// problem with e.g. asking the user for a new path and
// trying again but simply exit.
- exit(EXIT_FAILURE);
+ lyx_exit(EXIT_FAILURE);
}
if (lyxerr.debugging(Debug::INIT)) {
@@ -688,7 +703,7 @@ bool LyX::queryUserLyXDir(bool explicit_
_("&Create directory"),
_("&Exit LyX"))) {
lyxerr << _("No user LyX directory. Exiting.") << endl;
- exit(1);
+ lyx_exit(EXIT_FAILURE);
}
lyxerr << bformat(_("LyX: Creating directory %1$s"),
@@ -699,7 +714,7 @@ bool LyX::queryUserLyXDir(bool explicit_
// Failed, so let's exit.
lyxerr << _("Failed to create directory. Exiting.")
<< endl;
- exit(1);
+ lyx_exit(EXIT_FAILURE);
}
return true;
Index: src/lyx_main.h
===================================================================
--- src/lyx_main.h (Revision 14032)
+++ src/lyx_main.h (Arbeitskopie)
@@ -34,7 +34,21 @@ namespace lyx {
/// initial startup
class LyX : boost::noncopyable {
public:
+ /**
+ * Execute LyX. The startup sequence is as follows:
+ * -# LyX::exec()
+ * -# LyX::priv_exec()
+ * -# lyx_gui::exec()
+ * -# LyX::exec2()
+ * Step 3 is omitted if no gui is wanted. We need lyx_gui::exec()
+ * only to create the QApplication object in the qt frontend. All
+ * attempts with static and dynamically allocated QApplication
+ * objects lead either to harmless error messages on exit
+ * ("Mutex destroy failure") or crashes (OS X).
+ */
static void exec(int & argc, char * argv[]);
+ /// Execute LyX (inner execution loop, \sa exec)
+ void exec2(int & argc, char * argv[]);
static LyX & ref();
static LyX const & cref();
@@ -58,7 +72,7 @@ private:
void priv_exec(int & argc, char * argv[]);
/// initial LyX set up
- void init(bool);
+ void init();
/// set up the default key bindings
void defaultKeyBindings(kb_keymap * kbmap);
/// set up the default dead key bindings if requested
