Georg Baum wrote:
Abdel, I compiled your patch and had to make some adjustments to make it
compile. After that, I get a segfault with xforms and gtk on termination
that was not there before. I spent far too much time with this already,
so I did neither run gdb nor valgrind, but I am sure it is related to the
scoped_pointer problems.
Yes most surely. I am going to remove these scoped_ptr and propose
something to solve that.
Except for the crash on exit problem, could you confirm me that the
clipboard functionality remains the same for linux/X11/qt3/xforms/gtk?
screen_.reset(LyXScreenFactory::create(workarea()));
As mentioned by Angus: This is undefined behaviour too (only in qt4) and
needs to be changed.
Yep, I'll change that.
Index: src/frontends/gtk/WorkAreaFactory.C
===================================================================
--- src/frontends/gtk/WorkAreaFactory.C (revision 14001)
+++ src/frontends/gtk/WorkAreaFactory.C (working copy)
@@ -27,11 +27,20 @@
namespace WorkAreaFactory {
+static GWorkArea * work_area_ = 0;
Needs to be
+static lyx::frontend::GWorkArea * work_area_ = 0;
Apart from that it will not work with multiple work areas, so it is a step
backwards.
I will propose a solution that will work in the same way for all
frontends: a base class that will _the_ official interface to the
frontend, something like this:
class frontend
{
virtual ~frontend() {}
Clipboard * clipboard() = 0;
WorkArea * workArea() = 0;
LyXScreen * screen() = 0;
}
Of course this means that I will remove WorkAreaFactory and
LyXScreenFactory. The frontends would be responsible for the object
creations and destructions so there would be no need anymore for scoped_ptr.
Can I work on this? As in "Would it be accepted?"
This breaks compilation in QWorkarea.C, because Xlib.h #defines None,
which is also an enum in qurl.h. Moving this include to Application.C
works, it seems that Xevent gets declared in some qt headers.
OK, thanks.
Index: src/frontends/qt4/LyxClipboard.C
Should be named LyXClipboard.C
Well I don't like much this prefix and I was thinking of going back to
my initial thought and use the same naming:
frontend/Clipboard.h will be in the lyx namespace.
frontend/qt3/Clipboard.[Ch]
frontend/qt4/Clipboard.[Ch]
frontend/gtk/Clipboard.[Ch]
frontend/xforms/Clipboard.[Ch] will be in the lyx::frontend namespace
Are they any strong opinion against that? If any I could use the "Base"
prefix in the virtual base class instead:
frontend/BaseClipboard.h
frontend/BaseWorkArea.h
frontend/BaseScreen.h
frontend/[qt3,qt4,gtk,xforms]/Clipboard.[Ch]
frontend/[qt3,qt4,gtk,xforms]/WorkArea.[Ch]
frontend/[qt3,qt4,gtk,xforms]/Screen.[Ch]
Comments, opinion?
This is the reason for the undefined behaviour in BufferView_pimpl: No new
object is created, but a pointer to the workarea is returned (I know that
the patch does not change that). Scott Meyers has a rule that multiple
inheritance should only be used if there are very good reasons (Item 40
in Effective C++: Use multiple inheritance judiciously), and I think it
should be followed.
I agree that multiple inheritance of non base classes should be avoided.
But, in my opinion, polymorphism thanks to inheritance of multiple base
virtual classes is a very nice concept that should be used.
- // FIXME: this is an odd place to have it, but xforms needs it
here ...
This comment is important. In fact, gtk does also need the workarea in
haveSelection(). Unless that problem is solved first I think that it does
not make sense to separate the clipboard from the workarea, because you
will need to create a connection elsewhere. All in all I think that the
current solution is cleaner and easier to understand than your proposed
split.
In my opinion, the manual connection would be a good incentive to
cleanup the other frontends. As for gtk, my reading of the code makes me
think that it should be very easy to put that out of GWorkarea:
void GWorkArea::haveSelection(bool toHave)
{
if (toHave) {
Glib::RefPtr<Gtk::Clipboard> clipboard =
Gtk::Clipboard::get(GDK_SELECTION_PRIMARY);
std::vector<Gtk::TargetEntry> listTargets;
listTargets.push_back(Gtk::TargetEntry("UTF8_STRING"));
clipboard->set(listTargets,
sigc::mem_fun(const_cast<GWorkArea&>(*this),
&GWorkArea::onClipboardGet),
sigc::mem_fun(const_cast<GWorkArea&>(*this),
&GWorkArea::onClipboardClear));
}
}
AFAIS, the only thing to do is to define the onClipboardGet and
onClipboardClear signals outside of GWorkArea. That location would of
course be the new Clipboard class. John (Spray), would you care to
comment on that please?
As for XForms, it seems very easy to do also:
void XWorkArea::haveSelection(bool yes)
{
Window const owner = yes ? FL_ObjWin(work_area) : None;
XSetSelectionOwner(fl_get_display(), XA_PRIMARY, owner,
CurrentTime);
}
The idea is that if needed, the new Clipboard class could retrieve the
work_area pointer from the main frontend class (as defined above).
From a theoretical point of view it makes sense to say that the clipboard
has nothing to do with the workarea, but as long as xforms and gtk
require it, I think you have to live with the clipboard being part of the
workarea.
I think stagnation is bad. I think that my proposal would ease the path
to multiple workAreas in the future. So, would you like me to work on
that or no?
Thanks for your comments and your help Georg,
Abdel.
