Abdelrazak Younes wrote > 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 &);
I'll leave it to the build experts (Lars? JMarc?) to comment further on this. Personally, it leaves me feeling uncomfortable. >> 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 App > lication class. It looks like undefined behaviour to me. scoped_ptr "owns" the pointer and will call delete on it. Two pointers to the same bit of memory? It may not go boom for you, but it sure will for someone. Get someone with access to a 32bit linux machine to run this code through valgrind. It'll tell you definitely whether I'm seeing ghosts where none exist. >> 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. I don't think I explained what I meant here very well. I'm worried again by the scoped_ptr that ostensibly owns the Clipboard variable. In this case, the pointer is pointing to an object of the stack, not the heap. Calling delete on such a beast is undefined behaviour. > I am not ignoring you, I explained that to you already. I > agree with you but for now, until the other frontends implem > ent 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? <shrug> What's the point of posting broken code and asking for a review if you don't want to hear the comments? A long, long time ago (way before your time) a gentleman by the name of Lars posted a mail explaining how to use branches and stating that he'd like to see them used a lot more for smallish stuff like this that would benefit from polish. So, having asked for comments, here're mine: IMO your code is broken. I can't see what would be lost by setting up a branch and can see that lots can be gained by doing so. Having said all that, I'm no longer an active dev of this project and you are. Your opinion therefore carries far more weight than mine. </shrug> >> Could you add a comment that haveSelection should become >> const as/when the other frontends are tidied up? > Why? haveSelection() modify the clipboard: Ok, my misunderstanding. > 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. It may be that all you need to do is take the BufferView::Pimpl::Clipboard variable out of the scoped_ptr. Dunno. As things stand, you're introducing undefined behaviour. Sounds like you've introduced undefined behaviour already elsewhere with your (screen_ and workarea_) pointers. > No, sorry but if you don't accept my non-qt4 patches I wil > l turn around the (IMHO) deficiencies of the lyx API in the > qt4 frontend, as I have already done with LyxScreen and Work > Area. This time I wanted to help the other frontends but if > this is badly perceived, I will back off. Don't be so touchy, please. I'm attacking neither you personally nor your code. I put in a couple of hours of a Sunday evening to write what I believe are well considered and reasonably intelligent critiques of your code. I can certainly find other things to do with my time if you'd rather. > 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. As you choose. Good night, Angus
