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.) The patch also makes the fontloader a member of the Application. I have big question marks over your shared_ptr<foo> -> foo* changes. I suspect that you should be using weak_ptr<foo> instead. Specific points =============== If BufferView and BufferView::Pimpl now *require* a valid WorkArea, you should pass their c-tors a WorkArea& rather than a WorkArea*. 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. 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. 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. Angus
