From: Michal H. <ms...@gm...> - 2008-03-03 20:48:56
|
On Mon, Mar 03, 2008 at 07:11:49PM +0000, Jozef Misutka wrote: > > > ---------------------------------------- > > Date: Mon, 3 Mar 2008 16:41:00 +0100 > > From: ms...@gm... > > To: mis...@ho... > > CC: pdf...@li... > > Subject: Re: annotations > > > > On Mon, Mar 03, 2008 at 02:06:19PM +0000, Jozef Misutka wrote: > >> > >> I do not think a simple patch will help so i am enclosing my whole kernel directory. you can not compile it for now BUT the problems are not in cannotation. > > > > Differences are really huge. It is very hard to review, so I am > > wondering why don't you develop that in the CVS (in separate branch), so > > that we have full history of your changes. Quite honestly, I don't see > > any easy way how to get this changes to the CVS other than simple put > > clean up in the comment (which I don't find the right way). > > > > you *would* be right with another project but in kernel it takes ages to agree on many issues. Don't understand your point here... What do regular commits have in common with agreeing with issues? > > > Do you plan to announce your changes for comments/review in the devel > > list before they get to the CVS? > > > > no, just changes to your code which you have to ack. I don't think that this is very good idea. Remember that we are developing in several branches and you are doing rather big changes which may be in clash with other branches or even worse bad merges (accidentally not in conflict but depending on some old code). Moreover, it is not a good idea to have only one person who understand core part of code (e.g. there are many places in gui where only Martin understand what is going on - we shouldn't let this happen also in kernel). And IMO when you see the whole source, you don't see as much as in patch where you see differences, so only important part is shown. > > >> > >> NOTE: your code should not have changed and moreover, after this change i will be fully responsible for cpage.{cc,h} (if you want to extend cpage interface or move more lines of annotation code to cannotation.{cc,h} i will have no objections) > > > > This is true, but note that watchdog classes handle CPage internal state > > (page dictionary) and *not* annotation dictionary. It is not very common > > that they are in separate module. Is this really good from design point > > of view? > > (i think :)) it surely is. Of, course that you can change object's internal state *BUT* via public interface (or inherited). But this is not a case. You have to overcome access control with friend (mis)feature. I know that here we are in casual situation, when we could have endless thread about 2 points of view what is better but I don't want to come over it again (quite honestly). Try to discuss with someone else (possibly not the exactly same thinking) and do as you want in the end. Both approaches are *working*. I don't see a point why to do it: we will only have some parts of code logically inconsistent IMHO(*) and maybe cpage.{cc,h} will be under your full responsibility. (*) one of three object will have different watchdog logic and if we would like to change it, then the second part of the sentence (full responsibility) is not true. > > > This is very same for content stream which you have moved outside > > cdict too. Moreover, page tree watchdogs are defined in cpdf and not in > > cpage, so this is kind of inconsistency in design. > > If we would move page tree watchdogs to the cpage (to have this unified) > > then you are in the same situation like before from responsibility > > POV. > > Watchdogs were originally designed to be hidden from public, now they > > are visible to the world. > > I don't think that this is a good idea too. > > Why would someone (besides object it was developed for) need it? > > 1) they are friends > 2) when something is developed for an object it does not need to be in that object! see above (and motto - keep it simple - friends are not simple ;)). > > > > > getAnnotsArray and collectAnnotations also deals with page dictionary > > (page internals), so I would look for it in cpage.c too. But I have no > > strong opinion on that because it is just helper which doesn't change > > page dictionary (and we don't have to be afraid that page dictionary > > format will change in future). > > either you leave it there and i will change it to match my views or you can put it to cannotation. ??? (You may do it or I will do it) ???? > > basically, cpage consists of several modular parts like annotations, fonts, display module, etc.. therefore, cpage is just the access point where calls are delegated to these modules. it is 1) more modular==> easily tested, more usable, 2) less error prone So it is cpdf consisting of several pages... And I don't want to put page tree observers to the cpage (and so you do i think). But I understand and agree that smaller modules better overview. > > the pdf edit design is very difficult because we access objects in two different ways, through "high level" objects like cpage and through "raw" objects in object tree. i came with the idea of observers mainly because of the second approach and it can be easily seen that it does not make sense to have every implementation of observer in the object itself.. and moreover, note that observers are observing dictionary objects or their items not cpage itself! CPage is wrapper for page dictionary IIRC, so CPage (observing its own page dictionary) should handle all changes (and this is the point i have tried to show). > > hopefully, my design views are clearer now. > > jozo -- Michal Hocko |