Angus Leeming wrote:
I'm nervous about your comment to Bo about the Sconscript having a
circlar qt4->frontends->qt4 dependency. We avoid such dependencies by
use of a factory method to generate concrete instances of classes
whose interface is defined in src/frontends. All subsequent use is
through the abstract base class defined in src/frontends. I see you
have a clipboard factory function; do you really have a circular
dependency? Where? How?
It is nothing troublesome, don't worry. The circular dependency is due
to the two helper functions:
> toClipboardStr(std::string const &); std::string
> fromClipboardStr(std::string const &);
>
> What was the rationale for not merging these into the existing
> toqstr(), fromqstr()?
I have asked the same question once and JMarc answered me that they were
only needed for clipboard support not for general GUI text.
I guess the solution for now is to move those functions inside
LyxClipboard. I wanted those helper functions to be available for other
frontends... Maybe in "support/lstrings.h"?
Having "WorkAreaFactory::getClipboard" create a new clipboard
variable and "Clipboard::getClipboard" return the contents of the
clipboard as a std::string is confusing IMO. Call the WorkAreaFactory
version "createClipboard". Ahhhh; is this an artefact of your
decision to derive FrontendWorkArea from Clipboard?
Yes. But again, those methods were part of WorkArea before so I am not
So the core sees
two pointers (WorkArea, Clipboard) to a single memory location? How
does that work when you save the Clipboard pointer in a
boost::scoped_ptr? Looks to me like it's guaranteed to break for
frontend != Qt4.
I don't know and that's why I am asking others to test for me. But I
think it is fine even if this scoped_ptr is overkill, I should have
chosen a simple pointer. The reason why I am confident is that, in qt4,
the scoped_ptrs around LyXScreen and WorkArea (screen_ and workarea_)
work fine even though they point to the same object (QWorkArea) and this
object is not created in WorkAreaFactory::create but in my new
Application class.
Even for frontend == Qt4 I fail to see how this
works as you appear to store the Clipboard object in
lyx::frontend::Application:
I have done that on your demand and I think you were right... I think
you are missing the fact that QWorkArea doesn't derive from Clipboard
and actually doesn't do any clipboard operation anymore.
+class Application : public QApplication +{ +private: +
LyxClipboard clipboard_; +};
In src/frontends/Clipboard.[Ch] you introduce std::string
So?
You don't need the empty constructor.
Right, I'll remove it.
I've already mentioned that I'd prefer getClipboard, putClipboard to
be renamed. I guess you're ignoring me.
I am not ignoring you, I explained that to you already. I agree with you
but for now, until the other frontends implement the Clipboard/WorkArea
split-up they are more meaningful in those frontends like it is now. I
just want to minimize the change to those frontend, what is wrong with that?
FWIW, I think you should also
rename haveSelection(bool) as hasSelection(bool) and th "X clipboard"
comment would appear to be valid also for Windows/Mac.
I take note.
Further
thought: as/when the Selection idea is implemented, would
haveSelection not be better named as hasClipboard? Or even
"!empty()"?. Would Selection be implemented as a Selection class?
(See below for more on this idea.)
Could you add a comment that haveSelection should become const
as/when the other frontends are tidied up?
Why? haveSelection() modify the clipboard:
owner_->clipboard()->setText(QString(), CLIPBOARD_MODE);
I am probably missing something here but a const method should be
reserved to non methods that don't touch anything inside the class.
In src/frontends/gtk/WorkAreaFactory.C you introduce a new static
global variable. Why is that? Looks to me like its a side effect of
your decision to derive GWorkArea from WorkArea and Clipboard.
Yes.
src/frontends/qt4/LyxClipboard.C, class LyxClipboard: why the name? I
thought you were in favour of something like
lyx::frontend::qt::Clipboard?
I was and still am but Andre said he wasn't.
I like the general thrust of this patch, but I have more questions
than answers about its current implementation. If Georg doesn't care
about your desire to separate the clipboard into a separate object,
I'm damn sure he'll care that your implementation of same breaks the
other frontends.
Except for potential compil problem, I am quite sure I won't break
them. I you look more closely to the non-qt4 change, they are very minimal.
Perhaps you should put this into a short-lived branch, so that you
can get the interface right and get others to implement the other
frontends if you're not willing to do so yourself.
No, sorry but if you don't accept my non-qt4 patches I will turn around
the (IMHO) deficiencies of the lyx API in the qt4 frontend, as I have
already done with LyxScreen and WorkArea. This time I wanted to help the
other frontends but if this is badly perceived, I will back off.
If you do so, you
might think about the interface to "Edwin's" selection class
class Selection { public: virtual bool empty() const = 0; virtual
std::string const contents() const = 0; virtual void
contents(std::string const &) = 0; };
and how this interface appears to be identical to that of your
Clipboard.
Yes, the goal is enhance the clipboard class with what Georg and Edwin
have proposed. But after. I don't understand why an interface should be
perfect before it enters the trunk. Interface can evolve also. Beside,
lyx interfaces are far from perfect.
Abdel.
