Abdelrazak Younes wrote:
> Peter Kümmel wrote:
>> In the last patch is a bug for Mac.
>> Here the corrected.
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: src/lyxfunc.C
>> ===================================================================
>> --- src/lyxfunc.C (Revision 16123)
>> +++ src/lyxfunc.C (Arbeitskopie)
>> @@ -1034,12 +1034,6 @@
>> break;
>>
>> case LFUN_LYX_QUIT:
>> - if (argument == "closeOnly") {
>> - if (!theApp()->gui().closeAll())
>> - break;
>> - lyx_view_ = 0;
>> - }
>> -
>> // FIXME: this code needs to be transfered somewhere else
>> // as lyx_view_ will most certainly be null and a same
>> buffer
>> // might be visible in more than one LyXView.
>> @@ -1048,8 +1042,9 @@
>>
>> LyX::ref().session().lastFilePos().save(lyx_view_->buffer()->fileName(),
>> boost::tie(view()->cursor().pit(),
>> view()->cursor().pos()) );
>> }
>> -
>> - LyX::ref().quit();
>> +
>> + // quitting is trigged by the gui code (leaving the event
>> loop)
>> + theApp()->gui().closeAllRegisteredViews();
>
> What's the difference between this and closeAll()? Can a view be
> unregistered?
Yes, this is one of the most important change, unregister only in
the CloseEvent and quit when no views left.
>
>> break;
>>
>> case LFUN_TOC_VIEW: {
>> @@ -1670,7 +1665,7 @@
>> BOOST_ASSERT(lyx_view_);
>> BOOST_ASSERT(theApp());
>> lyx_view_->close();
>> - // We return here because lyx_view does not exists anymore.
>> + lyx_view_->closed(lyx_view_->id());
>
> I am not sure this signal is caught anywhere. I created it at some point
> but I did not make use of it at the end. You should maybe check whether
> it is used or not.
>
Ok.
>> return;
>>
>> case LFUN_BOOKMARK_GOTO: {
>> Index: src/frontends/Application.h
>> ===================================================================
>> --- src/frontends/Application.h (Revision 16123)
>> +++ src/frontends/Application.h (Arbeitskopie)
>> @@ -168,8 +168,7 @@
>> * remove a I/O read callback
>> * @param fd socket descriptor (file/socket/etc)
>> */
>> - template<class T>
>> - void unregisterSocketCallback(T fd);
>> + virtual void unregisterSocketCallback(int fd) = 0;
>
> Yes, that's definitely better than the template based solution.
It also was not a correct solution.
>
>>
>> /// Create the main window with given geometry settings.
>> LyXView & createView(unsigned int width, unsigned int height,
>> Index: src/frontends/qt4/GuiImplementation.C
>> ===================================================================
>> --- src/frontends/qt4/GuiImplementation.C (Revision 16123)
>> +++ src/frontends/qt4/GuiImplementation.C (Arbeitskopie)
>> @@ -30,103 +30,91 @@
>> namespace frontend {
>>
>>
>> -GuiImplementation::GuiImplementation(): max_view_id_(0), max_wa_id_(0)
>> +GuiImplementation::GuiImplementation()
>
> I'd rather keep the max_view_id_ vector as this is used quite often in
max_view_id_ is private; but, yes, viewIds() is now much more expensive.
> LyX::updateInset(). But this LyX::updateInset() is not good at all so
> we'd better clean this up at the same time.
How? Must first have a look at it. Maybe we could make viewIds() private.
>
>> -int GuiImplementation::newView()
>> +LyXView& GuiImplementation::createRegisteredView()
>
> Same question: can a View be unregistered? If not, then newView() is
> more explicit IMHO.
>
>> {
>> - size_t const id = max_view_id_;
>> - ++max_view_id_;
>> -
This is totally wrong, fixed in the new patch.
>> - views_[id] = new GuiView(id);
>> - view_ids_.push_back(id);
>> -
>> - return id;
>> + size_t const id = viewIds().size();
>> + views_.insert(std::pair<int, GuiView *>(id, new GuiView(id)));
>> + return *views_[id];
>> }
>>
>>
>> -LyXView& GuiImplementation::view(int id)
>> +bool GuiImplementation::unregisterView(int id)
>> {
>> - BOOST_ASSERT(views_.find(id) != views_.end());
>> + if (id < 0 && id >= viewIds().size())
>> + return false;
>> + if (!views_[id])
>> + return false;
>>
>> - return *views_[id];
>> + std::map<int, GuiView *>::iterator it;
>> + for (it = views_.begin(); it != views_.end(); ++it) {
>> + if (it->first == id) {
>> + std::vector<int> const & wa_ids = it->second->workAreaIds();
>> + for (size_t i = 0; i < wa_ids.size(); ++i)
>> + work_areas_.erase(wa_ids[i]);
>> + views_.erase(id);
>> + break;
>> + }
>> + }
>> + return true;
>> }
>>
>>
>> -bool GuiImplementation::closeAll()
>> +bool GuiImplementation::closeAllRegisteredViews()
>> {
>> - // ATM never used
>> - if (!theBufferList().quitWriteAll())
>> - return false;
>> + if (views_.empty())
>> + return true;
>>
>> - // In order to know if it is the last opened window,
>> - // GuiView::closeEvent() check for (view_ids_.size() == 1)
>> - // We deny this check by setting the vector size to zero.
>> - // But we still need the vector, hence the temporary copy.
>> - std::vector<int> view_ids_tmp = view_ids_;
>> - view_ids_.clear();
>> -
>> - for (size_t i = 0; i < view_ids_tmp.size(); ++i) {
>> - // LFUN_LYX_QUIT has already been triggered so we need
>> - // to disable the lastWindowClosed() signal before closing
>> - // the last window.
>> - views_[view_ids_tmp[i]]->setAttribute(Qt::WA_QuitOnClose,
>> false);
>> - views_[view_ids_tmp[i]]->close();
>> - // The view_ids_ vector is reconstructed in the closeEvent; so
>> - // let's clear that out again!
>> - view_ids_.clear();
>> + std::map<int, GuiView*> const cmap = views_;
>> + std::map<int, GuiView*>::const_iterator it;
>> + for (it = cmap.begin(); it != cmap.end(); ++it)
>> + {
>> + it->second->close();
>> + // unregisterd by the CloseEvent
>> }
>> -
>> views_.clear();
>> - view_ids_.clear();
>> work_areas_.clear();
>> -
>> return true;
>> }
>>
>> -
>> -void GuiImplementation::unregisterView(GuiView * view)
>> +LyXView& GuiImplementation::view(int id) const
>> {
>> - std::map<int, GuiView *>::iterator I;
>> + BOOST_ASSERT(views_.find(id) != views_.end());
>> + return *views_.find(id)->second;
>> +}
>>
>> - for (I = views_.begin(); I != views_.end(); ++I) {
>> - if (I->second == view) {
>> - std::vector<int> const & wa_ids = view->workAreaIds();
>> - for (size_t i = 0; i < wa_ids.size(); ++i)
>> - work_areas_.erase(wa_ids[i]);
>>
>> - views_.erase(I->first);
>> - break;
>> - }
>> - }
>> +template<class T>
>> + std::vector<int> const & GuiImplementation_Ids
>
> This is a bad name :-). It is just a helper function that uses nothing
> to do with GuiImplementation so just call it extractIds() and put that
> in the anon namespace.
Yes, that's better.
>> + (std::map<int, T*> const & stdmap, std::vector<int> & ids) +{
>> + ids.clear();
>> + typename std::map<int, T*>::const_iterator it;
>> + for (it = stdmap.begin(); it != stdmap.end(); ++it)
>> + ids.push_back(it->first);
>> + return ids;
>> +}
>>
>> - buildViewIds();
>>
>> - if (views_.empty()) {
>> - theLyXFunc().setLyXView(0);
>> - dispatch(FuncRequest(LFUN_LYX_QUIT));
>> - return;
>> - }
>> -
>> - theLyXFunc().setLyXView(views_.begin()->second);
>> +std::vector<int> const & GuiImplementation::viewIds()
>> +{
>> + return GuiImplementation_Ids(views_, view_ids_);
>> }
>>
>>
>> -void GuiImplementation::buildViewIds()
>> +std::vector<int> const & GuiImplementation::workAreaIds()
>> {
>> - view_ids_.clear();
>> - std::map<int, GuiView *>::const_iterator I;
>> - for (I = views_.begin(); I != views_.end(); ++I)
>> - view_ids_.push_back(I->first);
>> + return GuiImplementation_Ids(work_areas_, work_area_ids_);
>> }
>>
>>
>> int GuiImplementation::newWorkArea(unsigned int w, unsigned int h,
>> int view_id)
>> {
>> - size_t const id = max_wa_id_;
>> - ++max_wa_id_;
>> + size_t const id = workAreaIds().size();
>>
>> GuiView * view = views_[view_id];
>>
>> @@ -147,7 +135,6 @@
>> WorkArea& GuiImplementation::workArea(int id)
>> {
>> BOOST_ASSERT(work_areas_.find(id) != work_areas_.end());
>> -
>> return *work_areas_[id];
>> }
>>
>> Index: src/frontends/qt4/GuiImplementation.h
>> ===================================================================
>> --- src/frontends/qt4/GuiImplementation.h (Revision 16123)
>> +++ src/frontends/qt4/GuiImplementation.h (Arbeitskopie)
>> @@ -38,19 +38,18 @@
>> GuiImplementation();
>> virtual ~GuiImplementation() {}
>>
>> - virtual int newView();
>> - virtual LyXView& view(int id);
>> +
>> + virtual LyXView& createRegisteredView();
>> + virtual bool closeAllRegisteredViews();
>> + virtual bool unregisterView(int id);
>> +
>> + virtual LyXView& view(int id) const;
>> + virtual std::vector<int> const & viewIds();
>> +
>> virtual int newWorkArea(unsigned int width, unsigned int height,
>> int view_id);
>> virtual WorkArea& workArea(int id);
>> - virtual bool closeAll();
>>
>> -public Q_SLOTS:
>> - ///
>> - void unregisterView(GuiView * view);
>> -
>> private:
>> - ///
>> - void buildViewIds();
>>
>> /// Multiple views container.
>> /**
>> @@ -67,9 +66,9 @@
>> */
>> std::map<int, GuiWorkArea *> work_areas_;
>> ///
>> - size_t max_view_id_;
>> - ///
>> - size_t max_wa_id_;
>> + std::vector<int> const & workAreaIds();
>> +
>> + std::vector<int> work_area_ids_;
>> };
>>
>> } // namespace frontend
>> Index: src/frontends/qt4/GuiApplication.C
>> ===================================================================
>> --- src/frontends/qt4/GuiApplication.C (Revision 16123)
>> +++ src/frontends/qt4/GuiApplication.C (Arbeitskopie)
>> @@ -277,14 +277,8 @@
>> boost::shared_ptr<socket_callback>(new socket_callback(fd,
>> func));
>> }
>>
>> -template<>
>> -void Application::unregisterSocketCallback<int>(int fd)
>> -{
>> - GuiApplication* ptr = static_cast<GuiApplication*>(this);
>> - ptr->unregisterSocketCallbackImpl(fd);
>> -}
>>
>> -void GuiApplication::unregisterSocketCallbackImpl(int fd)
>> +void GuiApplication::unregisterSocketCallback(int fd)
>> {
>> socket_callbacks_.erase(fd);
>> }
>> Index: src/frontends/qt4/GuiApplication.h
>> ===================================================================
>> --- src/frontends/qt4/GuiApplication.h (Revision 16123)
>> +++ src/frontends/qt4/GuiApplication.h (Arbeitskopie)
>> @@ -75,7 +75,7 @@
>> virtual void updateColor(LColor_color col);
>> virtual void registerSocketCallback(
>> int fd, boost::function<void()> func);
>> - void unregisterSocketCallbackImpl(int fd);
>> + void unregisterSocketCallback(int fd);
>> //@}
>>
>> ///
>> Index: src/frontends/qt4/GuiView.C
>> ===================================================================
>> --- src/frontends/qt4/GuiView.C (Revision 16123)
>> +++ src/frontends/qt4/GuiView.C (Arbeitskopie)
>> @@ -155,6 +155,9 @@
>> GuiView::GuiView(int id)
>> : QMainWindow(), LyXView(id), commandbuffer_(0), d(*new
>> GuiViewPrivate)
>> {
>> + setAttribute(Qt::WA_QuitOnClose, false);
>> + //setAttribute(Qt::WA_DeleteOnClose, false);
>> +
>> // hardcode here the platform specific icon size
>> d.smallIconSize = 14; // scaling problems
>> d.normalIconSize = 20; // ok, default
>> @@ -210,6 +213,19 @@
>> updateMenubar();
>> }
>>
>> +void GuiView::closeEvent(QCloseEvent * close_event)
>> +{
>> + theApp()->gui().unregisterView(id());
>> + if (theApp()->gui().viewIds().empty())
>> + {
>> + // this is the place were we leave the frontend
>> + // and is the only point were we begin to quit
>> + saveGeometry();
>> + close_event->accept();
>> + // quit the event loop
>> + qApp->quit();
>> + }
>> +}
>
> I think this could be a good solution but the problem here is that I
> think there is a bug in QtMac. This is a theory that could well explain
> Bennett's report about non-saved files on exit:
I also have the impression that QCloseEvent isn't called on the Mac,
I've added an output in the new patch to verify it.
>
> qApp->exit() seems to always results in killing the application. It
> seems that the LyX destructor is always called the moment just after
> that. The execution process in LyX::exec() is thus not properly done and
> prepareExit() is not called here:
qApp->quit(); should only close the event loop and return, but
we will see it with Bennetts backtrace (now I'm pessimistic for the Mac)
>> @@ -402,12 +395,7 @@
>>
>> exit_status = pimpl_->application_->exec();
>>
>> - // FIXME: Do we still need this reset?
>> - // I assume it is the reason for strange Mac crashs
>> - // Test by reverting rev 16110 (Peter)
>> - // Kill the application object before exiting. This avoid crash
>> - // on exit on Linux.
>> - pimpl_->application_.reset();
>> + prepareExit();
>>
>> // Restore original font resources after Application is destroyed.
>> support::restoreFontResources();
>
>
>
> Abdel.
>
The are no new big changes in the new patch.
Thanks for the comments Abdel, this is really a hairy stuff,
costs me several hours.
Peter
