Angus Leeming wrote:
Abdelrazak Younes <[EMAIL PROTECTED]> writes:
Here is the next step on my GUI API cleanup. Only qt4 for now.
Committed to the "younes" branch. Comments welcome.
Summary
=======
So a summary of this patch might be: A BufferView requires a valid WorkArea, so
make the code do that. (ATM, the code logic is inverted.)
No, you got it wrong, that's exactly the opposite I want to achieve: a
WorkArea needs a valid BufferView. I want the WorkArea to use the
BufferView and the Bufferview to not even know about the WorkArea existence.
The patch also makes
the fontloader a member of the Application.
Yes, I've said that, no?
I have big question marks over your shared_ptr<foo> -> foo* changes. I suspect
that you should be using weak_ptr<foo> instead.
I'll investigate these weak_ptr later on. This patch was mostly ready
before my discussion with Lars about that. But let me assure of
something: my use of pointers is _very_ safe. They are used as
"reference" that's all. I would be happy to use reference instead but
they have to be set on construction and I want to be able to change them
at runtime.
Specific points
===============
If BufferView and BufferView::Pimpl now *require* a valid WorkArea, you should
pass their c-tors a WorkArea& rather than a WorkArea*.
No, see above.
In LyXView I see this:
- boost::shared_ptr<BufferView> const & view() const;
+ BufferView * view() const;
Is it safe? Who owns the view() now? Can a race occur such that someone tries to
access a view() after it has been deleted? Remember, things like asynchronous
loading of graphics really play havoc with simple program flow.
No, no, please read the patch. The LyXView (GuiView really)
creation/deletion is done at one place: GuiImplementation. There is no
possibility of race condition. I _hate_ "delete", we should banish them
from the code ;-). I reckon the use of weak_ptr would safe guard around
this fear you have that somebody could call a delete on one of those
pointers.
This feels very circular:
-WorkArea::WorkArea(LyXView & owner, int w, int h)
- : greyed_out_(true)
+WorkArea::WorkArea(BufferView * buffer_view)
+ : buffer_view_(buffer_view), greyed_out_(true)
Either WorkArea creation depends on a BufferView or BufferView creation depends
on a WorkArea. Given that WorkArea has a setBufferView method, I'd prefer to see
the WorkArea c-tor just initialize the buffer_view_ member to 0.
This is a temporary state, as I said WorkArea refernce will eventually
disappear from BufferView.
Please justify:
Index: lyx_main.h
- void addLyXView(boost::shared_ptr<LyXView> const & lyxview);
+ void addLyXView(LyXView * lyxview);
Maybe you should be storing boost::weak_ptrs here. Bald pointers are, in
general, evil.
IMHO, not in this case. The problem of shared_ptr is that you don't know
where they are reset. So I think it is wrong to pass shared_ptr around.
They are very useful and I use them in my Gui and GuiImplemtation.
Please find attached the header of these classes.
Thanks for the comments,
Abdel.
Angus
// -*- C++ -*-
/**
* \file GuiImplementation.h
* This file is part of LyX, the document processor.
* Licence details can be found in the file COPYING.
*
* \author John Levon
* \author Abdelrazak Younes
*
* Full author contact details are available in file CREDITS.
*/
#ifndef GUI_H
#define GUI_H
#include "frontends/Gui.h"
#include "GuiClipboard.h"
#include <boost/shared_ptr.hpp>
#include <map>
class LyXView;
namespace lyx {
namespace frontend {
class GuiWorkArea;
class GuiView;
/**
* The GuiImplementation class is the interface to all Qt4 components.
*/
class GuiImplementation: public Gui
{
public:
GuiImplementation();
virtual ~GuiImplementation() {}
Clipboard& clipboard();
int newView(unsigned int width, unsigned int height);
LyXView& view(int id);
void destroyView(int id);
int newWorkArea(unsigned int width, unsigned int height, int view_id);
int newWorkArea(int w, int h);
WorkArea& workArea(int id);
void destroyWorkArea(int id);
private:
///
GuiClipboard clipboard_;
///
std::map<int, boost::shared_ptr<GuiView> > views_;
///
std::map<int, boost::shared_ptr<GuiWorkArea> > work_areas_;
///
size_t max_view_id_;
///
size_t max_wa_id_;
};
} // namespace frontend
} // namespace lyx
#endif // GUI_H
// -*- C++ -*-
/**
* \file Gui.h
* This file is part of LyX, the document processor.
* Licence details can be found in the file COPYING.
*
* \author unknown
* \author John Levon
* \author Abdelrazak Younes
*
* Full author contact details are available in file CREDITS.
*/
#ifndef BASE_GUI_H
#define BASE_GUI_H
#include "frontends/GuiCursor.h"
#include <boost/shared_ptr.hpp>
class LyXView;
namespace lyx {
namespace frontend {
class Clipboard;
class WorkArea;
/**
* A Gui class manages the different frontend elements.
*/
class Gui
{
public:
virtual ~Gui() {}
///
virtual Clipboard& clipboard() = 0;
///
virtual int newView(unsigned int width, unsigned int height) = 0;
///
virtual LyXView& view(int id) = 0;
///
virtual void destroyView(int id) = 0;
///
virtual int newWorkArea(unsigned int width, unsigned int height, int
view_id) = 0;
///
virtual WorkArea& workArea(int id) = 0;
///
virtual void destroyWorkArea(int id) = 0;
///
GuiCursor & guiCursor() {return cursor_;}
protected:
/// view of a buffer. Eventually there will be several.
std::map<int, boost::shared_ptr<BufferView> > buffer_views_;
private:
///
GuiCursor cursor_;
};
} // namespace frontend
} // namespace lyx
#endif // BASE_GUI_H
